Set completion time when job exceed specified deadline. #1150

SimonCqk · 2020-03-31T08:12:13Z

If job stays in Created and all its controlled pods waits in Pending state, util it exceed its ActiveDeadline(if job.spec.ActiveDeadlineSeconds has been set), this job will be reconciled as past active deadline and be cleaned up, however, job.status.completionTime remains a nil pointer, and it will absolutely panic in JobController.cleanupJob func.

kubeflow-bot · 2020-03-31T08:12:19Z

This change is

k8s-ci-robot · 2020-03-31T08:12:24Z

Hi @SimonCqk. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

SimonCqk · 2020-03-31T08:14:55Z

/assign @johnugeorge

TravisBuddy · 2020-03-31T08:15:54Z

Travis tests have failed

Hey @SimonCqk,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

go build -o tf-operator.v1 github.com/kubeflow/tf-operator/cmd/tf-operator.v1

# github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow
pkg/controller.v1/tensorflow/controller.go:426:3: undefined: jobStatus

golangci-lint run ./...

pkg/controller.v1/tensorflow/controller.go:426:3: undeclared name: `jobStatus` (typecheck)
		jobStatus.CompletionTime = &now
		^

goveralls -service=travis-ci -v -package ./pkg/... -ignore "pkg/client/*/*.go,pkg/client/*/*/*.go,pkg/client/*/*/*/*.go,pkg/client/*/*/*/*/*.go,pkg/client/*/*/*/*/*/*.go,pkg/client/*/*/*/*/*/*/*.go,pkg/util/*.go,pkg/util/*/*.go,pkg/apis/tensorflow/*/zz_generated.*.go,pkg/apis/tensorflow/*/*_generated.go,pkg/apis/common/*/zz_generated.*.go,pkg/apis/common/*/*_generated.go"

