[Bug] Finish CleanupJob early if the job is suspended. #2243

mszadkow · 2024-08-29T16:37:16Z

What this PR does / why we need it:
To fix the bug related to the situation when the job was both suspendedandrunPolicy.ttlSecondsAfterFinished` was set.
In such situation CleanupJob was returning an error and activate status of the replicas couldn't be removed.

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

Checklist:

Docs included if any changes are user facing

mszadkow · 2024-08-29T16:37:37Z

cc @tenzen-y

mszadkow · 2024-08-29T16:38:07Z

cc @alculquicondor

google-oss-prow · 2024-08-29T17:01:11Z

@mszadkow: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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/test-infra repository.

coveralls · 2024-08-29T17:18:57Z

Pull Request Test Coverage Report for Build 10631502623

Details

0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.06%) to 31.801%

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

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

Totals
Change from base Build 10600609425:	0.06%
Covered Lines:	3950
Relevant Lines:	12421

💛 - Coveralls

pkg/controller.v1/common/job.go

Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com>

tenzen-y · 2024-08-30T10:15:50Z

pkg/controller.v1/tensorflow/job_test.go

+		Entry("No error with completionTime is nil if suspended", &cleanUpCases{
+			tfJob: tftestutil.NewTFJobWithCleanupJobDelay(1, 2, 0, nil),
+			runPolicy: &kubeflowv1.RunPolicy{
+				TTLSecondsAfterFinished: nil,


Suggested change

TTLSecondsAfterFinished: nil,

TTLSecondsAfterFinished: ptr.To[int32](10),

Shouldn't we need to specify the ttlSecondsAfterFinished?
Because previously, there were bugs in the situations where the Job has ttlsSecondsAfterFinished and has been suspended, right?

hmm yes, this should be another test case

Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com>

alculquicondor

/lgtm

tenzen-y

Great! Thanks!
/approve
/lgtm

google-oss-prow · 2024-08-30T16:31:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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~~ [tenzen-y]

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

* No cleaning up a job if the job is suspended. Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com> * run fmt Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com> * Another test case Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com> --------- Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com>

* No cleaning up a job if the job is suspended. Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com> Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

* No cleaning up a job if the job is suspended. Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com> Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com> Co-authored-by: Michał Szadkowski <michalszadkowski@yahoo.pl>

google-oss-prow bot added the do-not-merge/work-in-progress label Aug 29, 2024

google-oss-prow bot requested review from jinchihe and kuizhiqing August 29, 2024 16:37

google-oss-prow bot added the size/M label Aug 29, 2024

mszadkow force-pushed the bug/broken-preemption-on-suspended-with-ttl branch from 9b3b976 to c4d2858 Compare August 29, 2024 17:00

tenzen-y reviewed Aug 29, 2024

View reviewed changes

pkg/controller.v1/common/job.go Outdated Show resolved Hide resolved

mszadkow force-pushed the bug/broken-preemption-on-suspended-with-ttl branch from c4d2858 to 807c6cc Compare August 30, 2024 08:38

google-oss-prow bot added size/S and removed size/M labels Aug 30, 2024

mszadkow added 2 commits August 30, 2024 11:57

No cleaning up a job if the job is suspended.

2ba6a99

Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com>

run fmt

6a7748a

Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com>

mszadkow force-pushed the bug/broken-preemption-on-suspended-with-ttl branch from 84702d6 to 6a7748a Compare August 30, 2024 09:57

tenzen-y reviewed Aug 30, 2024

View reviewed changes

Another test case

cbc8456

Signed-off-by: Michal Szadkowski <michal_szadkowski@epam.com>

mszadkow force-pushed the bug/broken-preemption-on-suspended-with-ttl branch from 3db44ec to cbc8456 Compare August 30, 2024 10:59

mszadkow marked this pull request as ready for review August 30, 2024 11:28

google-oss-prow bot removed the do-not-merge/work-in-progress label Aug 30, 2024

alculquicondor reviewed Aug 30, 2024

View reviewed changes

google-oss-prow bot assigned alculquicondor Aug 30, 2024

google-oss-prow bot added the lgtm label Aug 30, 2024

tenzen-y reviewed Aug 30, 2024

View reviewed changes

google-oss-prow bot assigned tenzen-y Aug 30, 2024

google-oss-prow bot added the approved label Aug 30, 2024

google-oss-prow bot merged commit 98df3a8 into kubeflow:master Aug 30, 2024
39 checks passed

tenzen-y mentioned this pull request Aug 30, 2024

fix: suspend job status can not turn into Suspended and report error… #2159

Closed

1 task

tenzen-y mentioned this pull request Aug 30, 2024

Release-1.8: Cherry-pick of #2243 #2244

Merged

1 task

mszadkow deleted the bug/broken-preemption-on-suspended-with-ttl branch September 2, 2024 06:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] Finish CleanupJob early if the job is suspended. #2243

[Bug] Finish CleanupJob early if the job is suspended. #2243

mszadkow commented Aug 29, 2024

mszadkow commented Aug 29, 2024

mszadkow commented Aug 29, 2024

google-oss-prow bot commented Aug 29, 2024

coveralls commented Aug 29, 2024 •

edited

Loading

tenzen-y Aug 30, 2024

mszadkow Aug 30, 2024 •

edited

Loading

alculquicondor left a comment

tenzen-y left a comment

google-oss-prow bot commented Aug 30, 2024

	TTLSecondsAfterFinished: nil,
	TTLSecondsAfterFinished: ptr.To[int32](10),

[Bug] Finish CleanupJob early if the job is suspended. #2243

[Bug] Finish CleanupJob early if the job is suspended. #2243

Conversation

mszadkow commented Aug 29, 2024

mszadkow commented Aug 29, 2024

mszadkow commented Aug 29, 2024

google-oss-prow bot commented Aug 29, 2024

coveralls commented Aug 29, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10631502623

Details

💛 - Coveralls

tenzen-y Aug 30, 2024

Choose a reason for hiding this comment

mszadkow Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Aug 30, 2024

coveralls commented Aug 29, 2024 •

edited

Loading

mszadkow Aug 30, 2024 •

edited

Loading