-
Notifications
You must be signed in to change notification settings - Fork 305
Add Managed Connection Pooling support for an AlloyDB Instance #5611
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
base: master
Are you sure you want to change the base?
Conversation
|
The fuzz-roundtrippers check is failing due to a diff that is unrelated to my changes: I think that activation policy field is being worked on separately in #5052 |
maqiuyujoyce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fuzz-roundtrippers check is failing due to a diff that is unrelated to my changes: ...
Could you try to mark this field as unimplemented in the fuzzer? Fuzzer test is a blocker for any PR to be merged.
| @@ -172,6 +172,8 @@ const ( | |||
| DatabaseVersion_POSTGRES_15 DatabaseVersion = 3 | |||
| // The database version is Postgres 16. | |||
| DatabaseVersion_POSTGRES_16 DatabaseVersion = 4 | |||
| // The database version is Postgres 17. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, how did you update these files? Asking because git.versions file is not updated but the .pb.go files are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did change the git.versions file locally and ran ./dev/tasks/update-mockgcp. I just only kept the files that were updated in the AlloyDB folder cause I felt like updating the other API's mockgcp files was out of scope of this PR. But happy to send out a separate PR if need be to update the mockgcp for all APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, then yes, you'll want to update all APIs, otherwise others may revert your change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm having some issues while updating the mockgcp to the latest SHA. After updating them and running make ready-pr I get this error:
level=error msg="Running error: can't run linter goanalysis_metalinter\nfindcall: failed to load package dataplexpb: could not load export data: no export data for \"github.com/GoogleCloudPlatform/k8s-config-connector/mockgcp/generated/google/cloud/dataplex/v1\""
I did make sure that I had go setup correctly (re-did the go setup as per the guide), cleared the golangci-lint cache, tried to force mock gcp to use a different SHA by hard-coding it in dev/tasks/update-mockgcp, and also tried to do the same on a new clean branch synced to the latest off of the master branch.
Plus it seems like updating them also ends up breaking a bunch of tests. I've already reverted the file changes I did for mockgcp. You can see the test failures at: https://github.com/GoogleCloudPlatform/k8s-config-connector/actions/runs/19944914990/job/57191832228?pr=5611
I've also asked about this in the internal chat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can follow up separately. Thanks for working on this!
| @@ -5316,6 +5307,8 @@ type Instance_ConnectionPoolConfig struct { | |||
| Enabled bool `protobuf:"varint,12,opt,name=enabled,proto3" json:"enabled,omitempty"` | |||
| // Optional. Connection Pool flags, as a list of "key": "value" pairs. | |||
| Flags map[string]string `protobuf:"bytes,13,rep,name=flags,proto3" json:"flags,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"` | |||
| // Output only. The number of running poolers per instance. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field is added to Instance_ConnectionPoolConfig type, but it is not reflected in instance type (instance_types.go or types.generated.go). Looks like something is missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, when I generated the changes for instance_types.go and types.generated.go, the connection_pool_config.pooler_count field wasn't included. I've added it in manually in types.generated.go now (after running make ready-pr this also updated some other files too)
For instance_types.go, should be the field be under AlloyDBInstanceStatus? Similar to other output only fields like OutboundPublicIPAddresses. The only difference is though is that pooler_count isn't a top-level field like those are...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it in manually in types.generated.go now (after running make ready-pr this also updated some other files too)
Manual change should be done in XX_types.go file only. So you'll want to move the entire struct to instance_types.go file with your manual change. Otherwise your manual change in the generated file may be overridden.
Besides, could you file a GitHub issue about this and mention me there? It's unexpected to exclude fields in the types generation.
For instance_types.go, should be the field be under AlloyDBInstanceStatus? Similar to other output only fields like OutboundPublicIPAddresses. The only difference is though is that pooler_count isn't a top-level field like those are...
It should be under AlloyDBInstanceObservedState struct. And you can keep the original path under it. To summarize, you should make the following changes:
- In types.generated.go: Remove struct
Instance_ConnectionPoolConfig. - In instance_types.go: Add two structs, one is configurable, one is output only (with
ObservedStatesuffix).
// +kcc:proto=google.cloud.alloydb.v1beta.Instance.ConnectionPoolConfig
type Instance_ConnectionPoolConfig struct {
// Optional. Whether to enable Managed Connection Pool (MCP).
// +kcc:proto:field=google.cloud.alloydb.v1beta.Instance.ConnectionPoolConfig.enabled
Enabled *bool `json:"enabled,omitempty"`
// Optional. Connection Pool flags, as a list of "key": "value" pairs.
// +kcc:proto:field=google.cloud.alloydb.v1beta.Instance.ConnectionPoolConfig.flags
Flags map[string]string `json:"flags,omitempty"`
}
// +kcc:proto=google.cloud.alloydb.v1beta.Instance.ConnectionPoolConfig
type Instance_ConnectionPoolConfigObservedState struct {
// Output only. The number of running poolers per instance.
// +kcc:proto:field=google.cloud.alloydb.v1beta.Instance.ConnectionPoolConfig.pooler_count
PoolerCount *int32 `json:"pooler_count,omitempty"`
}
- Also in instance_types.go: Support
AlloyDBInstanceObservedState.- Uncomment the two places where AlloyDBInstanceObservedState is commented out.
- Add the following field in struct:
// Output for Managed Connection Pool (MCP). ConnectionPoolConfig *Instance_ConnectionPoolConfigObservedState `json:"connectionPoolConfig,omitempty"` - Run
make ready-pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the very detailed explanation+steps! I filed the bug at #5753, and have made the necessary changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Nancy!
After looking at the issue description, I suspect it is the problem of the tool mentioned in the doc, which hasn't been updated for a while (and I actually never used it). We probably want to update our doc.
^ This is outside of the scope of this PR though.
...testdata/basic/alloydb/v1beta1/alloydbinstance/alloydbinstanceconnectionpoolconfig/_http.log
Show resolved
Hide resolved
...testdata/basic/alloydb/v1beta1/alloydbinstance/alloydbinstanceconnectionpoolconfig/_http.log
Show resolved
Hide resolved
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
1f4d983 to
927c1e1
Compare
I think I fixed the fuzzer tests now? I also excluded a few other AlloyDB fields that haven't been onboarded yet to KCC |
…and under spec for the AlloyDB instance fuzzer
|
@maqiuyujoyce the PR is ready for your review again - I did have some issues on updating the mockgcp for all APIs, it'd be appreciated if you could help out there! Thanks! |
maqiuyujoyce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like supporting poolerCount is in your last commit. Do you mind rerunning the test case locally against real and mock to ensure now the logs have no unexpected diffs?
| @@ -172,6 +172,8 @@ const ( | |||
| DatabaseVersion_POSTGRES_15 DatabaseVersion = 3 | |||
| // The database version is Postgres 16. | |||
| DatabaseVersion_POSTGRES_16 DatabaseVersion = 4 | |||
| // The database version is Postgres 17. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can follow up separately. Thanks for working on this!
| @@ -5316,6 +5307,8 @@ type Instance_ConnectionPoolConfig struct { | |||
| Enabled bool `protobuf:"varint,12,opt,name=enabled,proto3" json:"enabled,omitempty"` | |||
| // Optional. Connection Pool flags, as a list of "key": "value" pairs. | |||
| Flags map[string]string `protobuf:"bytes,13,rep,name=flags,proto3" json:"flags,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"` | |||
| // Output only. The number of running poolers per instance. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Nancy!
After looking at the issue description, I suspect it is the problem of the tool mentioned in the doc, which hasn't been updated for a while (and I actually never used it). We probably want to update our doc.
^ This is outside of the scope of this PR though.
BRIEF Change description
Fixes #5610. We add a new nested object field
connection_pool_configto the AlloyDB instance. This will allow customers to enable managed connection pooling and set various settings on it.WHY do we need this change?
Special notes for your reviewer:
I did re-generate the MockGCP files to be on an newer release version of the googeapis. I did this only for the AlloyDB MockGCP files though.
I also did manually edit some files like the mapper, fuzzer, controller, etc because the commands to generate the changes didn't work for me.
Does this PR add something which needs to be 'release noted'?
Additional documentation e.g., references, usage docs, etc.:
Intended Milestone
Please indicate the intended milestone.
Tests you have done
make ready-prto ensure this PR is ready for review.