=== RUN   TestSetTypeNames
--- PASS: TestSetTypeNames (0.00s)
=== RUN   TestSetDefaultTFJob
--- PASS: TestSetDefaultTFJob (0.00s)
=== RUN   TestIsChieforMaster
--- PASS: TestIsChieforMaster (0.00s)
PASS
coverage: 27.7% of statements in github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/apis/tensorflow/validation, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/fake, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/scheme, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1/fake, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/internalinterfaces, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/listers/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/common/jobcontroller, github.com/kubeflow/tf-operator/pkg/common/util/v1/testutil, github.com/kubeflow/tf-operator/pkg/common/util/v1/unstructured, github.com/kubeflow/tf-operator/pkg/control, github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow, github.com/kubeflow/tf-operator/pkg/logger, github.com/kubeflow/tf-operator/pkg/util, github.com/kubeflow/tf-operator/pkg/util/k8sutil, github.com/kubeflow/tf-operator/pkg/util/signals, github.com/kubeflow/tf-operator/pkg/util/train, github.com/kubeflow/tf-operator/pkg/version
ok  	github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1	0.034s	coverage: 27.7% of statements in github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/apis/tensorflow/validation, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/fake, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/scheme, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1/fake, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/internalinterfaces, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/listers/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/common/jobcontroller, github.com/kubeflow/tf-operator/pkg/common/util/v1/testutil, github.com/kubeflow/tf-operator/pkg/common/util/v1/unstructured, github.com/kubeflow/tf-operator/pkg/control, github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow, github.com/kubeflow/tf-operator/pkg/logger, github.com/kubeflow/tf-operator/pkg/util, github.com/kubeflow/tf-operator/pkg/util/k8sutil, github.com/kubeflow/tf-operator/pkg/util/signals, github.com/kubeflow/tf-operator/pkg/util/train, github.com/kubeflow/tf-operator/pkg/version
=== RUN   TestValidateV1TFJobSpec
time="2020-03-31T08:15:18Z" level=error msg="TFJobSpec is not valid: Image is undefined in the container of Worker"
time="2020-03-31T08:15:18Z" level=error msg="TFJobSpec is not valid: There is no container named tensorflow in Worker"
--- PASS: TestValidateV1TFJobSpec (0.00s)
PASS
coverage: 20.1% of statements in github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/apis/tensorflow/validation, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/fake, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/scheme, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1/fake, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/internalinterfaces, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/listers/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/common/jobcontroller, github.com/kubeflow/tf-operator/pkg/common/util/v1/testutil, github.com/kubeflow/tf-operator/pkg/common/util/v1/unstructured, github.com/kubeflow/tf-operator/pkg/control, github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow, github.com/kubeflow/tf-operator/pkg/logger, github.com/kubeflow/tf-operator/pkg/util, github.com/kubeflow/tf-operator/pkg/util/k8sutil, github.com/kubeflow/tf-operator/pkg/util/signals, github.com/kubeflow/tf-operator/pkg/util/train, github.com/kubeflow/tf-operator/pkg/version
ok  	github.com/kubeflow/tf-operator/pkg/apis/tensorflow/validation	0.032s	coverage: 20.1% of statements in github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/apis/tensorflow/validation, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/fake, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/scheme, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1/fake, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/internalinterfaces, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/listers/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/common/jobcontroller, github.com/kubeflow/tf-operator/pkg/common/util/v1/testutil, github.com/kubeflow/tf-operator/pkg/common/util/v1/unstructured, github.com/kubeflow/tf-operator/pkg/control, github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow, github.com/kubeflow/tf-operator/pkg/logger, github.com/kubeflow/tf-operator/pkg/util, github.com/kubeflow/tf-operator/pkg/util/k8sutil, github.com/kubeflow/tf-operator/pkg/util/signals, github.com/kubeflow/tf-operator/pkg/util/train, github.com/kubeflow/tf-operator/pkg/version
?   	github.com/kubeflow/tf-operator/pkg/client/clientset/versioned	[no test files]
?   	github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/fake	[no test files]
?   	github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/scheme	[no test files]
?   	github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1	[no test files]
?   	github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1/fake	[no test files]
?   	github.com/kubeflow/tf-operator/pkg/client/informers/externalversions	[no test files]
?   	github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/internalinterfaces	[no test files]
?   	github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow	[no test files]
?   	github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow/v1	[no test files]
?   	github.com/kubeflow/tf-operator/pkg/client/listers/tensorflow/v1	[no test files]
=== RUN   TestGenGeneralName
--- PASS: TestGenGeneralName (0.00s)
PASS
coverage: 0.5% of statements in github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/apis/tensorflow/validation, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/fake, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/scheme, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1/fake, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/internalinterfaces, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/listers/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/common/jobcontroller, github.com/kubeflow/tf-operator/pkg/common/util/v1/testutil, github.com/kubeflow/tf-operator/pkg/common/util/v1/unstructured, github.com/kubeflow/tf-operator/pkg/control, github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow, github.com/kubeflow/tf-operator/pkg/logger, github.com/kubeflow/tf-operator/pkg/util, github.com/kubeflow/tf-operator/pkg/util/k8sutil, github.com/kubeflow/tf-operator/pkg/util/signals, github.com/kubeflow/tf-operator/pkg/util/train, github.com/kubeflow/tf-operator/pkg/version
ok  	github.com/kubeflow/tf-operator/pkg/common/jobcontroller	0.015s	coverage: 0.5% of statements in github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/apis/tensorflow/validation, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/fake, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/scheme, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1/fake, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/internalinterfaces, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/listers/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/common/jobcontroller, github.com/kubeflow/tf-operator/pkg/common/util/v1/testutil, github.com/kubeflow/tf-operator/pkg/common/util/v1/unstructured, github.com/kubeflow/tf-operator/pkg/control, github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow, github.com/kubeflow/tf-operator/pkg/logger, github.com/kubeflow/tf-operator/pkg/util, github.com/kubeflow/tf-operator/pkg/util/k8sutil, github.com/kubeflow/tf-operator/pkg/util/signals, github.com/kubeflow/tf-operator/pkg/util/train, github.com/kubeflow/tf-operator/pkg/version
?   	github.com/kubeflow/tf-operator/pkg/common/util/v1/testutil	[no test files]
?   	github.com/kubeflow/tf-operator/pkg/common/util/v1/unstructured	[no test files]
=== RUN   TestCreatePods
--- PASS: TestCreatePods (0.01s)
=== RUN   TestCreateService
time="2020-03-31T08:15:43Z" level=info msg="Controller test-tfjob created service empty_service"
--- PASS: TestCreateService (0.00s)
=== RUN   TestCreateServicesWithControllerRef
time="2020-03-31T08:15:43Z" level=info msg="Controller test-tfjob created service empty_service"
--- PASS: TestCreateServicesWithControllerRef (0.00s)
=== RUN   TestClaimServices
--- PASS: TestClaimServices (0.00s)
PASS
coverage: 24.4% of statements in github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/apis/tensorflow/validation, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/fake, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/scheme, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1/fake, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/internalinterfaces, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/listers/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/common/jobcontroller, github.com/kubeflow/tf-operator/pkg/common/util/v1/testutil, github.com/kubeflow/tf-operator/pkg/common/util/v1/unstructured, github.com/kubeflow/tf-operator/pkg/control, github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow, github.com/kubeflow/tf-operator/pkg/logger, github.com/kubeflow/tf-operator/pkg/util, github.com/kubeflow/tf-operator/pkg/util/k8sutil, github.com/kubeflow/tf-operator/pkg/util/signals, github.com/kubeflow/tf-operator/pkg/util/train, github.com/kubeflow/tf-operator/pkg/version
ok  	github.com/kubeflow/tf-operator/pkg/control	0.052s	coverage: 24.4% of statements in github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/apis/tensorflow/validation, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/fake, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/scheme, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1/fake, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/internalinterfaces, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow, github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/client/listers/tensorflow/v1, github.com/kubeflow/tf-operator/pkg/common/jobcontroller, github.com/kubeflow/tf-operator/pkg/common/util/v1/testutil, github.com/kubeflow/tf-operator/pkg/common/util/v1/unstructured, github.com/kubeflow/tf-operator/pkg/control, github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow, github.com/kubeflow/tf-operator/pkg/logger, github.com/kubeflow/tf-operator/pkg/util, github.com/kubeflow/tf-operator/pkg/util/k8sutil, github.com/kubeflow/tf-operator/pkg/util/signals, github.com/kubeflow/tf-operator/pkg/util/train, github.com/kubeflow/tf-operator/pkg/version
FAIL	github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow [build failed]
exit status 2: warning: no packages being tested depend on matches for pattern github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/fake
warning: no packages being tested depend on matches for pattern github.com/kubeflow/tf-operator/pkg/client/clientset/versioned/typed/tensorflow/v1/fake
warning: no packages being tested depend on matches for pattern github.com/kubeflow/tf-operator/pkg/util
warning: no packages being tested depend on matches for pattern github.com/kubeflow/tf-operator/pkg/util/signals
warning: no packages being tested depend on matches for pattern github.com/kubeflow/tf-operator/pkg/version
# github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow [github.com/kubeflow/tf-operator/pkg/controller.v1/tensorflow.test]
pkg/controller.v1/tensorflow/controller.go:426: undefined: jobStatus in jobStatus.CompletionTime

