feat: allow users to opt-in to direct reconciliation

[justinsb]

Add support for a new label, 'alpha.cnrm.cloud.google.com/reconciler'

If set to direct, the direct reconciler will be used.

[justinsb]

WIP because I think this presumes that all controllers are TF & Direct.

cc @yuwenma

[jasonvigil]

@justinsb seems there is some code missing? It doesn't compile.

[jasonvigil]

Should we update

k8s-config-connector/pkg/test/resourcefixture/contexts/register.go

Lines 94 to 105 in 1ed7c14

	// Determine the type of controller for the resource: TF, DCL, or Direct.
	crd, err := crdloader.GetCRDForGVK(rc.ResourceGVK)
	if err != nil {
	return ResourceContext{}, err
	}
	if crd.GetLabels()[crdgeneration.TF2CRDLabel] == "true" {
	rc.IsTFResource = true
	} else if crd.GetLabels()[crdgeneration.Dcl2CRDLabel] == "true" {
	rc.IsDCLResource = true
	} else {
	rc.IsDirectResource = true
	}

also? I actually think it's ok to leave as-is (even with this new behavior), it would just mean our periodic tests would be based on the CRD labels only (not the CR opt-in label).

[jasonvigil]

We need to update the other direct resources to include the optIn ReconcileGate also, right?

[justinsb]

It is the default now :-)

[justinsb]

(Check out RegisterModel)

[jasonvigil]

Oh I see, cool!

[justinsb]

And good question re the update; I think though that those tests probably should be based on the "primary" controller, rather than the "opt-in" / "override" controller... at least that's what I think looking at how IsTFResource etc are used. Open to other views though!

[jasonvigil]

And good question re the update; I think though that those tests probably should be based on the "primary" controller, rather than the "opt-in" / "override" controller... at least that's what I think looking at how IsTFResource etc are used. Open to other views though!

Right now, the behavior is based on the labels in the CRD. If TF label -> use TF reconciler. If DCL label -> use DCL reconciler. If neither TF or DCL labels -> use Direct reconciler. Does that seem ok?

[justinsb]

(I switched to an annotation, feels more consistent, and added a test)

[yuwenma]

I like the idea of allowing users to opt-in the alpha direct reconciler ( a big relief on migrating the stable resources). My main concern is that we basically surface the TF reconciler in the KCC object level, but to me these are the implementation details to users and they should not care. Also, when we have to recommend users to switch back to the TF-reconciler due to a regression (rather than fully control the reconciler and fixing it in the next release ourselves), we are basically sending the signals that Direct is less stable than TF. Or we can only turn on this feature in very specific resources like sql.

BTW, looks like this PR does not have test for sqlinstance (the test suite is for dataflowflextemplatejob), can we add one?

[justinsb]

So I imagine this annotation will be used to let customers opt-in to early access to the direct controller. That is going to be less tested than a beta controller in widespread use, I think that's OK and understandable. I don't think we would recommend using this to force the terraform controller once we've promoted the direct controller to be the default (I guess we could, though it isn't supported today)

I don't understand why SQLInstance, is that because it has the complex predicate? The problem is that it's already in KCC_USE_DIRECT_RECONCILERS, so we're testing the direct controller right now (and only the direct controller).

[yuwenma]

So I imagine this annotation will be used to let customers opt-in to early access to the direct controller. That is going to be less tested than a beta controller in widespread use, I think that's OK and understandable. I don't think we would recommend using this to force the terraform controller once we've promoted the direct controller to be the default (I guess we could, though it isn't supported today)

That sounds good. basically you mean the alpha.cnrm.cloud.google.com/reconciler only accepts direct and not extend to other value like tf / dcl.

I don't understand why SQLInstance, is that because it has the complex predicate? The problem is that it's already in KCC_USE_DIRECT_RECONCILERS, so we're testing the direct controller right now (and only the direct controller).

Yes and also because this PR adds the opt-in in sqlinstance and no test covers it (I think?).

[yuwenma]

/lgtm
/approve

Still have some open questions to discuss. But no blockers.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuwenma

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 [yuwenma]

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