-
Notifications
You must be signed in to change notification settings - Fork 211
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
Use gkehub direct controller for presubmits #2286
base: master
Are you sure you want to change the base?
Conversation
0b0cf1b
to
359e42b
Compare
/assign @yuwenma |
/assign @justinsb as yuwem is on vacation |
/cc @gemmahou as requested |
@ziyue-101: GitHub didn't allow me to request PR reviews from the following users: as, requested. Note that only GoogleCloudPlatform members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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-sigs/prow repository. |
const ctrlName = "gkehubfeaturemembership-controller" | ||
const ( | ||
ctrlName = "gkehubfeaturemembership-controller" | ||
timeout = 5 * time.Minute |
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 if 5 min is enough? I remember gkehubfeaturemembership is a long running resources(takes more than 10min). Shall we have special handling for long running resources?
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.
/cc @shumiao
The timeout is for a GKEHubfeature Update call,
What is the recommended timeout period for such operation?
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.
We don't want to do this in a direct controller. Currently you can just get it pass through like the cbwp controller. We can improve this later
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 cbwp controller uses https://github.com/googleapis/google-cloud-go/tree/main/cloudbuild to create GCP client. However, the corresponding gkehub
one misses gkehub feature fields in the repo https://github.com/googleapis/google-cloud-go/tree/main/gkehub.
By reading the repo description, https://screenshot.googleplex.com/57dR6itytjCs6Ls seems like the repo is still under development
The current gkehub controller uses https://github.com/googleapis/google-api-go-client/tree/main/gkehub/v1beta, which doesn't have Wait
built-in.
Which repo should we use to create GCP clients?
FYI @shumiao might be an opportunity to update https://github.com/googleapis/google-cloud-go/tree/main/gkehub to include v1beta
version
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.
Gemma is right that the long running operation's TTL is 20 minutes, meaning in the worst case, it will take that long be marked done, with either a success or a failure code. The client doesn't have wait
built-in. This is the client side wait
logic internal google3 controllers share: https://source.corp.google.com/piper///depot/google3/cloud/kubernetes/hub/common/hubclient/tools.go;l=38-50;rcl=657751230. If I understand correctly, they don't set a timeout, but wait for it to complete or until the context times out.
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.
@yuwenma Friendly ping, how about I go ahead implement the Wait similar to what shumiao shared?
/hold We do not want to set timer in the direct controller |
359e42b
to
78dbfdc
Compare
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Change description
Tests you have done
make ready-pr
to ensure this PR is ready for review.