[StatefulSet] Allow to change replicas without queue-name.

[mbobrovskyi]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Allow to change replicas in StatefulSet without queue-name.

Which issue(s) this PR fixes:

Fixes #3996

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix the bug that prevented scaling StatefulSets which aren't managed by Kueue when the "statefulset" integration is enabled.

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	7caaf22
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/678a4b817bd883000808e87f

[mbobrovskyi]

/cc @mimowo

[mimowo]

/hold
I need to first understand and align why we make this change. I see this needed for lws, but I expect we can instead make the skip decision based on the owner reference as we do already in other places.

[mimowo]

Also the current release note I think is not penetrable for users. It says what the code does but it does not say why a user would need to or what problem it solves from user perspective. At least I'm puzzled by what is the user observable issue.

[mbobrovskyi]

/close

[k8s-ci-robot]

@mbobrovskyi: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[mimowo]

/reopen
Could you please @mbobrovskyi continue on the PR? I think it fixes a very important issue for 0.10: #3996. Let me review the code details as I now understand the problem.

[k8s-ci-robot]

@mimowo: Reopened this PR.

In response to this:

/reopen
Could you please @mbobrovskyi continue on the PR? I think it fixes a very important issue for 0.10: #3996. Let me review the code details as I now understand the purpose.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[mimowo]

This covers 90% of cases. However, ideally, even if the StatefulSet has queue-name, but the owner is already managed by Kueue, then we don't do the validation.

It is more of a corner case, but this could matter for the LWS integration or AppWrapper.

Since this is a corner case I'm on a fence. I'm ok to merge as is, but maybe try to add the check @mbobrovskyi ?

Here is the code where we also look at the owner:

kueue/pkg/controller/jobframework/defaults.go

Lines 49 to 57 in 097262a

	// Do not default suspend a job whose owner is already managed by Kueue
	if owner := metav1.GetControllerOf(jobObj); owner != nil && IsOwnerManagedByKueue(owner) {
	return false, nil
	}
	// Jobs with queue names whose parents are not managed by Kueue are default suspended
	if QueueNameForObject(jobObj) != "" {
	return true, nil
	}

. I think it would be great to have the check consistent, unfortunetly not sure we can call this function easily.

[mimowo]

Maybe we introduce a helper function isManagedByKueue, and do both checks?

if parent managed by Kueue then 
 return false
else if hasQueueName
  return true
 return false

[mimowo]

Also the current release note I think is not penetrable for users. It says what the code does but it does not say why a user > would need to or what problem it solves from user perspective. At least I'm puzzled by what is the user observable issue.

Sorry for the comment, in retrospect, I think the release note is good. Maybe we can just slightly tweak it:

Fix the bug that prevented scaling StatefulSets which aren't managed by Kueue when the "statefulset" integration is enabled.

[mimowo]

/hold cancel
I have a good grasp of the issue now. I think the only remaining remark is how to formulate the check. My proposal would be to first exclude StatefulSets which are owned by another CRD managed by Kueue as in the comment, but I think this is not a blocking scenario here.

[mimowo]

/lgtm
/approve
I tested manually e2e and it solves #3996. Thanks!

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 260200ab3eb9b81a741abd864823b96c1d196e33

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbobrovskyi, mimowo

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:

• OWNERS [mimowo]

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

[mimowo]

/cherry-pick release-0.10 release-0.9

[k8s-infra-cherrypick-robot]

@mimowo: once the present PR merges, I will cherry-pick it on top of release-0.10 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.10 release-0.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-infra-cherrypick-robot]

@mimowo: new pull request created: #3997

In response to this:

/cherry-pick release-0.10 release-0.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[mimowo]

/release-note-edit

Fix the bug that prevented scaling StatefulSets which aren't managed by Kueue when the "statefulset" integration is enabled.