Fix validation for Job with suspend=true,completions=0 to set Complete condition #132614
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mimowo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/sig apps |
|
cc @tenzen-y @atiratree ptal |
ACK |
test/integration/job/job_test.go
Outdated
| // TestSuspendedJobWithZeroCompletions verifies the suspended Job with | ||
| // completions=0 is marked as Complete. | ||
| func TestSuspendedJobWithZeroCompletions(t *testing.T) { |
There was a problem hiding this comment.
Instead of this dedicated one, shouldn't we generalize TestSuspend and add a new case to that?
kubernetes/test/integration/job/job_test.go
Line 4039 in ca314b9
There was a problem hiding this comment.
We could, but it seems the pre-existing test was written with the purpose of checking how the Suspend condition changes when we update the Job, which makes it non-trivial.
Also note we have pre-existing TestSuspendJobControllerRestart. My new test seems easier to consolidate with this one. For now, I align the name of the new test to use the same prefix as the preexisting tests TestSuspendJob.
Ultimately, I see all three tests should be consolidated. Since we already have 2 different tests, I suggest adding a new one, and opening an issue to consolidate them all, wdyt? Otherwise I would probably prefer to consolidate with TestSuspendJobControllerRestart as it is less effort.
There was a problem hiding this comment.
That makes sense. Let us work on a follow-up PR / issue.
I raised the issue: #132621
|
LGTM label has been added. Git tree hash: 38d14c70bff8630eb7af09d18002de4bf6e975d7
|
|
/test pull-kubernetes-integration |
|
/assign @deads2k |
|
@tenzen-y @atiratree @deads2k this is an edge case, but OTOH it is a regression, and the issue was opened by an end-user, see also comment. Would you be supportive for cherry-picking to 1.32 and 1.33? If so I'm happy to prepare the PRs . |
IIUC, since v1.32, this Job behavior were broken user PoV. So, cherry-picking this to 1.32 and 1.33 sounds reasonable. If a suspended Job with |
I'm not clear I understand this. Strengthening validation was discussed under the issue, I think the documentation is not clarifying it, but suspend=true and competitions=0 was supported for many releases prior to 1.32, so I think it makes sense to call it a bug/regression and continue supporting. Now, if we decide this configuration should not be supported (strengthening validation), then it is different PR to prepare. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR is related to:
Fixes #132144
Special notes for your reviewer:
There is an alternative solution, as proposed in #132144 (comment).
I'm ok with both approaches, but with the relaxed validation the fix is smaller in scope as it only tweaks the validation introduced in the ManagedBy feature without setting StartTime.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: