Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spanner instance direct controller #3137

Merged

Conversation

anhdle-sso
Copy link
Collaborator

@anhdle-sso anhdle-sso commented Nov 8, 2024

Change description

Fixes #

Tests you have done

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

@anhdle-sso anhdle-sso requested a review from yuwenma November 8, 2024 00:35
@anhdle-sso anhdle-sso force-pushed the spanner_instance_mapper branch 5 times, most recently from bc06e0f to cb1912f Compare November 19, 2024 00:00
@anhdle-sso anhdle-sso removed the request for review from yuwenma November 19, 2024 21:09
@anhdle-sso anhdle-sso changed the title Spanner instance mapper [WIP] Spanner instance mapper Nov 19, 2024
@anhdle-sso anhdle-sso force-pushed the spanner_instance_mapper branch from cb1912f to 3e25f08 Compare December 9, 2024 23:49
@anhdle-sso anhdle-sso force-pushed the spanner_instance_mapper branch from 3e25f08 to 181088a Compare December 17, 2024 01:18
@anhdle-sso anhdle-sso changed the title [WIP] Spanner instance mapper Spanner instance direct controller Dec 17, 2024
@anhdle-sso anhdle-sso requested a review from yuwenma December 17, 2024 01:19
apis/spanner/v1beta1/instance_reference.go Outdated Show resolved Hide resolved
hack/record-gcp Outdated
@@ -27,7 +27,7 @@ echo "TEST_BILLING_ACCOUNT_ID=${TEST_BILLING_ACCOUNT_ID}"
export TEST_BILLING_ACCOUNT_ID

if [[ -z "${KCC_USE_DIRECT_RECONCILERS:-}" ]]; then
KCC_USE_DIRECT_RECONCILERS="ComputeForwardingRule"
KCC_USE_DIRECT_RECONCILERS="ComputeForwardingRule,SpannerInstance"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest not changing this env var. We don't run real gcp against presubmit, so this is more like a local setup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, I added this to run with real gcp, I can take it out here

"nodeCount": 3,
"processingUnits": 2000
"name": "projects/${projectId}/instances/spannerinstance-sample-${uniqueId}",
"nodeCount": 3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

apis/spanner/v1beta1/instance_types.go Outdated Show resolved Hide resolved
Comment on lines +83 to +93
resourceID := direct.ValueOf(obj.Spec.ResourceID)
if resourceID == "" {
resourceID = obj.GetName()
}
if resourceID == "" {
return nil, fmt.Errorf("cannot resolve resource ID")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be from the NewSpannerInstanceRef

return mapCtx.Err()
}

updateMask := &fieldmaskpb.FieldMask{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest using the common.CompareProtoMessage here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the weird computing fields behaviors (unset, unmanaged, swap), I would keep the check to be manual as we have been doing before.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, right... Is nodeCount and processingUnit the only two fields have defaulting values in SpannerInstance?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, we can improve the CompareProtoMessage to apply comparison with customized conditions.

@anhdle-sso anhdle-sso force-pushed the spanner_instance_mapper branch from f64c2ae to c0f4723 Compare December 19, 2024 00:58
Copy link
Collaborator

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Dec 19, 2024
Copy link
Contributor

[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:

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

@google-oss-prow google-oss-prow bot merged commit da53ff0 into GoogleCloudPlatform:master Dec 19, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants