Add predicate to partially enable SQLInstance direct reconciler #2559

jasonvigil · 2024-08-22T16:58:41Z

Change description

If spec.cloneSource is specified, use direct reconciler. Otherwise, use
the terraform-based reconciler.

However, if KCC_USE_DIRECT_RECONCILERS=SQLInstance is set, always use
the direct reconciler.

This commit adds a reusable way for other direct reconcilers to
accomplish a similar goal of partially enabling the direct reconciler,
based on the resource to-be-reconciled. The ReconcileGate can be
registered for any direct reconciler, with custom logic based on the
resource.

Depends on #2429

justinsb

A few comments, but I don't think anything blocking

apis/sql/v1beta1/sqlinstance_types.go

justinsb · 2024-08-23T14:07:15Z

apis/sql/v1beta1/sqlinstance_types.go

@@ -569,7 +595,7 @@ type SQLInstanceStatus struct {
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 // +kubebuilder:resource:categories=gcp,shortName=gcpsqlinstance;gcpsqlinstances
 // +kubebuilder:subresource:status
-// +kubebuilder:metadata:labels="cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=stable";"cnrm.cloud.google.com/system=true";"cnrm.cloud.google.com/tf2crd=true"
+// +kubebuilder:metadata:labels="cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=stable";"cnrm.cloud.google.com/system=true"


We'll need to be careful that this resource isn't skipped by the TF reconciler - IIUC we want both?

You did handle this, so I think you can disregard!

Yeah, it should run both, unless there is an error with getting or converting the resource in the predicate. In that case, neither controller will run (currently).

justinsb · 2024-08-23T14:10:40Z

pkg/controller/direct/directbase/directbase_controller.go

@@ -97,16 +98,21 @@ func NewReconciler(mgr manager.Manager, immediateReconcileRequests chan event.Ge
 }

 // add adds a new Controller to mgr with r as the reconcile.Reconciler.
-func add(mgr manager.Manager, r *DirectReconciler) error {
+func add(mgr manager.Manager, r *DirectReconciler, rolloutPredicate predicate.Predicate) error {


Nit: rolloutPredicate is good in that it says what it is used for, but "rollout" could refer to the resource rolling out or something like that. So it might be better to call this additionalPredicate or additionalPredicates or something like that... I'm not sure though!

Hmm, I had some trouble deciding on a name for this one. Just changed it to ReconcileGate / ReconcilePredicate, wdyt?

pkg/controller/direct/registry/registry.go

justinsb · 2024-08-23T14:13:25Z

pkg/controller/direct/sql/mapping.go

-	return &v
+	cloneReq.CloneContext.DestinationInstanceName = resourceID
+
+	if in.Spec.Settings.IpConfiguration != nil {


Will be interesting to see if we can get this under fuzz-testing, so we can detect new fields...

Where is the fuzz testing code? I am still just trying to achieve test coverage of the basic functionality lol.

justinsb · 2024-08-23T14:34:13Z

pkg/controller/direct/sql/sqlinstance_controller.go

+		if err := gax.Sleep(ctx, pollingBackoff.Pause()); err != nil {
+			return fmt.Errorf("waiting for SQLInstance %s clone failed: %w", a.desired.Name, err)
+		}
+		op, err = a.sqlOperationsClient.Get(a.projectID, op.Name).Do()


If you have ideas or a PoC for surfacing information during this reconciliation ... that is so hot right now!

Hmm.. probably out of scope for this PR, but am def planning to improve the code for this soon.

justinsb · 2024-08-23T14:37:25Z

pkg/controller/direct/sql/sqlinstance_controller.go

@@ -185,7 +257,7 @@ func (a *sqlInstanceAdapter) Create(ctx context.Context, u *unstructured.Unstruc
 				// Ref: https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance


One problem with doing more than one thing in a reconciliation is what happens if the first one succeeds but the second fails. So we might want to extract this into a function, so that we can also call it when handling the adoption case. But of course, then we need to be careful that we don't delete the root user if the user created it.

(Arguably, this is why it's not a good idea to do things like this in the IaC layer, particularly if the user has not asked us to)

Agreed. Though, maybe we don't need to delete the user at all? This was only done to achieve implementation parity with Terraform. Perhaps it would be ok to just.. not do this? Wdyt?

I don't think we want to change the behaviour here. But yeah, I'm sure it will come up again...

justinsb · 2024-08-23T14:39:59Z

pkg/controller/predicate/rolloutgate.go

+	}
+}
+
+func (p *RolloutPredicate) Create(e event.CreateEvent) bool {


I personally like to add a comment like // Create implements the Predicate interface just to make it clearer what we're doing. I've seen it done elsewhere, and I assume at some stage there will be better support in IDEs for going to the interface method definition (so it's not worth doing more)...

For me, I usually just add a "self-documenting" variable assignment (like here: https://github.com/GoogleCloudPlatform/k8s-config-connector/pull/2559/files#diff-b3ba7e1f2676394700779aab805a0ac09590b4d96b002e9f84eb581915a6eb17R43) to show that it fits the interface: var _ k8spredicate.Predicate = &RolloutPredicate{}. Not sure if comments are really all that necessary

OK, let's go with that!

The challenge is when there are more methods than just the interface functions, but maybe the answer is we should add comments on those functions.

justinsb · 2024-08-23T14:40:25Z

pkg/controller/predicate/rolloutgate.go

+	if err != nil {
+		return false
+	}
+	return p.rg.ShouldReconcile(obj)


Nit: Feels like you could pass the event to ShouldReconcile?

Ah I see, actually this makes sense because I think we're assuming that at some stage we will watch more intelligently and not the extra GET. We could eliminate some of the duplication by having a helper method in RolloutPredicate, but ... not a blocker.

Yeah, it felt like this was the least hassle way to do it, because the UpdateEvent passes in ObjectNew and ObjectOld (unlike all of the other events, which only pass in Object). So, there wasn't really any way to make a generic handler for every single event.

justinsb · 2024-08-23T14:41:36Z

pkg/controller/predicate/rolloutgate.go

+}
+
+// InverseRolloutPredicate generates a controller-runtime predicate based on the inverse of a RolloutGate.
+type InverseRolloutPredicate struct {


Nit: might be easier to just have an InversePredicate that always returns the inverse of a predicate. You don't have to export inversePredicate, rather NewInverseRolloutPredicate could just return k8spredicate.Predicate.

This was sorta intentional, but I am not sure if it was wise. The idea was that if there is an error, then inversePredicate would return false also (same as non-inverse predicate). In that case, neither direct or terraform reconcilers would run.

justinsb · 2024-08-23T14:49:17Z

Overall LGTM - comments but mostly not blocking. This is going to be great and unblock moving important resources to direct actuation 🎉

One blocker (I think): sqlDatabaseRefs or databaseNames. The databases have to be scoped to the source sqlInstance, I believe, so I think databaseNames might be fine (?)

jasonvigil · 2024-08-23T17:48:45Z

/hold wait for #2429 to merge first

jasonvigil · 2024-08-23T20:52:58Z

/hold cancel

If spec.cloneSource is specified, use direct reconciler. Otherwise, use the terraform-based reconciler. However, if `KCC_USE_DIRECT_RECONCILERS=SQLInstance` is set, always use the direct reconciler. This commit adds a reusable way for other direct reconcilers to accomplish a similar goal of partially enabling the direct reconciler, based on the resource to-be-reconciled. The ReconcileGate can be registered for any direct reconciler, with custom logic based on the resource.

google-oss-prow · 2024-08-27T16:56:14Z

[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

jasonvigil requested review from justinsb and yuwenma August 22, 2024 16:58

justinsb added the service:sql label Aug 22, 2024

justinsb reviewed Aug 23, 2024

View reviewed changes

google-oss-prow bot added the do-not-merge/hold label Aug 23, 2024

jasonvigil force-pushed the cloudsql-rolloutgate branch from 61aeb0e to 741e4db Compare August 23, 2024 20:38

google-oss-prow bot removed the do-not-merge/hold label Aug 23, 2024

jasonvigil requested a review from justinsb August 23, 2024 20:53

jasonvigil changed the title ~~Partially enable SQLInstance direct reconciler with a RolloutGate~~ Add predicate to partially enable SQLInstance direct reconciler Aug 23, 2024

jasonvigil force-pushed the cloudsql-rolloutgate branch from 741e4db to f5f22cf Compare August 23, 2024 21:05

justinsb approved these changes Aug 27, 2024

View reviewed changes

google-oss-prow bot assigned justinsb Aug 27, 2024

google-oss-prow bot added the lgtm label Aug 27, 2024

google-oss-prow bot added the approved label Aug 27, 2024

google-oss-prow bot merged commit ec9aceb into GoogleCloudPlatform:master Aug 27, 2024
13 checks passed

jasonvigil deleted the cloudsql-rolloutgate branch August 27, 2024 20:54

yuwenma added this to the 1.122 milestone Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add predicate to partially enable SQLInstance direct reconciler #2559

Add predicate to partially enable SQLInstance direct reconciler #2559

jasonvigil commented Aug 22, 2024 •

edited

Loading

justinsb left a comment

justinsb Aug 23, 2024

justinsb Aug 23, 2024

jasonvigil Aug 23, 2024

justinsb Aug 23, 2024

jasonvigil Aug 23, 2024

justinsb Aug 23, 2024

jasonvigil Aug 23, 2024

justinsb Aug 23, 2024

jasonvigil Aug 23, 2024

justinsb Aug 23, 2024

jasonvigil Aug 23, 2024

justinsb Aug 27, 2024

justinsb Aug 23, 2024

jasonvigil Aug 23, 2024

justinsb Aug 27, 2024

justinsb Aug 23, 2024

justinsb Aug 23, 2024

jasonvigil Aug 23, 2024

justinsb Aug 23, 2024

jasonvigil Aug 23, 2024

justinsb commented Aug 23, 2024

jasonvigil commented Aug 23, 2024

jasonvigil commented Aug 23, 2024

google-oss-prow bot commented Aug 27, 2024

		@@ -185,7 +257,7 @@ func (a sqlInstanceAdapter) Create(ctx context.Context, u unstructured.Unstruc
		// Ref: https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance

Add predicate to partially enable SQLInstance direct reconciler #2559

Add predicate to partially enable SQLInstance direct reconciler #2559

Conversation

jasonvigil commented Aug 22, 2024 • edited Loading

Change description

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

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

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

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

justinsb commented Aug 23, 2024

jasonvigil commented Aug 23, 2024

jasonvigil commented Aug 23, 2024

google-oss-prow bot commented Aug 27, 2024

jasonvigil commented Aug 22, 2024 •

edited

Loading