fix(framework): validate all ReplicatedJobs in Volcano priorityClassName check - #3918
Conversation
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…ame check Volcano.Validate returned early when a ReplicatedJob used a reserved priority class (system-cluster-critical or system-node-critical), skipping priorityClassName validation for every ReplicatedJob listed after it. Use continue instead of an early return so each ReplicatedJob is validated independently. Fixes kubeflow#3917 Signed-off-by: Abhishek <abhikokadwar2@gmail.com>
c4da52d to
81af251
Compare
|
Friendly ping on this one — it's a small fix (early Happy to rebase or adjust if anything needs changing. cc @robert-bell @kuizhiqing |
What this PR does / why we need it
Volcano.ValidateiteratesjobSetSpec.ReplicatedJobsto check each job'spriorityClassName. When aReplicatedJobused a reserved priority class (system-cluster-criticalorsystem-node-critical), the loop returned early instead of continuing to the next job, so validation was silently skipped for everyReplicatedJoblisted after it.This meant a TrainJob could be admitted with an invalid
priorityClassNameon a laterReplicatedJob, only surfacing as a pod scheduling failure at runtime instead of being rejected at admission time.This PR:
return nil, allErrstocontinueso eachReplicatedJobis validated independently;ReplicatedJobscenarios.Validation
go build ./...go vet ./...go test ./pkg/runtime/framework/plugins/volcano/... -run TestValidate -v— all cases pass, including the two new ones.Fixes #3917