fix volcano podgroup update issue

[ckyuto]

What this PR does / why we need it:
This is the fix cause by this PR, the minMember may be updated when the number of replica is changed. However, this also accidentally change the queue value. It also sync up the queue value in the podGroup with the value in runPolicy.SchedulingPolicy.Queue, which is not always applicable to all use cases.

In our use cases we'll inject the queue value according to which org this user belongs to. This change will override the value we set in the queue. The queue value should not be updated once the it is set.

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #

Checklist:

• Docs included if any changes are user facing

[andreyvelich]

Thank you for this fix @ckyuto!
Please can you rebase it ?

[coveralls]

Pull Request Test Coverage Report for Build 8936440813

Details

• 0 of 3 (0.0%) changed or added relevant lines in 1 file are covered.
• 3 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.02%) to 35.398%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controller.v1/common/job.go	0	3	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/controller.v1/mpi/mpijob.go	1	91.06%
pkg/controller.v1/mpi/mpijob_controller.go	2	81.05%

Totals
Change from base Build 8932297222:	0.02%
Covered Lines:	4377
Relevant Lines:	12365

---

💛 - Coveralls

[ckyuto]

@andreyvelich Can you help review?

[tenzen-y]

@ckyuto Could you eliminate irrelevant commits?

[ckyuto]

tenzen-y
Thanks for the comment. Removed.

[ckyuto]

@andreyvelich @tenzen-y I think there's a simple way to fix this. Can I get a review again?

[Tomcli]

/lgtm

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Tomcli
Once this PR has been reviewed and has the lgtm label, please assign johnugeorge for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Tomcli]

Hi @tenzen-y @andreyvelich Could you help trigger the CI test for this PR? We can add a simple unit test if needed. Thanks

[tenzen-y]

Hi @tenzen-y @andreyvelich Could you help trigger the CI test for this PR? We can add a simple unit test if needed. Thanks

Sure, we should review and evaluate this PR during the code freeze.
/assign

[tenzen-y]

@ckyuto This overriding looks fine, but this might be surprising for users since users can not observe this overriding anywhere.
So, could you implement validation webhooks here as well?

https://github.com/kubeflow/training-operator/tree/73398806a64899a36263860c1d34bdc2dd1ec291/pkg/webhooks

[ckyuto]

Thanks @tenzen-y . I'm not sure what should I validate here. Can you elaborate?
I think this should be the expected behavior for most use cases. This change prevent the queue being overridden by the change of this PR, which originally just intended to sync minMember of queue name to replica but accidentally sync the queue name.

[tenzen-y]

@ckyuto You can prevent updating on queue name here:

training-operator/pkg/webhooks/paddlepaddle/paddlepaddle_webhook.go

Line 61 in 7339880

func (w Webhook) ValidateUpdate(ctx context.Context, _, newObj runtime.Object) (admission.Warnings, error) {

Also, could you implement the same validation in the PyTorchJob, TFJob, and XGBoost as well?

[tenzen-y]

@ckyuto Ah, I guess that we can use CEL validation here instead of implementing validations like this.
Could you try it?

type SchedulingPolicy struct { 
 	MinAvailable           *int32                                 `json:"minAvailable,omitempty"` 
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="field is immutable"
 	Queue                  string                                 `json:"queue,omitempty"` 
[...]

training-operator/pkg/apis/kubeflow.org/v1/common_types.go

Lines 226 to 234 in 7339880

	// SchedulingPolicy encapsulates various scheduling policies of the distributed training
	// job, for example `minAvailable` for gang-scheduling.
	type SchedulingPolicy struct {
	MinAvailable *int32 `json:"minAvailable,omitempty"`
	Queue string `json:"queue,omitempty"`
	MinResources *map[v1.ResourceName]resource.Quantity `json:"minResources,omitempty"`
	PriorityClass string `json:"priorityClass,omitempty"`
	ScheduleTimeoutSeconds *int32 `json:"scheduleTimeoutSeconds,omitempty"`
	}

[ckyuto]

@tenzen-y No, none of above will work. These objects are validation for the value of kubeflow jobs, and these value won't be changed after the job is created. The queue value update happens on the queue in volcano podGroup.

[tenzen-y]

We should block updating the .runPolicy.schedulingPolicy.queue since the field is propagated to the PodGroup resource. We should not override or ignore the defined field without notification for the users.

I didn't want to say that we should introduce webhook validation instead of controller logic.
I wanted to say that we should implement the validation as well.

If we don't implement the webhook validations, users can not find the reason why updated .runPolicy.schedulingPolicy.queue was not propagated to the PodGroup resource.

[andreyvelich]

@ckyuto @tenzen-y Do we have any updates here ?
Do we need to include this fix as part of 1.8 release ?

[tenzen-y]

I'm still waiting for response from @ckyuto.

[ckyuto]

Hi, I am in a travel. I’ll have a commit next Mon to address the comments. Can you wait?Sent from my iPhoneOn May 24, 2024, at 3:32 PM, Yuki Iwai ***@***.***> wrote: @tenzen-y commented on this pull request. In pkg/controller.v1/common/job.go:

+ if q := volcanoPodGroup.Spec.Queue; len(q) > 0 {

+ queue = q + } I'm still waiting for response from @ckyuto. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[tenzen-y]

Hi, I am in a travel. I’ll have a commit next Mon to address the comments. Can you wait?Sent from my iPhoneOn May 24, 2024, at 3:32 PM, Yuki Iwai @.***> wrote: @tenzen-y commented on this pull request. In pkg/controller.v1/common/job.go:

• if q := volcanoPodGroup.Spec.Queue; len(q) > 0 {
• queue = q + } I'm still waiting for response from @ckyuto. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

Sure, safe travel 👍