TravisBuddy Request Identifier: d3601670-7327-11ea-8d6e-5f9eb1f9039f

gaocegege

/lgtm
/assign @ChanYiLin

Thanks for your contribution! 🎉 👍

coveralls · 2020-03-31T09:46:32Z

Coverage remained the same at 96.512% when pulling 21e7089 on SimonCqk:master into 95a0f62 on kubeflow:master.

ChanYiLin · 2020-04-06T03:02:10Z

Hi,
I think I have fixed the issue before
so if the tfJobExceedsLimit is true
then it will

deletePodsAndServices
https://github.com/kubeflow/tf-operator/blob/master/pkg/controller.v1/tensorflow/controller.go#L432
record the CompletionTime
https://github.com/kubeflow/tf-operator/blob/master/pkg/controller.v1/tensorflow/controller.go#L447

in the following if-else
so job.status.completionTime will not be a nil pointer in this case.
Please correct me if I missed something.

SimonCqk · 2020-04-07T02:47:10Z

Hi,
I think I have fixed the issue before
so if the tfJobExceedsLimit is true
then it will

deletePodsAndServices
https://github.com/kubeflow/tf-operator/blob/master/pkg/controller.v1/tensorflow/controller.go#L432

record the CompletionTime
https://github.com/kubeflow/tf-operator/blob/master/pkg/controller.v1/tensorflow/controller.go#L447

in the following if-else
so job.status.completionTime will not be a nil pointer in this case.
Please correct me if I missed something.

hi，the panic exception happens inside cleanupTFJob while the completion time has not set yet, and the job.status.completionTime=now should be promoted.

johnugeorge · 2020-04-07T04:47:05Z

/lgtm

ChanYiLin · 2020-04-07T08:59:49Z

Hi,
I think I have fixed the issue before
so if the tfJobExceedsLimit is true
then it will

