Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Various fixes to job #14142

Merged
merged 3 commits into from
Sep 18, 2015
Merged

Various fixes to job #14142

merged 3 commits into from
Sep 18, 2015

Conversation

mikedanese
Copy link
Member

No description provided.

@mikedanese mikedanese added this to the v1.1 milestone Sep 17, 2015
@alex-mohr
Copy link
Contributor

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Sep 17, 2015

GCE e2e build/test passed for commit 12257569221a2c79d5860333c8f776a5b4de63e5.

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 17, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/S

@k8s-bot
Copy link

k8s-bot commented Sep 18, 2015

GCE e2e build/test passed for commit 12257569221a2c79d5860333c8f776a5b4de63e5.

@mikedanese mikedanese changed the title only allow updates of parrallelism in jobspec Various fixes to job Sep 18, 2015
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-bot
Copy link

k8s-bot commented Sep 18, 2015

GCE e2e build/test passed for commit 70baa11c0f0d5493347b7a0a8b4e929545a23bf3.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2015
@erictune
Copy link
Member

LGTM

@erictune erictune added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge labels Sep 18, 2015
@erictune
Copy link
Member

needs rebase

@mikedanese mikedanese force-pushed the job-update branch 2 times, most recently from 3af20e4 to e29e606 Compare September 18, 2015 18:50
@mikedanese mikedanese added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-merge labels Sep 18, 2015
@k8s-bot
Copy link

k8s-bot commented Sep 18, 2015

GCE e2e build/test passed for commit e29e606.

@erictune erictune closed this Sep 18, 2015
@erictune erictune reopened this Sep 18, 2015
thockin added a commit that referenced this pull request Sep 18, 2015
@thockin
Merge pull request #14142 from mikedanese/job-update
e07eb38 
Various fixes to job
@thockin thockin merged commit e07eb38 into kubernetes:master Sep 18, 2015
@mikedanese mikedanese deleted the job-update branch September 18, 2015 21:45
soltysh
soltysh reviewed Sep 21, 2015
View reviewed changes
pkg/controller/job/controller.go
@@ -403,10 +398,6 @@ func (jm *JobManager) manageJob(activePods []*api.Pod, successful, unsuccessful
} else if active < parallelism {
// how many executions are left to run
diff := *job.Spec.Completions - successful
// for RestartPolicyNever we need to count unsuccessful pods as well
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this was removed? Without it, the comment in types.go does not make sense, since we'll always update the unsuccessful value. Besides, this PR also change the idea of not restarting failed pods when RestartPolicyNever is set. Currently the RestartPolicy does not change behavior of job controller, is that indented? May I know the reasoning?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How job interacts with restart policy was not part of the initial proposal. We really wanted to get job in but we wanted to discuss this interaction in design review over the next couple months while it was in experimental. Thanks for pointing out that incorrect comment, but with the logic the way it was this comment is incorrect because both successful pods and failed pod will be counted towards my completion.

Please see #14186 which is an umbrella issue for jobcontroller enhancements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll move entire discussion there. Besides thanks for all the fixes you've made in the mean time to the jobs!

@erictune erictune mentioned this pull request Sep 22, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@erictune erictune

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@mikedanese @alex-mohr @k8s-bot @k8s-github-robot @erictune @soltysh @googlebot @thockin