-
Notifications
You must be signed in to change notification settings - Fork 228
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
[multikueue] Make the MultiKueue adapter part of the jobframework #2373
[multikueue] Make the MultiKueue adapter part of the jobframework #2373
Conversation
Skipping CI for Draft Pull Request. |
/test all |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
30f9b69
to
91cb893
Compare
/test all |
91cb893
to
69ad097
Compare
/cc @tenzen-y |
ACK |
69ad097
to
51324dc
Compare
/assign |
/hold |
80ae069
to
f3fb4ca
Compare
f3fb4ca
to
a9f3211
Compare
|
||
gotManagersJobSets := &jobsetapi.JobSetList{} | ||
if err := managerClient.List(ctx, gotManagersJobSets); err != nil { | ||
t.Errorf("unexpected list manager's jobsets error %s", 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.
If the List returns an error we can interrupt the test with t.Failure
, then we don't need to use `else below.
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 prefer to continue and check the worker's list as well.
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.
as in the other 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.
I like this overall
/approve
/hold
I'll leave the LGTM to @mimowo
@@ -100,7 +102,7 @@ func (b *batchJobAdapter) SyncJob(ctx context.Context, localClient client.Client | |||
return remoteClient.Create(ctx, &remoteJob) | |||
} | |||
|
|||
func (b *batchJobAdapter) DeleteRemoteObject(ctx context.Context, remoteClient client.Client, key types.NamespacedName) error { | |||
func (b *multikueueAdapter) DeleteRemoteObject(ctx context.Context, remoteClient client.Client, key types.NamespacedName) error { |
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.
It doesn't look like this needs to be part of the interface. Maybe the interface only needs to return an empty object (batchv1.Job in this case). And the rest can be implemented by the admission check.
But it can be also left for a follow up.
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.
Let's keep this for a potential follow-up.
(Keeping it as is could be useful if we plan to support composable jobs )
We could also create an utils generic func like:
func DeleteByKey[T client.Object](ctx, client, key ..)
but the added value may not be that big.
) | ||
|
||
type jobsetAdapter struct{} | ||
type multikueueAdapter struct{} |
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 agree that the prefix is redundant.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, trasc 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 |
/retest (test infra issue) |
Cool, just two nits like this: #2373 (comment) and I'm happy to lgtm. |
Thanks! |
LGTM label has been added. Git tree hash: c267c010a5fd9c76d750c3a7eb87f5a00392e2cd
|
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.
everything looks good to me.
Thank you for driving this refactoring!
…bernetes-sigs#2373) * [multikueue] Make the MultiKueue adapter part of the jobframework * Review Remarks * Review Remarks * Review Remarks
What type of PR is this?
/kind feature
What this PR does / why we need it:
Make the MultiKueue adapter part of the
jobframework
, and:pkg/controller/jobs/...
).integrationManager
capabilities to collect available adapter implementations.Which issue(s) this PR fixes:
Fixes #2350
Special notes for your reviewer:
Does this PR introduce a user-facing change?