check job ActiveDeadlineSeconds #48454

weiwei04 · 2017-07-04T06:10:54Z

What this PR does / why we need it:

enqueue a sync task after ActiveDeadlineSeconds

Which issue this PR fixes *:

Special notes for your reviewer:

Release note:

enqueue a sync task to wake up jobcontroller to check job ActiveDeadlineSeconds in time

k8s-ci-robot · 2017-07-04T06:11:02Z

Hi @weiwei04. Thanks for your PR.

I'm waiting for a kubernetes 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.

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. I understand the commands that are listed here.

weiwei04 · 2017-07-04T06:21:49Z

/assign @janetkuo

k8s-ci-robot · 2017-07-04T06:21:50Z

@weiwei04: GitHub didn't allow me to assign the following users: PTAL.

Note that only kubernetes members can be assigned.

In response to this:

/assign @janetkuo PTAL

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.

weiwei04 · 2017-07-07T02:50:34Z

@janetkuo please take a look, thanks.

soltysh · 2017-07-31T13:54:18Z

@weiwei04 sorry, was out for quite a long, will look into it today or tomorrow.

weiwei04 · 2017-08-01T01:25:02Z

@soltysh thanks:)

soltysh · 2017-08-02T12:04:44Z

/assign

soltysh

Mostly nits, overall looks good.

soltysh · 2017-08-02T12:06:55Z

pkg/controller/job/jobcontroller.go

+	curJob := cur.(*batch.Job)
+
+	// never return error
+	key, _ := controller.KeyFunc(curJob)


There's no point in re-trying if there was an error. Check error and just return if that happens.

soltysh · 2017-08-02T12:13:28Z

pkg/controller/job/jobcontroller.go

+	jm.queue.Add(key)
+	// check if need to add a new rsync for ActiveDeadlineSeconds
+	if curJob.Status.StartTime != nil {
+		curSeconds := curJob.Spec.ActiveDeadlineSeconds


Can you please use currentADS and oldADS it's less confusing name :)

yes ADS would be a more meaningful name:)

soltysh · 2017-08-02T12:15:09Z

staging/src/k8s.io/api/batch/v1/types.go

@@ -150,6 +150,10 @@ const (
 	JobFailed JobConditionType = "Failed"
 )

+const (


Not needed, drop it. It'll cause only confusion wrt to our API.

soltysh · 2017-08-02T12:16:17Z

test/e2e/apps/job.go

@@ -28,6 +28,8 @@ import (
 	"k8s.io/kubernetes/pkg/kubectl"
 	"k8s.io/kubernetes/test/e2e/framework"

+	batch "k8s.io/api/batch/v1"


Use batchv1 as the name alias, to make it clear it's that version.

soltysh · 2017-08-02T12:17:34Z

test/e2e/apps/job.go

@@ -86,9 +88,20 @@ var _ = SIGDescribe("Job", func() {
 		Expect(err).NotTo(HaveOccurred())
 	})

+	It("should exceed active deadline", func() {
+		By("Creating a job")
+		var activeDeadlineSeconds int64 = 10


Make it shorter, this will cause the entire test suite longer by 10 seconds. I'd propose 1-2 seconds max.

changed to 2 seconds.

weiwei04 · 2017-08-08T06:15:45Z

@soltysh modified code according to your comments, please take a look, thanks:)

dims · 2017-08-09T20:27:26Z

/ok-to-test

weiwei04 · 2017-08-10T05:16:46Z

/test pull-kubernetes-e2e-gce-etcd3

weiwei04 · 2017-08-14T02:47:09Z

/test pull-kubernetes-e2e-gce-etcd3

weiwei04 · 2017-08-14T04:01:54Z

@soltysh modified code according to your comments, and all the test are passed, please take a look, thanks:)

weiwei04 · 2017-08-17T09:24:50Z

ping @soltysh :)

weiwei04 · 2017-08-25T03:25:18Z

ping @soltysh :)

k8s-ci-robot · 2017-08-25T03:25:20Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

weiwei04 · 2017-08-29T08:19:25Z

ping @soltysh :)

soltysh · 2017-08-29T11:08:42Z

@weiwei04 sorry for the long silence, looking at it right now...

soltysh

LGTM

soltysh · 2017-08-29T11:32:40Z

@weiwei04 mind squashing these changes into a single commit and I'll apply all the necessary labels for merge

soltysh · 2017-08-29T12:36:13Z

/test pull-kubernetes-node-e2e

soltysh · 2017-08-29T12:36:22Z

/lgtm
/approve

k8s-github-robot · 2017-08-29T12:36:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, weiwei04

Associated issue: 32149

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/controller/job/OWNERS~~ [soltysh]
~~test/OWNERS~~ [soltysh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

weiwei04 · 2017-08-29T13:08:12Z

Thanks:)

k8s-github-robot · 2017-08-29T15:25:05Z

