this change is less obvious to me. Why does this existing option become true some times? Actually it's more than that. This change takes us from AllowMutableSchedulingDirectives = suspended && notStarted to ignoring whether there is a start time, so a job that has started, but doesn't have an active pod can change. Why did this validation change as part of the pod resources change?

I believe this comes from #134980 (comment).

cc @soltysh @mimowo

The KEP update (done today) was kubernetes/enhancements#5678.

Why does this existing option become true some times?

Yeah, currently the precondition logic is inside the strategy.go. We could also set the option based on the future gate and then check all preconditions inside the validation.go. I think the only explanation is to put the code next to the pre-existing FG which follows the pattern, ie. AllowMutableSchedulingDirectives. +1 to move it to validation.go - from my PoV either in this PR or a small follow up.

Why did this validation change as part of the pod resources change?

The motivation is to align the handling of AllowMutableSchedulingDirectives. Over time it turned out that the check for status.startTime is not good, because:

1. it does not allow to change PodTemplate while the job is suspended, which is essential for Kueue, and thus we do this trick of clearing starttime ourselves. It allows to change only if never started which is too restrictive.
2. It does not really guarantee that all Pods are deleted (have deletionTimestamp), and thus in Kueue we need to defer the stop until status.Active=0, we do it here.
3. There is not much of conceptual difference between scheduling directives, like nodeSelectors, and Pod resources - both influence scheduling decisions, and so it seems reasonable to align the condition for mutability.

I think it is reasonable to align the mutability condition in this KEP, otherwise it would require the overhead of yet another KEP just to align.

I think it is reasonable to align the mutability condition in this KEP, otherwise it would require the overhead of yet another KEP just to align.

I don't see a need for another KEP, but another featuregate with a different graduation lifecycle makes sense. We can update our job controller to manipulate startTime in this way, but we have no practical way of doing that for other controllers and certainly not in skewing cases. This means that we cannot simply change the validation under an unrelated featuregate and promote that featuregate in a normal cycle.

I see, so can we just have another feature gate under this KEP like AdjustMutableSchedulingDirectives?

Our controller might make a change (weird featuregate to wire it to btw), but other job controllers may not. Certainly not on a version skew. I can see the justification for expanding to allow validation for oldCondition || newCondition, but this tightening of validation needs a separate gate and a different cycle for deciding when to force it.

I can appreciate that it is a gap we'd like to close, but suddenly requiring new validation to use unrelated features is untenable.

Opened: #135104 PTAL.

non-blocking remark, but this might be heavy, and we already take the entire template above: https://github.com/kubernetes/kubernetes/pull/132441/files#diff-67fdcc10f97b3bbec1288558a1575efd01474516066db5332a141592ee5dd4e4R642

So we could optimize by reusing the same copy if we do early:

if opts.AllowMutableSchedulingDirectives || opts.AllowMutablePodResources {
   oldTemplate = oldSpec.Template.DeepCopy() // +k8s:verify-mutation:reason=clone
}

This comment is non-blocking.

I think we could rely still on this line if we above just follow the tested pattern of mungling the oldTemplate. It would also save the unnecessary DeepCopy, posted here: https://github.com/kubernetes/kubernetes/pull/132441/files#r2489218558

This comment is non-blocking.

I think it better captures the intention, and also prevents some race conditions when someone could mutate the Job while suspend=false, but the JobSuspended is still there from the previous suspend.

Oh I see you moved this condition here: https://github.com/kubernetes/kubernetes/pull/132441/files#diff-67fdcc10f97b3bbec1288558a1575efd01474516066db5332a141592ee5dd4e4

I think splitting these checks makes it hard to follow. Shouldn't we move also batchvalidation.IsConditionTrue(oldJob.Status.Conditions, batch.JobSuspended) && oldJob.Status.Active == 0 to validation.go?

Also, splitting the checks allows adding tests in validation_test.go which would be detached from reality (bypassing the checks).

I think we could have opts.AllowMutablePodResources = utilfeature.DefaultFeatureGate.Enabled(features.MutablePodResourcesForSuspendedJobs)

I have a thread with david on this.

Validation checks spec and status as two separate function calls.

So in the code I added validation I don't have access to anything on the status field.

Same for updates to status.

I left the commit in because I do find this harder to follow.

ack

The structure is highlighting edges in the "spec cannot stop status updates". We haven't had a peer "status cannot stop spec updates", so it's an interesting dynamic. I'm glad to have the spec related validation all sitting in validation at least.

updated.

Please add a test case which shows that the status.Active==0 checks is used, so

"feature gate enabled - job suspended, but with remaining active Pods  resource updates rejected": {
			enableFeatureGate: true,
			job: &batch.Job{
				ObjectMeta: metav1.ObjectMeta{
					Name:            "myjob",
					Namespace:       metav1.NamespaceDefault,
					ResourceVersion: "0",
				},
				Spec: batch.JobSpec{
					Selector:       validSelector,
					Template:       validPodTemplateSpec,
					ManualSelector: ptr.To(true),
					Parallelism:    ptr.To[int32](1),
					Suspend:        ptr.To(true ),
				},
				Status: batch.JobStatus{
					Active: 1,
					Conditions: []batch.JobCondition{
						{
							Type:   batch.JobSuspended,
							Status: api.ConditionStatus(metav1.ConditionFalse),
						},
					},
				},
			},
			update: func(job *batch.Job) {
				job.Spec.Template.Spec.Containers[0].Resources.Requests = api.ResourceList{
					api.ResourceCPU: resource.MustParse("200m"),
				}
			},
			wantErrs: field.ErrorList{
				{Type: field.ErrorTypeInvalid, Field: "spec.template"},
			},
		},

and

"feature gate enabled - job suspended, with the suspended condition, but pods remain active - reject": {
			enableFeatureGate: true,
			job: &batch.Job{
				ObjectMeta: metav1.ObjectMeta{
					Name:            "myjob",
					Namespace:       metav1.NamespaceDefault,
					ResourceVersion: "0",
				},
				Spec: batch.JobSpec{
					Selector:       validSelector,
					Template:       validPodTemplateSpec,
					ManualSelector: ptr.To(true),
					Parallelism:    ptr.To[int32](1),
					Suspend:        ptr.To(true ),
				},
				Status: batch.JobStatus{
					Active: 1,
					Conditions: []batch.JobCondition{
						{
							Type:   batch.JobSuspended,
							Status: api.ConditionStatus(metav1.ConditionTrue),
						},
					},
				},
			},
			update: func(job *batch.Job) {
				job.Spec.Template.Spec.Containers[0].Resources.Requests = api.ResourceList{
					api.ResourceCPU: resource.MustParse("200m"),
				}
			},
			wantErrs: field.ErrorList{
				{Type: field.ErrorTypeInvalid, Field: "spec.template"},
			},
		},

updated.