[Cleanup] Refactor multikueue adapter tests #2869
Conversation
k8s-ci-robot
added
the
release-note-none
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
size/XL
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
mszadkow
force-pushed
the
cleanup/refactor-multikueue-adapter-tests
branch
from
ef45d3e
to
52666e4
Compare
|
/retest |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@alculquicondor @tenzen-y here is the test refactor, please take a look |
| }) | ||
| } | ||
|
|
||
| func finishWorkloadTestBody(wlLookupKey types.NamespacedName, finishJobReason string) { |
There was a problem hiding this comment.
| func finishWorkloadTestBody(wlLookupKey types.NamespacedName, finishJobReason string) { | |
| func waitForWorkloadToFinishAndRemoteWorkloadToBeDeleted(wlLookupKey types.NamespacedName, finishJobReason string) { |
why is only the remote workload deleted, btw?
There was a problem hiding this comment.
Not sure about that, I thought that's enough.
In order to be deleted in manager we would have to update the status.
| @@ -1846,3 +1487,70 @@ var _ = ginkgo.Describe("Multikueue no GC", ginkgo.Ordered, ginkgo.ContinueOnFai | |||
| }) | |||
| }) | |||
| }) | |||
|
|
|||
| func jobAdmissionTestBody(acName string, wlLookupKey types.NamespacedName, admission *utiltesting.AdmissionWrapper) { | |||
There was a problem hiding this comment.
| func jobAdmissionTestBody(acName string, wlLookupKey types.NamespacedName, admission *utiltesting.AdmissionWrapper) { | |
| func admitWorkloadAndCheckWorkerCopies(acName string, wlLookupKey types.NamespacedName, admission *utiltesting.AdmissionWrapper) { |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: b0337b5ada71a8a7c693382fa780647ed7d9a3c7
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, mszadkow 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 |
k8s-ci-robot
added
the
approved
There was a problem hiding this comment.
My assumption was reducing the e2e test cases between all KFJob integrations because the MK adapters for the KFJobs are consolidated in https://github.com/kubernetes-sigs/kueue/blob/6001753df1c59455686cbce7143298d78b1c7cae/pkg/controller/jobs/kubeflow/kubeflowjob/kubeflowjob_multikueue_adapter.go.
So, I would recommend that we have MK e2e for KFJobs only for PyTorchJob.
This allows us to reduce e2e testing duration.
@alculquicondor Any objections about having MK e2e testing only for PyTorchJob?
There was a problem hiding this comment.
As long as PyTorchJob is representative enough (it has more than one role, for example), I'm fine with that.
There was a problem hiding this comment.
We can use the PyTorchJob with the "Master" and "Worker" role patterns.
@mszadkow Could you take this?
There was a problem hiding this comment.
@tenzen-y sure, that sounds ok to me.
One e2e for KFJob with PyTorchJob, but another e2e for MPIJob as it's handled with another operator, is that ok?
There was a problem hiding this comment.
@mszadkow Yes, that is my expectation :)
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Too much of repetitive code for tests.
Which issue(s) this PR fixes:
Relates #2552
Special notes for your reviewer:
Does this PR introduce a user-facing change?