deletePodsAndServices
https://github.com/kubeflow/tf-operator/blob/master/pkg/controller.v1/tensorflow/controller.go#L432

record the CompletionTime
https://github.com/kubeflow/tf-operator/blob/master/pkg/controller.v1/tensorflow/controller.go#L447

in the following if-else
so job.status.completionTime will not be a nil pointer in this case.
Please correct me if I missed something.

hi，the panic exception happens inside cleanupTFJob while the completion time has not set yet, and the job.status.completionTime=now should be promoted.

Ok, I got it !
Then the fix should be like

if tfJobExceedsLimit {
                // move the following code snippet to here
                if tfjob.Status.CompletionTime == nil {
			now := metav1.Now()
			tfjob.Status.CompletionTime = &now
		}

		// If the TFJob exceeds backoff limit or is past active deadline
		// delete all pods and services, then set the status to failed
		if err := tc.deletePodsAndServices(tfjob, pods); err != nil {
			return err
		}
		if err := tc.cleanupTFJob(tfjob); err != nil {
			return err
		}
		if tc.Config.EnableGangScheduling {
			if err := tc.DeletePodGroup(tfjob); err != nil {
				return err
			}
		}
		tc.Recorder.Event(tfjob, v1.EventTypeNormal, tfJobFailedReason, failureMessage)
		 
                 // remove the following code snippet
                 // if tfjob.Status.CompletionTime == nil {
		//	now := metav1.Now()
		//	tfjob.Status.CompletionTime = &now
		// }
...
...
}

In your fix, there are two issues

it will update the finishtime every time when reconciling , because you didn't add if tfjob.Status.CompletionTime == nil condition
you also have to do the same thing to if exceedsBackoffLimit || pastBackoffLimit this condition, because this is also the same situation as tfJobExceedsLimit.
Thus, I think adding the code snippet to the if tfJobExceedsLimit{...} scope as above is much better.
Then you don't need to add finish time to both condition if exceedsBackoffLimit || pastBackoffLimit{...} else if tc.pastActiveDeadline(tfjob){...}

SimonCqk · 2020-04-07T11:54:09Z

Hi,
I think I have fixed the issue before
so if the tfJobExceedsLimit is true
then it will

deletePodsAndServices
https://github.com/kubeflow/tf-operator/blob/master/pkg/controller.v1/tensorflow/controller.go#L432

record the CompletionTime
https://github.com/kubeflow/tf-operator/blob/master/pkg/controller.v1/tensorflow/controller.go#L447

in the following if-else
so job.status.completionTime will not be a nil pointer in this case.
Please correct me if I missed something.

hi，the panic exception happens inside cleanupTFJob while the completion time has not set yet, and the job.status.completionTime=now should be promoted.

Ok, I got it !
Then the fix should be like
if tfJobExceedsLimit {
                // move the following code snippet to here
                if tfjob.Status.CompletionTime == nil {
			now := metav1.Now()
			tfjob.Status.CompletionTime = &now
		}

		// If the TFJob exceeds backoff limit or is past active deadline
		// delete all pods and services, then set the status to failed
		if err := tc.deletePodsAndServices(tfjob, pods); err != nil {
			return err
		}
		if err := tc.cleanupTFJob(tfjob); err != nil {
			return err
		}
		if tc.Config.EnableGangScheduling {
			if err := tc.DeletePodGroup(tfjob); err != nil {
				return err
			}
		}
		tc.Recorder.Event(tfjob, v1.EventTypeNormal, tfJobFailedReason, failureMessage)
		 
                 // remove the following code snippet
                 // if tfjob.Status.CompletionTime == nil {
		//	now := metav1.Now()
		//	tfjob.Status.CompletionTime = &now
		// }
...
...
}
In your fix, there are two issues

it will update the finishtime every time when reconciling , because you didn't add if tfjob.Status.CompletionTime == nil condition

you also have to do the same thing to if exceedsBackoffLimit || pastBackoffLimit this condition, because this is also the same situation as tfJobExceedsLimit.
Thus, I think adding the code snippet to the if tfJobExceedsLimit{...} scope as above is much better.
Then you don't need to add finish time to both condition if exceedsBackoffLimit || pastBackoffLimit{...} else if tc.pastActiveDeadline(tfjob){...}

I got you, I'll fix this soon. THX!

ChanYiLin · 2020-04-08T12:31:53Z

/LGTM
/approve
Thanks for your contribution !

