Add CloudSQL support for "create from clone" #2429

jasonvigil · 2024-08-06T17:09:03Z

Change description

Depends on #2393

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

maqiuyujoyce

Code LGTM except for the potential reference fields. I assume you'll add test cases for this feature, right?

apis/sql/v1beta1/sqlinstance_types.go

maqiuyujoyce · 2024-08-06T19:33:50Z

apis/sql/v1beta1/sqlinstance_types.go

+
+	/* DatabaseNames: (SQL Server only) Clone only the specified databases from the
+	source instance. Clone all databases if empty. */
+	DatabaseNames []string `json:"databaseNames,omitempty"`


This can potentially be references to SQLDatabase resources, right?

Fixed! Thank you

Actually we have gone back on this decision, and are thinking it's overkill to use refs here. Switched back to just a list of names. #2429 (comment)

maqiuyujoyce

Overall lgtm with some nits and open questions, i.e. none of the comment is a blocker.

Have you got the chance to test the clone locally? Do you plan to support test coverage and mockgcp coverage in this PR?

apis/sql/v1beta1/sqlinstance_types.go

pkg/controller/direct/sql/mapping.go

maqiuyujoyce · 2024-08-16T00:25:29Z

pkg/controller/direct/sql/normalize.go

+	sqlDatabaseRefs := obj.Spec.CloneSource.SQLDatabaseRefs
+	sqlDatabases := make([]string, len(sqlDatabaseRefs))
+
+	for i, sqlDatabaseRef := range sqlDatabaseRefs {


Not a blocker, but I believe we also want to check and ensure the list contains all external values or name values, but not a mix of both.

Why? I think it should be ok to mix

@yuwenma proposed it and suggested it will help us avoid complicated edge cases. Yuwen, could you elaborate on it a bit?

I think that it is not really complicated to implement this, and customer may want the functionality. There could be some databases managed by kcc, others managed externally, but customer might want to clone all of them.

SG!

Personally, I like the flexibility of supporting a mix of both. Implementation-wise, it's fine to start with a stricter rule (don't allow the mix) and then loosen it (allow the mix) if we think this provides a better UX, but ideally not the other way around.

I think it's more complicated to add a check to ensure that all references in the list are either external or kcc-managed. That requires more code. However, am not sure about the scenario you mentioned where it could cause "complicated edge cases". Do you remember the details?

Thanks @maqiuyujoyce for raising this.
I put the rationale and some thoughts here. The no-mix is a recommendation ("shall" rather than "must", see RFC2119) . And that it is for GCP unique list.

Just spoke w/ @justinsb about this. We are thinking it is overkill to use refs for these SQLDatabases anyway, since they are associated with the parent SQLInstanceRef. Decided to make this field just a list of strings of database names.

maqiuyujoyce · 2024-08-16T00:33:06Z

pkg/webhook/immutable_fields_validator.go

@@ -380,6 +382,21 @@ func validateImmutableFieldsForLoggingLogMetricResource(oldSpec, spec map[string
 	return allowedResponse
 }

+func validateImmutableFieldsForSQLInstanceResource(oldSpec, spec map[string]interface{}) admission.Response {
+	ImmutableFields := []string{"cloneSource"}


It's out of scope, though we probably need to think about how to handle use cases when a user realizes they made a mistake and want to fix it in-place after the creation/clone failed. Making the field strictly immutable means a user needs to delete the object, edit the manifest, and reapply, and could be a bit troublesome for some GitOps workflow.

I think that we should require delete + recreate, rather than build in some special other mechanism, but maybe we can wait for product feedback on this.

Agreed it is worth debating and we should wait for more customer feedback. Starting with strict immutability is good.
We did receive UX feedback previously about troubles like this in general.

I think it's more idiomatic to require delete + recreate. But maybe this could be partially solved by educating customer about k8s best practices. Or, perhaps there is some k8s-native pattern here the customer had suggested that I am unaware of.

pkg/webhook/immutable_fields_validator.go

jasonvigil · 2024-08-16T17:21:36Z

Overall lgtm with some nits and open questions, i.e. none of the comment is a blocker.

Have you got the chance to test the clone locally? Do you plan to support test coverage and mockgcp coverage in this PR?

Yes, tested locally. Added a basic mockgcp test.

maqiuyujoyce

/lgtm
/approve

/hold
I wondered if @yuwenma wants to take another look.

Besides, can we add a sample about it? https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/README.Samples.md

maqiuyujoyce · 2024-08-16T18:20:29Z

apis/sql/v1beta1/sqlinstance_types.go

 type SQLInstanceSpec struct {
+	/* Create this database as a clone of a source instance. Immutable. */
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="spec.cloneSource is immutable"


A silly question: Does the type of the field matter for the validation? E.g. is it able to understand "equals" for object type here?

Yes, equality works for maps: https://github.com/google/cel-spec/blob/master/doc/langdef.md#equality. I also tested this manually.

I am a little wary of introducing CEL (1) on a high profile resource and (2) accidentally i.e. as part of a wider PR. (2) is in case we need to revert and so that we can focus on CEL.

I think in the short term, it'll be up to the team to decide the preferred behavior of the types/CRDs. I personally am okay with either CEL or webhook checks as long as there is an immutability check. @yuwenma as you proposed the CEL option, do you have any thoughts on it?
In the long term, I feel it's less error-prone if we automate the type creation as much as possible including the CEL and have proper tests to guarantee that we are supporting the right behavior.

I proposed using CEL for immutable check in the CR Validation doc and put the reasons on how the decision is made:

Kubernetes supports CEL since 1.26 which is the oldest GKE cluster version. Using CEL shall not cause GKE cluster version issues, but we shall reevaluate this when the condition changes.

Some KCC resources use webhook to validate immutable fields. We can continue using that, but for external contributors CEL is much easier for self-learning.

We only apply CEL on immutable fields. More complicated CEL validation requires further discussion.

Regarding Justin's concerns, +1 to split the immutable check in a separate PR. Because 1. we do want to use CEL for immutable field check and makes it as the trends. 2. users may hit the CEL-supportability problem when they install KCC in a non-GKE cluster and the k8s is very old. Though the possibility of this case itself is low, SQL is a high profile resource like Justin mentioned. So splitting the PR is a good idea, we can still use CEL and keep us with the clean rollback options.

Ok, moving this CEL validation to a separate PR.

maqiuyujoyce · 2024-08-16T18:29:50Z

...est/resourcefixture/testdata/basic/sql/v1beta1/sqlinstance/sqlinstanceclonebasic/create.yaml

+apiVersion: sql.cnrm.cloud.google.com/v1beta1
+kind: SQLInstance
+metadata:
+  name: postgres-clone-${uniqueId}


It's out of scope, so bringing it up just for discussion.

I personally like consistent naming for easier validation, maintenance, education (contributor may ask about how to name the resource), and potential automation. That's why we had this guide: https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/docs/archive/README.NewResourceFromTerraform.md#naming-conventions-special-variables-and-constants-in-testdata

Now we are moving to direct controllers. Do you find any preferred way of naming the test resources? Or do you find any other purposes (other than consistency, maintenance easiness, etc) more important when coming up with the names?

I feel like it doesn't really matter that much, as long as the test resource names are unique and meaningful / understandable at first glance. Much like variable names.

maqiuyujoyce · 2024-08-16T18:30:19Z

...est/resourcefixture/testdata/basic/sql/v1beta1/sqlinstance/sqlinstanceclonebasic/create.yaml

+    backupConfiguration:
+      enabled: true
+      pointInTimeRecoveryEnabled: true
+    # Location preference is not actually a required field. However, setting it for tests


justinsb · 2024-08-19T18:19:31Z

apis/sql/v1beta1/sqlinstance_types.go

 type SQLInstanceSpec struct {
+	/* Create this database as a clone of a source instance. Immutable. */
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="spec.cloneSource is immutable"


I am a little wary of introducing CEL (1) on a high profile resource and (2) accidentally i.e. as part of a wider PR. (2) is in case we need to revert and so that we can focus on CEL.

justinsb · 2024-08-19T18:22:23Z

pkg/controller/direct/sql/merge.go

@@ -626,7 +626,7 @@ func MergeDesiredSQLInstanceWithActual(desired *krm.SQLInstance, refs *SQLInstan
 	}

 	if desired.Spec.Settings.DiskAutoresize != nil {
-		if desired.Spec.Settings.DiskAutoresize != actual.Settings.StorageAutoResize {
+		if direct.ValueOf(desired.Spec.Settings.DiskAutoresize) != direct.ValueOf(actual.Settings.StorageAutoResize) {
 			// Change disk autoresize
 			updateRequired = true


We might also be able to look at the updateTime...

It felt like this was a more reliable way to do it, but am planning to switch to use the mapper / generator code soon anyway.

Though maybe the mapper generator code wouldn't really help here. Can you elaborate a bit on what you mean by using updateTime? Not sure I understand completely.

justinsb · 2024-08-19T18:24:37Z

pkg/controller/direct/sql/normalize.go

+			}
+			sqlDatabases[i] = sqlDatabaseName
+		} else {
+			return nil, fmt.Errorf("must specify either spec.settings.cloneSource.sqlDatabaseRefs[].external or spec.settings.cloneSource.sqlDatabaseRefs[].name")


FYI, k8s has fieldpath to help with this problem (writing annoyingly long field paths!)

Can you share a link to an example of this?

pkg/controller/direct/sql/sqlinstance_controller.go

yuwenma · 2024-08-20T07:52:51Z

/lgtm

Looks great! You may want to resolve the comments before merging.

google-oss-prow · 2024-08-23T17:54:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, maqiuyujoyce

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,maqiuyujoyce]

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

jasonvigil · 2024-08-23T20:09:19Z

/unhold (everyone has reviewed at this point)

jasonvigil · 2024-08-23T20:17:35Z

/hold cancel

jasonvigil requested a review from maqiuyujoyce August 6, 2024 17:09

maqiuyujoyce reviewed Aug 6, 2024

View reviewed changes

justinsb added the service:sql label Aug 12, 2024

jasonvigil force-pushed the cloudsql-clone branch from 1f5782c to 1a119bf Compare August 15, 2024 20:29

jasonvigil requested a review from maqiuyujoyce August 15, 2024 20:30

jasonvigil force-pushed the cloudsql-clone branch from 1a119bf to b1d9e91 Compare August 15, 2024 21:15

maqiuyujoyce reviewed Aug 16, 2024

View reviewed changes

jasonvigil force-pushed the cloudsql-clone branch from b1d9e91 to 69213b8 Compare August 16, 2024 17:18

maqiuyujoyce reviewed Aug 16, 2024

View reviewed changes

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

google-oss-prow bot assigned maqiuyujoyce Aug 16, 2024

google-oss-prow bot added lgtm approved labels Aug 16, 2024

jasonvigil force-pushed the cloudsql-clone branch from 69213b8 to 9847142 Compare August 16, 2024 18:41

google-oss-prow bot removed the lgtm label Aug 16, 2024

jasonvigil requested review from maqiuyujoyce, yuwenma and justinsb and removed request for yuwenma August 16, 2024 18:45

justinsb reviewed Aug 19, 2024

View reviewed changes

google-oss-prow bot assigned yuwenma Aug 20, 2024

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

jasonvigil force-pushed the cloudsql-clone branch from 9847142 to a241835 Compare August 22, 2024 16:52

google-oss-prow bot removed the lgtm label Aug 22, 2024

This was referenced Aug 22, 2024

Add CEL-based validation rules for SQLInstance #2558

Closed

Add predicate to partially enable SQLInstance direct reconciler #2559

Merged

jasonvigil requested a review from justinsb August 22, 2024 18:06

feat: Add cloudsql support for "create from clone"

395fef7

jasonvigil force-pushed the cloudsql-clone branch from a241835 to 395fef7 Compare August 23, 2024 17:37

justinsb approved these changes Aug 23, 2024

View reviewed changes

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

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

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

google-oss-prow bot merged commit 1d4eebe into GoogleCloudPlatform:master Aug 23, 2024
12 of 13 checks passed

jasonvigil deleted the cloudsql-clone branch August 23, 2024 20:18

jasonvigil mentioned this pull request Aug 27, 2024

Ability to duplicate an existing SQLInstance #489

Closed

yuwenma added this to the 1.122 milestone Oct 25, 2024

Add CloudSQL support for "create from clone" #2429

Add CloudSQL support for "create from clone" #2429

Conversation

jasonvigil commented Aug 6, 2024

Change description

maqiuyujoyce 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

maqiuyujoyce 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

maqiuyujoyce Aug 16, 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

Choose a reason for hiding this comment

jasonvigil Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

jasonvigil commented Aug 16, 2024

maqiuyujoyce 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

yuwenma Aug 20, 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

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

yuwenma commented Aug 20, 2024

google-oss-prow bot commented Aug 23, 2024

jasonvigil commented Aug 23, 2024

jasonvigil commented Aug 23, 2024

maqiuyujoyce Aug 16, 2024 •

edited

Loading

jasonvigil Aug 16, 2024 •

edited

Loading

yuwenma Aug 20, 2024 •

edited

Loading