Support AlloyDBInstance type, mapper and direct controller #3281

maqiuyujoyce · 2024-12-03T02:42:02Z

Change description

Supported AlloyDBInstance type, mapper and direct controller

Changes in the AlloyDBInstance CRDs are non-breaking. Mostly caused by the format of the field descriptions and how we generate direct CRDs.
- spec.resourceID becomes conditionally mutable (can be updated between empty value and the correct value).
- spec.instanceType and spec.instanceTypeRef are no longer mutually exclusive.
- status.externalRef is supported.
Supported the resource in the controller including special logics to handle the secondary instance in both creation and deletion.
Added "direct" annotation to all the AlloyDBInstance in the alloydbinstance test cases.
Updated the AlloyDBInstance in the test cases to use the direct controller instead.
- The golden files are updated based on the behavior of the direct controller, which are all expected. Note that the TF-based controller somehow does an additional reconciliation right after the successful creation. The direct reconciliation doesn't do it, which caused one less GET event. Another thing is that the EnsureFinalizer() step in TF-based reconciliation possibly increased the generation by one during reconciliation, so the generation value is different. Overall the values and behaviors look more reasonable in the direct controller test result.
Added a no change scenario test.

Tests you have done

Run make ready-pr to ensure this PR is ready for review.
Perform necessary E2E testing for changed resources.

Logs when running against the real GCP using the direct controller: #3361 Note that the dependent resources are all abandoned to save test time so please ignore the diffs unrelated to AlloyDBInstance.

google-oss-prow · 2024-12-03T02:42:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from maqiuyujoyce. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

apis/refs/v1beta1/alloydbref.go

...sions.k8s.io_v1_customresourcedefinition_alloydbinstances.alloydb.cnrm.cloud.google.com.yaml

pkg/controller/direct/alloydb/instance_controller.go

yuwenma · 2024-12-16T13:51:50Z

pkg/controller/direct/alloydb/instance_controller.go