k8s-ci-robot · 2020-04-08T12:32:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChanYiLin

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~~ [ChanYiLin]

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

SimonCqk · 2020-04-09T01:58:01Z

/retest

k8s-ci-robot · 2020-04-09T01:58:23Z

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

In response to this:

/retest

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.

SimonCqk · 2020-04-09T03:27:42Z

@gaocegege @jimexist The test failed and I am not a trusted user.

ChanYiLin · 2020-04-09T03:47:23Z

/ok-to-test

k8s-ci-robot added the needs-ok-to-test label Mar 31, 2020

k8s-ci-robot added the size/XS label Mar 31, 2020

k8s-ci-robot requested review from gaocegege and jimexist March 31, 2020 08:12

k8s-ci-robot assigned johnugeorge Mar 31, 2020

gaocegege reviewed Mar 31, 2020

View reviewed changes

k8s-ci-robot assigned ChanYiLin and gaocegege Mar 31, 2020

k8s-ci-robot added the lgtm label Mar 31, 2020

SimonCqk force-pushed the master branch from e89223c to 3a40be2 Compare March 31, 2020 09:42

k8s-ci-robot removed the lgtm label Mar 31, 2020

SimonCqk requested a review from gaocegege March 31, 2020 09:43

SimonCqk mentioned this pull request Apr 1, 2020

optimize satisfy-expectation computations and fix completion-time panic. kubedl-io/kubedl#44

Merged

k8s-ci-robot added the lgtm label Apr 7, 2020

Set completion time when job exceed specified deadline.

21e7089

SimonCqk force-pushed the master branch from 3a40be2 to 21e7089 Compare April 8, 2020 12:24

k8s-ci-robot removed the lgtm label Apr 8, 2020

k8s-ci-robot added the lgtm label Apr 8, 2020

k8s-ci-robot added the approved label Apr 8, 2020

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Apr 9, 2020

k8s-ci-robot merged commit a74b423 into kubeflow:master Apr 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set completion time when job exceed specified deadline. #1150

Set completion time when job exceed specified deadline. #1150

SimonCqk commented Mar 31, 2020

kubeflow-bot commented Mar 31, 2020

k8s-ci-robot commented Mar 31, 2020

SimonCqk commented Mar 31, 2020

TravisBuddy commented Mar 31, 2020

gaocegege left a comment

coveralls commented Mar 31, 2020 •

edited

Loading

ChanYiLin commented Apr 6, 2020 •

edited

Loading

SimonCqk commented Apr 7, 2020 •

edited

Loading

johnugeorge commented Apr 7, 2020

ChanYiLin commented Apr 7, 2020 •

edited

Loading

SimonCqk commented Apr 7, 2020

ChanYiLin commented Apr 8, 2020

k8s-ci-robot commented Apr 8, 2020

SimonCqk commented Apr 9, 2020

k8s-ci-robot commented Apr 9, 2020

SimonCqk commented Apr 9, 2020

ChanYiLin commented Apr 9, 2020

Set completion time when job exceed specified deadline. #1150

Set completion time when job exceed specified deadline. #1150

Conversation

SimonCqk commented Mar 31, 2020

kubeflow-bot commented Mar 31, 2020

k8s-ci-robot commented Mar 31, 2020

SimonCqk commented Mar 31, 2020

TravisBuddy commented Mar 31, 2020

Travis tests have failed

1st Build

TravisBuddy Request Identifier: d3601670-7327-11ea-8d6e-5f9eb1f9039f

gaocegege left a comment

Choose a reason for hiding this comment

coveralls commented Mar 31, 2020 • edited Loading

ChanYiLin commented Apr 6, 2020 • edited Loading

SimonCqk commented Apr 7, 2020 • edited Loading

johnugeorge commented Apr 7, 2020

ChanYiLin commented Apr 7, 2020 • edited Loading

SimonCqk commented Apr 7, 2020

ChanYiLin commented Apr 8, 2020

k8s-ci-robot commented Apr 8, 2020

SimonCqk commented Apr 9, 2020

k8s-ci-robot commented Apr 9, 2020

SimonCqk commented Apr 9, 2020

ChanYiLin commented Apr 9, 2020

coveralls commented Mar 31, 2020 •

edited

Loading

ChanYiLin commented Apr 6, 2020 •

edited

Loading

SimonCqk commented Apr 7, 2020 •

edited

Loading

ChanYiLin commented Apr 7, 2020 •

edited

Loading