Register direct controller in init() #1910

yuwenma · 2024-05-30T05:36:52Z

Change description

Currently, developers need to register each direct controller in multiple places, and needs to list their dedicated GVK values.

This PR proposes two changes:

Register the direct controller's "Model" via go init. This should simplify the code and increase the readability as: developing a direct controller starts from register a Model --> the Model implements a Adapter --> the Adapter implements crud + export.
~~Introduce a new CRD label cnrm.cloud.google.com/directcrd=true, so~~ developers do not need to list the new resource GVK in both CC registry and the e2e test base workflow. ~~This label approach is the same as the TF and DCL based CRDs.~~ Direct controller will be the default controller to use. If using TF/DCL controller, developers need to use the corresponding CRD labels. KCC_USE_DIRECT_RECONCILERS flag will override the TF/DCL label to force using the direct controller.

How to use

Register your direct controller in pkg/controller/direct/register/register.go

import  _ "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct/apikeys"

Register your direct controller Model to directbase.ControllerBuilder

func init() {
	directbase.ControllerBuilder.RegisterModel(krm.APIKeysKeyGVK, func (config *controller.Config) directbase.Model)
}

pkg/controller/direct/register/register.go

yuwenma · 2024-05-30T07:07:20Z

scripts/github-actions/tests-e2e-direct.sh

This file is no longer needed since the direct controller e2e test will be covered in tests-e2e-fixtures.

Are we able to turn on the golden checks in tests-e2e-fixtures? That's the big advantage of these tests, they are actually much stricter....

Are we able to turn on the golden checks in tests-e2e-fixtures?

I think we should. Do you happen to know why we don't add the golden check in tests-e2e-fixtures previously?

I plan to turn on the stricter golden check on the generic e2e fixtures (that would be in a separate PR). Right now, I put back the direct e2e path.

justinsb

Looks good, a few nits/suggestions

justinsb · 2024-05-30T14:40:52Z

...extensions.k8s.io_v1_customresourcedefinition_apikeyskeys.apikeys.cnrm.cloud.google.com.yaml

@@ -5,10 +5,10 @@ metadata:
    cnrm.cloud.google.com/version: 0.0.0-dev
  creationTimestamp: null
  labels:
+    cnrm.cloud.google.com/directcrd: "true"


Nit: cnrm.cloud.google.com/direct (it's the controller that's direct, not really the CRD). Or it could just the default if we don't specify tf2crd or dcl2crd ?

I'll change this to "cnrm.cloud.google.com/direct". And +1 to make the direct as default eventually.

I rethink about this. I think we can be more "direct aggressive" here (and we should be good if we do):

By default we expect a resource to use direct controller

if to use TF/DCL controller, the CRD has to have the corresponding label.(current behavior)

justinsb · 2024-05-30T14:41:39Z

pkg/controller/direct/apikeys/apikeyskey_controller.go

