-
Notifications
You must be signed in to change notification settings - Fork 138
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
Support to add ResoureManagerTags to GCP Compute Disk, Image, Snapshot #1377
Support to add ResoureManagerTags to GCP Compute Disk, Image, Snapshot #1377
Conversation
|
Welcome @arkadeepsen! |
Hi @arkadeepsen. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
c272900
to
5fe0523
Compare
1f7ccfb
to
6400a6d
Compare
4726b09
to
3b7aeaf
Compare
3b7aeaf
to
d4a2788
Compare
d4a2788
to
7847650
Compare
7847650
to
9a385e4
Compare
9a385e4
to
e93b90c
Compare
/retest-required |
pkg/common/parameters.go
Outdated
case ParameterKeyResourceTags: | ||
paramResourceTags, err := ConvertTagsStringToMap(v) | ||
if err != nil { | ||
return p, fmt.Errorf("parameters contain invalid tags parameter: %w", err) |
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.
Can you specify the exact name of the parameter key in this error? This will make it easier for the user to address the error (by knowing exactly which parameter key to modify).
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.
@pwschuurman I have added the full string containing the parent ID, tag key and value in the error message returned by ConvertTagsStringToMap()
.
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'm looking for the actual key that the user would be familiar with (eg: ParameterKeyResourceTags) to be present in the output error message. This will allow the user to know what parameter to correct in their gRPC call. The end user will likely be a Kubernetes user, and this parameter should map to a parameter key they are familiar with, so they can update the corresponding parameter on their StorageClass. Without that context, they'll just be left with the value, and it won't be as clear about what parameter to change to fix the error they're seeing.
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.
Got it. Added the name of the parameter key in the error message.
const maxNumberOfTags = 50 | ||
if len(tagsMap) > maxNumberOfTags { | ||
return nil, fmt.Errorf("more than %d tags is not allowed, given: %d", maxNumberOfTags, len(tagsMap)) | ||
} |
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
cmd/gce-pd-csi-driver/main.go
Outdated
@@ -79,6 +79,8 @@ var ( | |||
computeEndpoint *url.URL | |||
allowedComputeEnvironment = []gce.Environment{gce.EnvironmentStaging, gce.EnvironmentProduction} | |||
|
|||
extraTagsStr = flag.String("extra-tags", "", "Extra tags to attach to each Compute Disk, Image, Snapshot created. It is a comma separated list of parent id, key and value like '<parent_id1>/<tag_key1>/<tag_value1>,...,<parent_idN>/<tag_keyN>/<tag_valueN>'. parent_id is the Organization or the Project ID where the tag key and the tag value resources exist. A maximum of 50 tags bindings is allowed for a resource. See https://cloud.google.com/resource-manager/docs/tags/tags-overview, https://cloud.google.com/resource-manager/docs/tags/tags-creating-and-managing for details") |
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.
Great, thanks.
// GCP has a rate limit of 600 requests per minute, restricting | ||
// here to 8 requests per second. | ||
limiter := common.NewLimiter(gcpTagsRequestRateLimit, gcpTagsRequestTokenBucketSize, true) |
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 think one optimization would be to use a shared rate limiter, scoped to a CloudProvider client. This would allow each PDCSI controller instance to rate limit calls across CreateSnapshot/CreateVolume.
The 90 second initial delay seems a bit long. I think in the worst case you'd only need to wait for a maximum of 60 seconds until the per-minute quota is refilled (per the documentation: https://cloud.google.com/compute/api-quota#api-rate-limits).
Also, I don't think having an exponential backoff is that useful here (especially at a rate of 2.0), since the per-minute quota is a fixed window rate limit. You'll either be competing with another client, and exhaust quota before the next minute, or quota will refill and the bucket will be replenished. The only thing you may want to adjust is a negotiation scheme for the per-second bucket rate, but without direct knowledge of the request rate of any other client, this seems complicated to determine.
c781133
to
17dd55b
Compare
/retest-required |
17dd55b
to
fe6154e
Compare
/retest-required |
pkg/common/parameters.go
Outdated
case ParameterKeyResourceTags: | ||
paramResourceTags, err := ConvertTagsStringToMap(v) | ||
if err != nil { | ||
return p, fmt.Errorf("parameters contain invalid tags parameter: %w", err) |
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'm looking for the actual key that the user would be familiar with (eg: ParameterKeyResourceTags) to be present in the output error message. This will allow the user to know what parameter to correct in their gRPC call. The end user will likely be a Kubernetes user, and this parameter should map to a parameter key they are familiar with, so they can update the corresponding parameter on their StorageClass. Without that context, they'll just be left with the value, and it won't be as clear about what parameter to change to fix the error they're seeing.
fe6154e
to
00ed53a
Compare
/retest-required |
1 similar comment
/retest-required |
00ed53a
to
95bee6e
Compare
…mage, Snapshot resources
95bee6e
to
e703794
Compare
/retest-required |
1 similar comment
/retest-required |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkadeepsen, pwschuurman 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR allows users to bind a list of tags to resources created by the driver, namely GCP Compute Disk, Image, Snapshot. The original issue #1319 provides more details on the usefulness of GCP tags.
Which issue(s) this PR fixes:
Fixes #1319
Special notes for your reviewer:
This PR adds the functionality to bind GCP resource manager tags to compute pd resources created by the driver. The tag keys and values will be created by the user and only the tag bindings to the pd resources will be created by the driver. The driver now accepts a new argument,
--extra-tags
, and a list of tags can be provided to the driver using this argument. The tag list is then validated to check if they are in the expected format or not. The list is also validated to check that a tag parent_id and key combination is not used more that once. The tags are attached to the Compute Disk, Image, Snapshot when the corresponding resources are created.Does this PR introduce a user-facing change?:
Yes