Feat: Add spanner database mockgcp #1829

jasonvigil · 2024-05-17T00:00:26Z

Change description

Add protos for Spanner Database
Add mockgcp implementation for Spanner Database

Tasks

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

mockgcp/fixup-third-party.sh

mockgcp/mockspanner/database.go

mockgcp/mockspanner/names.go

justinsb · 2024-05-20T17:26:25Z

Code LGTM. I'll look at that test failure :-)

justinsb · 2024-05-20T17:33:28Z

I think the test failure is that we need to implement DropDatabase in mockgcp...

    samples.go:297: error while polling for resource cleanup on SpannerDatabase with name 'spannerdatabase-test': context deadline exceeded; last seen status: map[conditions:[map[lastTransitionTime:2024-05-20T13:28:12-04:00 message:Delete call failed: error deleting resource: [{0 Error when reading or editing Database: googleapi: got HTTP response code 501 with body: {"code":12, "message":"method DropDatabase not implemented", "details":[]}  []}] reason:DeleteFailed status:False type:Ready]] observedGeneration:3 state:STATE_UNSPECIFIED]

I got this by running hack/compare-mock fixture/externalwithsubresource locally

(I determined it was externalwithsubresource by downloading the raw logs and searching for FAIL:)

jasonvigil · 2024-05-20T19:50:40Z

Thanks @justinsb, fixed those issues. Ready for another review.

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

justinsb · 2024-05-22T12:25:53Z

I think there are some merge conflicts now that we merged SpannerInstance - mind rebasing?

jasonvigil · 2024-05-24T20:39:15Z

@justinsb / @yuwenma ok, rebased. PTAL when you have a chance :)

yuwenma · 2024-06-05T20:20:33Z

...sions.k8s.io_v1_customresourcedefinition_spannerdatabases.spanner.cnrm.cloud.google.com.yaml

@@ -8,7 +8,6 @@ metadata:
    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"


This infers the spanner database is using the TF controller. I think that is good, because we are not ready to turn on the direct controller on this resource yet.
Then to test the mockGCP with the direct controller, you would need to use the KCC_USE_DIRECT_RECONCILERS flag. Right now you can put the test coverage for "direct controller" here https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/scripts/github-actions/tests-e2e-direct.sh with KCC_USE_DIRECT_RECONCILERS flag.

This was just my hack to enable testing the direct controller manually in a real gcp environment with make deploy-controller && kubectl delete pods --namespace cnrm-system --all. Not planning to merge it, will remove later.

yuwenma

I think this looks good. You can consider spliting the mockGCP test (for TF controller) so it can goes in first.

yuwenma · 2024-06-05T20:21:50Z

hack/compare-mock

@@ -5,6 +5,8 @@ set -x

 export KUBEBUILDER_ASSETS=$(go run sigs.k8s.io/controller-runtime/tools/setup-envtest@latest use -p path)

+export KCC_USE_DIRECT_RECONCILERS=SpannerDatabase


nit: cleanup. (I know this PR is WIP)

I am a bit confused on this actually. What is our final / production method for switching a resource to use the "direct" controller instead of the terraform-based controller?

To switch a resource to the direct controller, you only need to remove the CRD's tf2crd label (for DCL based it is dcl2crd). If a resource is not yet ready for direct controller but you want to run mock test against the direct controller, you can use the KCC_USE_DIRECT_RECONCILERS flag.

In other words, direct controller is the default.

yuwenma · 2024-06-05T20:22:20Z

mockgcp/mock_http_roundtrip.go

@@ -59,7 +59,7 @@ import (
 	"github.com/GoogleCloudPlatform/k8s-config-connector/mockgcp/mocksecretmanager"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/mockgcp/mockservicenetworking"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/mockgcp/mockserviceusage"
-	mockspannerinstance "github.com/GoogleCloudPlatform/k8s-config-connector/mockgcp/mockspanner/admin"
+	mockspanner "github.com/GoogleCloudPlatform/k8s-config-connector/mockgcp/mockspanner/admin"


Actually I had a question on this -- Why did you create the admin/ subdirectory for the instance mock? For other resources it looks like the code is placed directly in mockgcp/mock<resourcename>/

I'm trying to align that with its cloud client API https://pkg.go.dev/cloud.google.com/go/spanner#section-directories where the admin/ has instance and database and executor has executor. I assume that's how spanner categorize its resources (and maybe will add more resources in the executor dir). But I don't have strong preference on this.

yuwenma · 2024-06-05T20:22:48Z

mockgcp/mockspanner/admin/database.go

+		return nil, err
+	}
+
+	return s.operations.NewLRO(ctx)


I think we would want to use StartLRO if possible which better reflects the actual service behavior. (Feel free to make the improvement in follow up PRs.)

Ok, I will look into this. Thanks 👍

yuwenma · 2024-06-05T20:25:13Z

pkg/controller/direct/directbase/utils.go

This is just a copy of your code from #1761. And, actually, I would prefer to reorganize and put the "util" code elsewhere, there is some guidance I've seen to avoid "util" files / packages, since they tend to become dumping grounds. WDYT?

yes, I'm not a fan of utils or helpers. I think those functions are generic and could be reused by other mock functions. Feel free to refactor the code!

yuwenma

/lgtm

Looks great! Some minor questions

yuwenma · 2024-06-18T00:35:09Z

mockgcp/mockspanner/admin/database.go

+		Statements: req.Statements,
+	}
+	return s.operations.StartLRO(ctx, fqn, metadata, func() (proto.Message, error) {
+		// Many fields are not populated in the LRO result


nit: set the createTime, updateTime

There is no field for updateTime for this resource, but just double-checked that createTime is being set.

yeah, it looks like the Database uses Reconciling (bool) to tell if the update succeeds or not.

yuwenma · 2024-06-18T00:35:50Z

mockgcp/mockspanner/admin/database.go

+// Given a list of DDL extra statement strings, find one with the format
+// "ALTER DATABASE `%s` SET OPTIONS (version_retention_period = '%s')"
+// and return the version_retention_period, or "1h" if no match.
+func versionRetentionPeriodFromDDL(ddl []string) string {


yuwenma · 2024-06-18T00:39:30Z

...testdata/basic/spanner/v1beta1/spannerdatabase/_generated_object_spannerdatabase.golden.yaml

+  generation: 2
+  labels:
+    cnrm-test: "true"
+  name: spannerdatabase-test


nit: There's some naming convention requirement. I think this should be spannerdatabase-${uniqueId}

Fixed, thanks :) I think I had accidentally changed this for manual testing.

yuwenma · 2024-06-18T00:41:18Z

pkg/test/resourcefixture/testdata/basic/spanner/v1beta1/spannerdatabase/_http.log

+
+---
+
+PATCH https://spanner.googleapis.com/v1/projects/${projectId}/instances/spannerinstance-${uniqueId}/databases/spannerdatabase-test/ddl?alt=json


Maybe I miss something.
I don't see a update.yaml file under https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/pkg/test/resourcefixture/testdata/basic/spanner/v1beta1/spannerdatabase/
How this patch is triggered?

This is an implementation detail of the terraform controller. I think it works around the issue that it's impossible to specify extra DDL statements for Postgres-flavor databases in the create call by just always taking two steps (even if it's a GOOGLESQL-flavor database).

That makes sense. I see this is a really good example on relying the mockgcp to test, validate and verify controller operations.

justinsb · 2024-06-18T17:26:34Z

LGTM!

(Failure on tests-e2e-fixtures looks unrelated and looks like envtest crashed, hopefully if we can get to mockkubeapiserver that should address these sorts of flakes!)

yuwenma · 2024-06-18T21:20:34Z

/lgtm
/approve

google-oss-prow · 2024-06-18T21:20:45Z

[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

yuwenma · 2024-06-20T17:21:22Z

/lgtm

justinsb reviewed May 20, 2024

View reviewed changes

mockgcp/fixup-third-party.sh Outdated Show resolved Hide resolved

mockgcp/mockspanner/database.go Outdated Show resolved Hide resolved

mockgcp/mockspanner/names.go Outdated Show resolved Hide resolved

jasonvigil force-pushed the spanner-db branch from 0f63984 to 84b4d4b Compare May 20, 2024 18:49

jasonvigil requested a review from justinsb May 20, 2024 19:50

yuwenma reviewed May 20, 2024

View reviewed changes

scripts/github-actions/tests-e2e-direct.sh Outdated Show resolved Hide resolved

jasonvigil force-pushed the spanner-db branch from 84b4d4b to 8baf0e5 Compare May 20, 2024 20:37

jasonvigil force-pushed the spanner-db branch from 8baf0e5 to 7f0903f Compare May 24, 2024 20:01

jasonvigil requested a review from yuwenma May 24, 2024 20:36

jasonvigil force-pushed the spanner-db branch from 7f0903f to 2b62271 Compare May 31, 2024 22:23

jasonvigil changed the title ~~Add mockgcp implementation for Spanner Database~~ Add direct controller for Spanner Database May 31, 2024

jasonvigil changed the title ~~Add direct controller for Spanner Database~~ [WIP] Add direct controller for Spanner Database May 31, 2024

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

jasonvigil force-pushed the spanner-db branch 2 times, most recently from 55823e4 to 45efbf0 Compare June 5, 2024 17:14

yuwenma reviewed Jun 5, 2024

View reviewed changes

jasonvigil force-pushed the spanner-db branch from 45efbf0 to e5f9886 Compare June 6, 2024 18:10

jasonvigil force-pushed the spanner-db branch from e5f9886 to 5971007 Compare June 13, 2024 22:03

jasonvigil changed the title ~~[WIP] Add direct controller for Spanner Database~~ Feat: Add spanner database mockgcp Jun 13, 2024

google-oss-prow bot removed the do-not-merge/work-in-progress label Jun 13, 2024

jasonvigil force-pushed the spanner-db branch 2 times, most recently from a991444 to 31cb74c Compare June 17, 2024 23:00

yuwenma reviewed Jun 18, 2024

View reviewed changes

google-oss-prow bot assigned yuwenma Jun 18, 2024

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

jasonvigil force-pushed the spanner-db branch from 31cb74c to 91a1e7e Compare June 18, 2024 16:32

google-oss-prow bot removed the lgtm label Jun 18, 2024

jasonvigil requested a review from yuwenma June 18, 2024 16:37

jasonvigil force-pushed the spanner-db branch from 91a1e7e to 66b30e5 Compare June 18, 2024 17:51

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

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

jasonvigil force-pushed the spanner-db branch from 66b30e5 to 10f0e1b Compare June 18, 2024 23:42

google-oss-prow bot removed the lgtm label Jun 18, 2024

Feat: Add spanner database mockgcp

72aa7b0

jasonvigil force-pushed the spanner-db branch from 10f0e1b to 72aa7b0 Compare June 20, 2024 15:03

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

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

jasonvigil deleted the spanner-db branch June 20, 2024 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add spanner database mockgcp #1829

Feat: Add spanner database mockgcp #1829

jasonvigil commented May 17, 2024 •

edited

Loading

justinsb commented May 20, 2024

justinsb commented May 20, 2024

jasonvigil commented May 20, 2024

justinsb commented May 22, 2024

jasonvigil commented May 24, 2024

yuwenma Jun 5, 2024

jasonvigil Jun 5, 2024

yuwenma left a comment

yuwenma Jun 5, 2024

jasonvigil Jun 5, 2024

yuwenma Jun 7, 2024 •

edited

Loading

yuwenma Jun 5, 2024

jasonvigil Jun 5, 2024

yuwenma Jun 7, 2024

yuwenma Jun 5, 2024

jasonvigil Jun 5, 2024

yuwenma Jun 5, 2024

jasonvigil Jun 5, 2024

yuwenma Jun 7, 2024

yuwenma left a comment

yuwenma Jun 18, 2024

jasonvigil Jun 18, 2024

yuwenma Jun 18, 2024

yuwenma Jun 18, 2024

yuwenma Jun 18, 2024

jasonvigil Jun 18, 2024

yuwenma Jun 18, 2024

jasonvigil Jun 18, 2024

yuwenma Jun 18, 2024

justinsb commented Jun 18, 2024

yuwenma commented Jun 18, 2024

google-oss-prow bot commented Jun 18, 2024

yuwenma commented Jun 20, 2024

		@@ -5,6 +5,8 @@ set -x

		export KUBEBUILDER_ASSETS=$(go run sigs.k8s.io/controller-runtime/tools/setup-envtest@latest use -p path)

		export KCC_USE_DIRECT_RECONCILERS=SpannerDatabase


		---

		PATCH https://spanner.googleapis.com/v1/projects/${projectId}/instances/spannerinstance-${uniqueId}/databases/spannerdatabase-test/ddl?alt=json

Feat: Add spanner database mockgcp #1829

Feat: Add spanner database mockgcp #1829

Conversation

jasonvigil commented May 17, 2024 • edited Loading

Change description

Tasks

justinsb commented May 20, 2024

justinsb commented May 20, 2024

jasonvigil commented May 20, 2024

justinsb commented May 22, 2024

jasonvigil commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenma 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 Jun 7, 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

yuwenma 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

justinsb commented Jun 18, 2024

yuwenma commented Jun 18, 2024

google-oss-prow bot commented Jun 18, 2024

yuwenma commented Jun 20, 2024

jasonvigil commented May 17, 2024 •

edited

Loading

yuwenma Jun 7, 2024 •

edited

Loading