-func AddKeyReconciler(mgr manager.Manager, config *controller.Config, opts directbase.Deps) error {
-	gvk := krm.APIKeysKeyGVK
+func init() {
+	directbase.ControllerBuilder.RegisterModel(krm.APIKeysKeyGVK, NewModel)


I like this!

pkg/controller/direct/directbase/directbase_controller.go

justinsb · 2024-05-30T14:46:58Z

pkg/controller/registration/registration_controller.go

@@ -247,6 +221,14 @@ func registerDefaultController(r *ReconcileRegistration, config *controller.Conf
 		}

 	default:
+		// register controllers for direct CRDs
+		if val, ok := crd.Labels[k8s.DirectCRDLabel]; ok && val == "true" {


Might be easier to have a directbase.ControllerBuilder.IsDirect helper, than to require the additional label.

I don't have a preference. This is just to keep the same code style as below (line 232 and line 241).

so as the function name "Add" vs "AddController"

justinsb · 2024-05-30T14:48:00Z

scripts/github-actions/tests-e2e-direct.sh

Are we able to turn on the golden checks in tests-e2e-fixtures? That's the big advantage of these tests, they are actually much stricter....

gemmahou · 2024-05-30T17:38:35Z

I noticed the vcr test failed due to test timeout on "apikeyskey". Unsure if that's related to this PR, but we can fix it by

Regenerate the vcr log and make sure the "replay" pass locally
Skip this test, and I'll regenerate the log later and re-enable it. An example: https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/config/tests/samples/create/harness.go#L681-L682, we'll need to comment out this test from the allow list.

acpana · 2024-05-30T17:55:09Z

apis/apikeys/v1alpha1/apikey_type.go

@@ -41,7 +41,7 @@ var (
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 // +kubebuilder:resource:categories=gcp,shortName=gcpapikeyskey;gcpapikeyskeys
 // +kubebuilder:subresource:status
-// +kubebuilder:metadata:labels="cnrm.cloud.google.com/tf2crd=true";"cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=alpha";"cnrm.cloud.google.com/system=true"


With this change, can we still have resources in transition? For instance, for ApiKey AFAICT, we didn't actually make the switch to a direct CRD but I could be wrong.

What I like about the current state of the world is that we can turn on TF/ DCL based resources to use the DirectController via the feature flag (KCC_USE_DIRECT_RECONCILERS) and have developers iterate between the behavior of a TF/ DCL based controller and a direct one. Can we preserve that somehow?

With this change, can we still have resources in transition?

Yes, we can. The KCC_USE_DIRECT_RECONCILERS should still work as it is.

acpana · 2024-05-30T17:56:17Z

.github/workflows/presubmit.yaml

-          go-version-file: 'go.mod'
-      - name: "run tests-e2e-direct"
-        run: |
-          ./scripts/github-actions/tests-e2e-direct.sh


I think in the end state we don't have direct tests specifically but I think these can be useful for folks, especially as they iterate on a resource conversion.

The KCC_USE_DIRECT_RECONCILERS flag shall still work (which gives developers the testing approach during the resource conversion). For release, we can purely rely on the CRD annotations to tell whether a direct or TF controller shall manage a resource kind (and eventually direct will be the default so it does not need a annotation).

One thing I'm not sure (as @justinsb pointed out) is that if we can turn on golden checks for all e2e-fixture tests. Maybe we can try the strict rules and see how big the fix need to be. But anyways, I plan to keep the tests-e2e-dierct.sh for now until the generic e2e has the golden check.

yuwenma · 2024-06-01T14:58:07Z

pkg/controller/registration/registration_controller.go

@@ -196,44 +192,17 @@ func registerDefaultController(r *ReconcileRegistration, config *controller.Conf
 		JitterGen:    r.jitterGenerator,
 		Defaulters:   r.defaulters,
 	}
-
 	var schemaUpdater k8s.SchemaReferenceUpdater
 	if kccfeatureflags.UseDirectReconciler(gvk.GroupKind()) {


Just a note for reviewers: The previous code only turn on LLM in direct controller, all other resources that already have direct controller and API are only turning on via the KCC_USE_DIRECT_RECONCILERS flag.
When those resources are ready for direct controller, we can simply remove their "cnrm.cloud.google.com/tf2crd=true" label.

cc @justinsb @acpana

pkg/controller/direct/alloydb/cluster_controller.go

pkg/controller/direct/directbase/directbase_controller.go

justinsb · 2024-06-03T22:12:17Z

pkg/controller/registration/registration_controller.go

@@ -196,44 +192,17 @@ func registerDefaultController(r *ReconcileRegistration, config *controller.Conf
 		JitterGen:    r.jitterGenerator,
 		Defaulters:   r.defaulters,
 	}
-
 	var schemaUpdater k8s.SchemaReferenceUpdater
 	if kccfeatureflags.UseDirectReconciler(gvk.GroupKind()) {


justinsb · 2024-06-03T22:14:11Z

pkg/test/controller/reconciler/testreconciler.go

-
-			return directbase.NewReconciler(r.mgr, immediateReconcileRequests, resourceWatcherRoutines, loggingapis.LoggingLogMetricGVK, m, jg)
+		if directbase.ControllerBuilder.IsDirectByCRD(crd) {
+			return directbase.ControllerBuilder.NewReconciler(r.mgr, &kcccontroller.Config{HTTPClient: r.httpClient}, immediateReconcileRequests, resourceWatcherRoutines, crd.GroupVersionKind(), jg)


Nit: IIRC crd.GroupVersionKind() is problematic, ideally ControllerBuilder would record the GVK

justinsb · 2024-06-03T22:14:33Z

Thanks @yuwenma

/approve
/lgtm

google-oss-prow · 2024-06-03T22:14:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yuwenma assigned justinsb May 30, 2024

yuwenma commented May 30, 2024

View reviewed changes

pkg/controller/direct/register/register.go Outdated Show resolved Hide resolved

yuwenma force-pushed the register-direct-controller branch from ee939db to c97f73d Compare May 30, 2024 07:05

yuwenma commented May 30, 2024

View reviewed changes

yuwenma force-pushed the register-direct-controller branch from c97f73d to f3f8760 Compare May 30, 2024 08:08

justinsb reviewed May 30, 2024

View reviewed changes

acpana reviewed May 30, 2024

View reviewed changes

yuwenma force-pushed the register-direct-controller branch 5 times, most recently from df62dc3 to 0483854 Compare June 1, 2024 14:49

yuwenma commented Jun 1, 2024

View reviewed changes

yuwenma force-pushed the register-direct-controller branch 5 times, most recently from 6b2a985 to 5794e45 Compare June 1, 2024 16:04

yuwenma added 3 commits June 3, 2024 18:34

Register direct controller from crd annotation

60ebcec

further clean up

3e70af2

No direct CRD labels; direct will be the expected default

9a749a4

yuwenma force-pushed the register-direct-controller branch from 5794e45 to 9a749a4 Compare June 3, 2024 18:35

justinsb reviewed Jun 3, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Jun 3, 2024

google-oss-prow bot added the approved label Jun 3, 2024

google-oss-prow bot merged commit ea7ee5c into GoogleCloudPlatform:master Jun 3, 2024
13 checks passed

yuwenma mentioned this pull request Jun 4, 2024

chore: register direct controller from crd annotation (part 2) #1935

Merged

yuwenma changed the title ~~Register direct controller from crd annotation~~ Register direct controller in init() Jun 5, 2024

yuwenma mentioned this pull request Jun 5, 2024

tests: simplify monitoringdashboard test case, add to tests-e2e-direct #1861

Merged

yuwenma added this to the 1.119 milestone Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register direct controller in init() #1910

Register direct controller in init() #1910

yuwenma commented May 30, 2024 •

edited

Loading

yuwenma May 30, 2024

justinsb May 30, 2024

yuwenma May 30, 2024

yuwenma May 31, 2024

justinsb left a comment

justinsb May 30, 2024

yuwenma May 30, 2024

yuwenma May 31, 2024 •

edited

Loading

justinsb May 30, 2024

justinsb May 30, 2024

yuwenma May 31, 2024

yuwenma May 31, 2024

justinsb May 30, 2024

gemmahou commented May 30, 2024

acpana May 30, 2024

yuwenma May 30, 2024 •

edited

Loading

acpana May 30, 2024

yuwenma May 31, 2024

yuwenma May 31, 2024

yuwenma Jun 1, 2024 •

edited

Loading

justinsb Jun 3, 2024

justinsb Jun 3, 2024

justinsb Jun 3, 2024

justinsb commented Jun 3, 2024

google-oss-prow bot commented Jun 3, 2024

Register direct controller in init() #1910

Register direct controller in init() #1910

Conversation

yuwenma commented May 30, 2024 • edited Loading

Change description

How to use

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenma May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gemmahou commented May 30, 2024

Choose a reason for hiding this comment

yuwenma May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenma Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Jun 3, 2024

google-oss-prow bot commented Jun 3, 2024

yuwenma commented May 30, 2024 •

edited

Loading

yuwenma May 31, 2024 •

edited

Loading

yuwenma May 30, 2024 •

edited

Loading

yuwenma Jun 1, 2024 •

edited

Loading