+	return updateOp.UpdateStatus(ctx, status, nil)
+}
+
+func compareInstance(ctx context.Context, actual, desired *krm.AlloyDBInstanceSpec) (updatePaths []string, err error) {


Is there any problems to use the CompareProtoMessage function?

Yes, two problems:

CompareProtoMessage compares all the proto fields, but AlloyDBInstance only supports a subset of the proto fields supported by the API now.

CompareProtoMessage doesn't understand the default values determined by the API code logic. e.g. not-yet supported field, spec.observabilityConfig.trackWaitEvents, has a default value true:

k8s-config-connector/config/samples/resources/alloydbinstance/primary-instance/alloydb_v1beta1_alloydbinstance.yaml

Line 43 in 6fac4a5

trackWaitEvents: true

Because of the problems, CompareProtoMessage always return an error no matter if there is any KRM object update or not.

Discussed offline. CompareProtoMessage compares the parsed desired pb with the actual pb, and the actual pb is the result of Find() (API GET). actual pb contains all the service-defaulted values of those unsupported fields and will result in a diff.

Because there are actually quite some unsupported fields, I think it'll be easier for us to compare the supported fields in Update() explicitly, and switch to CompareProtoMessage once we support most/all pb fields.

Can you list which fields that have GCP defaulted values are not supported? It means we may need to deal with them specially in other places

@yuwenma are you suggesting I should list them in comments here?

They are handled specially in mockalloydb and probably won't be touched unless we support them in the CRD: https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/mockgcp/mockalloydb/instance.go#L50

yuwenma · 2024-12-16T13:53:21Z

pkg/controller/direct/alloydb/instance_controller.go

+	log.V(2).Info("deleting instance", "name", a.id)
+
+	// Returning true directly if it is to delete a secondary instance.
+	// Technically the secondary instance is only abandoned but not deleted.


Is there an option to "abandon" the instance?

Yes, users can still set cnrm.cloud.google.com/deletion-policy: "abandon" to abandon the secondary AlloyDBInstance.

The API behavior the comment here describes basically means even if users tries to use KCC to delete a secondary AlloyDBInstance, the underlying secondary AlloyDB instance (the GCP resource) will not be deleted. This is because the instance by itself is not deletable. The deletion of a secondary AlloyDB instance is a side effect of force deletion of a secondary AlloyDB cluster, which can be done via the deletion of a secondary AlloyDBCluster with spec.deletionPolicy set to FORCE in KCC.

Are you talking about KCC abandon but not GCP having an abandon concept?
What does the a.gcpClient.DeleteInstance(ctx, req) returns if the type is SECONDARY?

Yes, I'm talking about KCC abandon. I think this GCP resource doesn't have an abandon concept.

DeleteInstance() returns the following error if it is a secondary instance:

googleapi: Error 400: Invalid resource state for "projects/[projectNumber]/locations/[location]/clusters/alloydbcluster-2-20241210/instances/alloydbinstance-2-20241210": Cannot delete SECONDARY instance without deleting the SECONDARY cluster. Delete the SECONDARY cluster in force mode to delete the entire cluster and the instances

apis/alloydb/v1alpha1/instance_types.go

...sourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/fullalloydbinstance/create.yaml

apis/alloydb/v1beta1/instance_identity.go

...sions.k8s.io_v1_customresourcedefinition_alloydbinstances.alloydb.cnrm.cloud.google.com.yaml

apis/alloydb/v1beta1/instance_identity.go

yuwenma · 2024-12-19T04:12:00Z

apis/alloydb/v1beta1/instance_identity.go

+func NewInstanceIdentity(ctx context.Context, reader client.Reader, obj *AlloyDBInstance) (*InstanceIdentity, error) {
+
+	// Get Parent
+	clusterRef, err := refsv1beta1.ResolveAlloyDBCluster(ctx, reader, obj, obj.Spec.ClusterRef)


nit: could we add a comment to remove the ResolveAlloyDBCluster once we migrate the Cluster instance to direct? Otherwise, it could be easy to forget

Added a TODO to the ResolveAlloyDBCluster() function. It shouldn't be needed after we migrate AlloyDBCluster.

pkg/controller/direct/alloydb/instance_controller.go

yuwenma · 2024-12-19T04:27:35Z

pkg/controller/direct/alloydb/instance_controller.go

+	return updateOp.UpdateStatus(ctx, status, nil)
+}
+
+func compareInstance(ctx context.Context, actual, desired *krm.AlloyDBInstanceSpec) (updatePaths []string, err error) {


Can you list which fields that have GCP defaulted values are not supported? It means we may need to deal with them specially in other places

yuwenma · 2024-12-19T04:29:44Z

pkg/controller/direct/alloydb/instance_controller.go

+	log.V(2).Info("deleting instance", "name", a.id)
+
+	// Returning true directly if it is to delete a secondary instance.
+	// Technically the secondary instance is only abandoned but not deleted.


Are you talking about KCC abandon but not GCP having an abandon concept?
What does the a.gcpClient.DeleteInstance(ctx, req) returns if the type is SECONDARY?

maqiuyujoyce · 2024-12-21T08:00:15Z

Note that the direct controller has this suboptimal deletion UX: #3362

google-oss-prow bot added the do-not-merge/work-in-progress label Dec 3, 2024

maqiuyujoyce force-pushed the 202411-alloydbinstance-type branch 3 times, most recently from 09d53c4 to 230a0fd Compare December 11, 2024 08:52

maqiuyujoyce removed the do-not-merge/work-in-progress label Dec 12, 2024

maqiuyujoyce changed the title ~~[WIP] Support AlloyDBInstance type, mapper and direct controller~~ Support AlloyDBInstance type, mapper and direct controller Dec 12, 2024

maqiuyujoyce assigned yuwenma Dec 12, 2024

maqiuyujoyce force-pushed the 202411-alloydbinstance-type branch from 666917f to 2b857bd Compare December 13, 2024 23:55

yuwenma reviewed Dec 16, 2024

View reviewed changes

apis/refs/v1beta1/alloydbref.go Show resolved Hide resolved

yuwenma reviewed Dec 16, 2024

View reviewed changes

...sions.k8s.io_v1_customresourcedefinition_alloydbinstances.alloydb.cnrm.cloud.google.com.yaml Outdated Show resolved Hide resolved

yuwenma reviewed Dec 16, 2024

View reviewed changes

maqiuyujoyce force-pushed the 202411-alloydbinstance-type branch 2 times, most recently from 431125c to 8902f25 Compare December 17, 2024 03:47

maqiuyujoyce added 5 commits December 17, 2024 03:47

Support AlloyDBInstance type, mapper and direct controller

6ae6bb7

Clean up

f7646b6

Fix updates in mockalloydb

1fcfd24

Update testdata to use direct controller instead

239edf0

Capitalize acronyms in AlloyDB instance types

0b6b188

maqiuyujoyce force-pushed the 202411-alloydbinstance-type branch from 8902f25 to 0b6b188 Compare December 17, 2024 03:48

yuwenma reviewed Dec 18, 2024

View reviewed changes

Address comments

59e4167

maqiuyujoyce force-pushed the 202411-alloydbinstance-type branch from f13cccf to 59e4167 Compare December 19, 2024 02:56

Support no change test to verify the diffing behavior in Update()

7c7cc39

maqiuyujoyce mentioned this pull request Dec 19, 2024

Remove the CEL rule for resourceID in the template #3427

Closed

yuwenma reviewed Dec 19, 2024

View reviewed changes

Address further comments

e33dba7

maqiuyujoyce force-pushed the 202411-alloydbinstance-type branch from 24d08f8 to e33dba7 Compare December 19, 2024 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support AlloyDBInstance type, mapper and direct controller #3281

Support AlloyDBInstance type, mapper and direct controller #3281

maqiuyujoyce commented Dec 3, 2024 •

edited

Loading

google-oss-prow bot commented Dec 3, 2024

yuwenma Dec 16, 2024

maqiuyujoyce Dec 16, 2024

maqiuyujoyce Dec 19, 2024

yuwenma Dec 19, 2024

maqiuyujoyce Dec 19, 2024

yuwenma Dec 16, 2024

maqiuyujoyce Dec 16, 2024

yuwenma Dec 19, 2024

maqiuyujoyce Dec 19, 2024

yuwenma Dec 19, 2024

maqiuyujoyce Dec 19, 2024

yuwenma Dec 19, 2024

yuwenma Dec 19, 2024

maqiuyujoyce commented Dec 21, 2024

Support AlloyDBInstance type, mapper and direct controller #3281

Are you sure you want to change the base?

Support AlloyDBInstance type, mapper and direct controller #3281

Conversation

maqiuyujoyce commented Dec 3, 2024 • edited Loading

Change description

Tests you have done

google-oss-prow bot commented Dec 3, 2024

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

maqiuyujoyce commented Dec 21, 2024

maqiuyujoyce commented Dec 3, 2024 •

edited

Loading