-
Notifications
You must be signed in to change notification settings - Fork 262
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
Start JobFramework controller and webhook on new CRDs availability #2574
Start JobFramework controller and webhook on new CRDs availability #2574
Conversation
Hi @ChristianZaccaria. 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-sigs/prow repository. |
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
cc: @trasc thank you for your previous input on this. |
/ok-to-test |
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.
Nice, I tested this out on a cluster and it worked as expected for me
9cd4aa4
to
a9eee9f
Compare
dda8965
to
2d4543e
Compare
Rebased |
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.
Works as expected.
Hi @trasc, wondering what are the next steps for this PR to be successful and merged? Thank you. |
7f87160
to
c98e070
Compare
…to streamline REST mapping checks
5ab97e2
to
86e480d
Compare
Rebased |
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.
/lgtm
/approve
@@ -123,29 +149,73 @@ func TestSetupControllers(t *testing.T) { | |||
return gvk.GroupVersion() | |||
}) | |||
mapper := apimeta.NewDefaultRESTMapper(gvs) | |||
testMapper := &TestRESTMapper{ | |||
DefaultRESTMapper: mapper, | |||
lock: sync.RWMutex{}, |
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.
nit: this line is not needed
LGTM label has been added. Git tree hash: a08aaa26c555df954eb00e2fbe84214100690f0a
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, ChristianZaccaria, Ygnas 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 |
@tenzen-y @alculquicondor as we are about to prepare the 0.8.1 release - how do you find cherry-picking this PR? I feel this issue is on a boundary between a feature and a bug. It could be considered a feature, because we didn't plan for it up front. OTOH, I participated in some debugging sessions for users, and I think the users' perception was always that this is a bug. Finally, the change does not introduce any API changes, so I would suggest considering it for cherry-picking. |
Thank you for sharing this experience. In that case, I agree with cherry-picking. |
/cherry-pick release-0.8 |
@mimowo: #2574 failed to apply on top of branch "release-0.8":
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. |
There is a conflict in |
Thank you, on it now. |
For reference, here is the generated PR: #2991 Thanks! |
…RDs availability (#2991) * Start Controller and Webhook on new CRDs availability * Log errors from JobFramework controller and webhook * Add test case for delayed JobFramework API becoming available * Wait for API integration to be enabled * Implement synchronization for safe concurrent access * Implement exponential backoff for waitForAPI() and add function type to streamline REST mapping checks * Add Job Framework API to mapper directly * Cast mgr.GetRESTMapper() to *TestRESTMapper and ensure proper locking for tests
…ubernetes-sigs#2574) * Start Controller and Webhook on new CRDs availability * Log errors from JobFramework controller and webhook * Add test case for delayed JobFramework API becoming available * Wait for API integration to be enabled * Implement synchronization for safe concurrent access * Implement exponential backoff for waitForAPI() and add function type to streamline REST mapping checks * Add Job Framework API to mapper directly * Cast mgr.GetRESTMapper() to *TestRESTMapper and ensure proper locking for tests
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add function to wait for API availability using the RESTMapper in controller manager.
This PR checks for availability of JobFramework integrations in the Kubernetes cluster before executing the action().
The function utilises the RESTMapper from the controller-runtime manager to verify the existence of the CRD by its GroupVersionKind (GVK). If one of the CRDs become available, the respective controller and webhook will start.
The user/dev will no longer be required to restart the Kueue pod to allow for integration on new CRDs available.
No restarts on the Kueue pod is performed.
Which issue(s) this PR fixes:
Fixes #2414
Special notes for your reviewer:
Does this PR introduce a user-facing change?