Automatic merge from submit-queue (batch tested with PRs 44719, 48454)

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 4, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 4, 2017

k8s-github-robot assigned janetkuo and madhusudancs Jul 4, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 4, 2017

weiwei04 force-pushed the check-job-activeDeadlineSeconds branch from 8ebd760 to 0432a5f Compare July 4, 2017 06:19

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2017

weiwei04 force-pushed the check-job-activeDeadlineSeconds branch from 0432a5f to 5a888ac Compare July 19, 2017 08:05

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 19, 2017

weiwei04 mentioned this pull request Jul 31, 2017

Mark exceed activeDeadlineSeconds job with completions == succeed pod as Complete instead of Failed #46499

Closed

weiwei04 force-pushed the check-job-activeDeadlineSeconds branch from 5a888ac to 0218c72 Compare July 31, 2017 06:41

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2017

k8s-ci-robot assigned soltysh Aug 2, 2017

soltysh requested changes Aug 2, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2017

weiwei04 force-pushed the check-job-activeDeadlineSeconds branch from 36896d9 to 1662f30 Compare August 8, 2017 06:10

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 9, 2017

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 25, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 29, 2017

soltysh added this to the v1.8 milestone Aug 29, 2017

soltysh approved these changes Aug 29, 2017

View reviewed changes

check job ActiveDeadlineSeconds

46239ea

weiwei04 force-pushed the check-job-activeDeadlineSeconds branch from 92e91cc to 46239ea Compare August 29, 2017 12:15

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2017

k8s-github-robot merged commit 25da6e6 into kubernetes:master Aug 29, 2017

weiwei04 deleted the check-job-activeDeadlineSeconds branch August 30, 2017 02:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check job ActiveDeadlineSeconds #48454

check job ActiveDeadlineSeconds #48454

weiwei04 commented Jul 4, 2017 •

edited

Loading

k8s-ci-robot commented Jul 4, 2017

weiwei04 commented Jul 4, 2017 •

edited

Loading

k8s-ci-robot commented Jul 4, 2017

weiwei04 commented Jul 7, 2017

soltysh commented Jul 31, 2017

weiwei04 commented Aug 1, 2017

soltysh commented Aug 2, 2017

soltysh left a comment

soltysh Aug 2, 2017

soltysh Aug 2, 2017

weiwei04 Aug 8, 2017

soltysh Aug 2, 2017

soltysh Aug 2, 2017

soltysh Aug 2, 2017

weiwei04 Aug 8, 2017

weiwei04 commented Aug 8, 2017

dims commented Aug 9, 2017

weiwei04 commented Aug 10, 2017

weiwei04 commented Aug 14, 2017

weiwei04 commented Aug 14, 2017

weiwei04 commented Aug 17, 2017

weiwei04 commented Aug 25, 2017

k8s-ci-robot commented Aug 25, 2017

weiwei04 commented Aug 29, 2017

soltysh commented Aug 29, 2017

soltysh left a comment

soltysh commented Aug 29, 2017

soltysh commented Aug 29, 2017

soltysh commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

weiwei04 commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

check job ActiveDeadlineSeconds #48454

check job ActiveDeadlineSeconds #48454

Conversation

weiwei04 commented Jul 4, 2017 • edited Loading

k8s-ci-robot commented Jul 4, 2017

weiwei04 commented Jul 4, 2017 • edited Loading

k8s-ci-robot commented Jul 4, 2017

weiwei04 commented Jul 7, 2017

soltysh commented Jul 31, 2017

weiwei04 commented Aug 1, 2017

soltysh commented Aug 2, 2017

soltysh left a comment

Choose a reason for hiding this comment

soltysh Aug 2, 2017

Choose a reason for hiding this comment

soltysh Aug 2, 2017

Choose a reason for hiding this comment

weiwei04 Aug 8, 2017

Choose a reason for hiding this comment

soltysh Aug 2, 2017

Choose a reason for hiding this comment

soltysh Aug 2, 2017

Choose a reason for hiding this comment

soltysh Aug 2, 2017

Choose a reason for hiding this comment

weiwei04 Aug 8, 2017

Choose a reason for hiding this comment

weiwei04 commented Aug 8, 2017

dims commented Aug 9, 2017

weiwei04 commented Aug 10, 2017

weiwei04 commented Aug 14, 2017

weiwei04 commented Aug 14, 2017

weiwei04 commented Aug 17, 2017

weiwei04 commented Aug 25, 2017

k8s-ci-robot commented Aug 25, 2017

weiwei04 commented Aug 29, 2017

soltysh commented Aug 29, 2017

soltysh left a comment

Choose a reason for hiding this comment

soltysh commented Aug 29, 2017

soltysh commented Aug 29, 2017

soltysh commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

weiwei04 commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

weiwei04 commented Jul 4, 2017 •

edited

Loading

weiwei04 commented Jul 4, 2017 •

edited

Loading