Job: Respect ControllerRef #42176

enisoc · 2017-02-27T18:15:07Z

What this PR does / why we need it:

This is part of the completion of the ControllerRef proposal. It brings Job into full compliance with ControllerRef. See the individual commit messages for details.

Which issue this PR fixes:

This ensures that Job does not fight with other controllers over control of Pods.

Ref: #24433

Special notes for your reviewer:

Release note:

Job controller now respects ControllerRef to avoid fighting over Pods.

cc @erictune @kubernetes/sig-apps-pr-reviews

k8s-reviewable · 2017-02-27T18:15:19Z

This change is

soltysh

A nit, but mostly LGTM.

soltysh · 2017-02-27T20:44:45Z

pkg/controller/job/jobcontroller_test.go

+		}
+		// Make sure the ControllerRefs are correct.
+		for _, controllerRef := range fakePodControl.ControllerRefs {
+			if got, want := controllerRef.APIVersion, "batch/v1"; got != want {


Can you verify the entire ControllerRef object, iow. including the Name and UID?

kow3ns · 2017-02-27T22:13:17Z

Reviewed 31 of 31 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

pkg/controller/job/jobcontroller.go, line 51 at r1 (raw file):

// ControllerKind contains the schema.GroupVersionKind for this controller type.
var ControllerKind = batch.SchemeGroupVersion.WithKind("Job")

Probably should be package private

pkg/controller/job/jobcontroller.go, line 149 at r1 (raw file):

func (jm *JobController) getPodJobs(pod *v1.Pod) []*batch.Job {
	jobs, err := jm.jobLister.GetPodJobs(pod)
	if err != nil {

This is already here, but you have been removing th

Comments from Reviewable

enisoc · 2017-03-03T23:45:37Z

@k8s-bot non-cri e2e test this

enisoc · 2017-03-06T20:10:08Z

@k8s-bot non-cri e2e test this

enisoc · 2017-03-06T21:36:00Z

@soltysh Please take a look. I believe all comments are addressed.

enisoc · 2017-04-10T16:49:22Z

I've removed LGTM because I have some changes planned to bring this up to date with the other ControllerRef PRs that moved forward during the 1.6 cycle. I'll request re-review once I've pushed those changes.

enisoc · 2017-04-14T17:53:04Z

This is ready for final re-review.

I've added 3 commits to fix the equivalent of #42938 and #43034. I made no changes to the previous commits other than rebasing on master.

Now that Job adds ControllerRef to Pods it creates, we need to set this default so legacy behavior is maintained.

janetkuo

LGTM in general

janetkuo · 2017-04-18T23:57:53Z

pkg/controller/job/jobcontroller.go

 		jobKey, err := controller.KeyFunc(job)
 		if err != nil {
-			utilruntime.HandleError(fmt.Errorf("Couldn't get key for job %#v: %v", job, err))


Why remove this?

In a comment on another ControllerRef PR, the reviewer asked me to remove this type of logging during watch handlers.

janetkuo · 2017-04-19T00:06:50Z

pkg/controller/job/jobcontroller_test.go

@@ -598,7 +599,8 @@ func TestJobPodLookup(t *testing.T) {
 	}
 	for _, tc := range testCases {
 		sharedInformerFactory.Batch().V1().Jobs().Informer().GetIndexer().Add(tc.job)
-		if job := manager.getPodJob(tc.pod); job != nil {
+		if jobs := manager.getPodJobs(tc.pod); len(jobs) > 0 {
+			job := jobs[0]


Also make sure len(jobs)==1

This is part of the completion of ControllerRef, as described here: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/controller-ref.md#watches

The Job Listers still use selectors, because this is the behavior expected by callers. This clarifies the meaning of the returned list. Some callers may need to switch to using GetControllerOf() instead, but that is a separate, case-by-case issue.

Now that the default delete option for Job is OrphanDependents, Job deletion is asynchronous.

This is necessary to avoid racing with the GC when it orphans dependents.

This is needed to update ControllerRef during adopt/release.

An e2e test is currently the only way to ensure we have the correct RBAC permissions to PATCH Pods.

janetkuo

LGTM

k8s-github-robot · 2017-04-20T19:57:04Z

Automatic merge from submit-queue (batch tested with PRs 42177, 42176, 44721)

enisoc assigned janetkuo and kow3ns Feb 27, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 27, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 27, 2017

enisoc added this to the v1.6 milestone Feb 27, 2017

enisoc force-pushed the controller-ref-job branch from 51aa4f9 to 38c4f5d Compare February 27, 2017 18:30

0xmichalis assigned soltysh Feb 27, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 27, 2017

soltysh approved these changes Feb 27, 2017

View reviewed changes

enisoc force-pushed the controller-ref-job branch from 38c4f5d to db98286 Compare February 28, 2017 01:00

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

janetkuo assigned caesarxuchao Mar 1, 2017

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

enisoc added the kind/bug Categorizes issue or PR as related to a bug. label Mar 1, 2017

enisoc force-pushed the controller-ref-job branch from db98286 to 9e87ecf Compare March 2, 2017 19:02

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 2, 2017

enisoc force-pushed the controller-ref-job branch from 9e87ecf to 9b724c9 Compare March 3, 2017 22:15

0xmichalis mentioned this pull request Mar 6, 2017

Implement ScheduledJob Reaper #31553

Closed

enisoc force-pushed the controller-ref-job branch from 9b724c9 to 37f804b Compare March 6, 2017 19:13

enisoc removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2017

enisoc force-pushed the controller-ref-job branch 3 times, most recently from 37c0e26 to 3eed987 Compare April 13, 2017 22:41

enisoc removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Apr 14, 2017

enisoc added 3 commits April 18, 2017 13:56

Job: Add ControllerRef on all created Pods.

bdfe18f

Job: Set DefaultGarbageCollectionPolicy to OrphanDependents.

bc423ac

Now that Job adds ControllerRef to Pods it creates, we need to set this default so legacy behavior is maintained.

Job: Use ControllerRefManager to adopt/orphan.

424de52

enisoc force-pushed the controller-ref-job branch from 3eed987 to a307588 Compare April 18, 2017 20:59

janetkuo reviewed Apr 19, 2017

View reviewed changes

enisoc added 9 commits April 19, 2017 14:03

Job: Use ControllerRef to route watch events.

067a8ff

This is part of the completion of ControllerRef, as described here: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/controller-ref.md#watches

Job: Always set BlockOwnerDeletion in ControllerRef.

c82b537

Job: Fix CronJob e2e test for async Job deletion.

e207f6c

Now that the default delete option for Job is OrphanDependents, Job deletion is asynchronous.

Job: Check that ControllerRef UID matches.

06536cb

Job: Check ControllerRef Name and UID in unit test.

fd8dd26

Job: Recheck DeletionTimestamp before adoption.

f502ab6

This is necessary to avoid racing with the GC when it orphans dependents.

Job: Add PATCH Pods permission.

aa7bc25

This is needed to update ControllerRef during adopt/release.

Job: Add e2e test for Pod adopt/release.

d5b86bb

An e2e test is currently the only way to ensure we have the correct RBAC permissions to PATCH Pods.

enisoc force-pushed the controller-ref-job branch from a307588 to d5b86bb Compare April 19, 2017 21:03

janetkuo approved these changes Apr 19, 2017

View reviewed changes

enisoc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2017

k8s-github-robot merged commit 7b43f92 into kubernetes:master Apr 20, 2017

enisoc deleted the controller-ref-job branch April 21, 2017 19:19

enisoc mentioned this pull request Apr 24, 2017

Implement controllerRef #24946

Closed

soltysh mentioned this pull request Jun 22, 2017

Deleting jobs via the API leaves pods #20902

Closed

jsravn mentioned this pull request Jun 28, 2017

Cannot delete jobs when their associated pods are gone #43168

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Job: Respect ControllerRef #42176

Job: Respect ControllerRef #42176

enisoc commented Feb 27, 2017 •

edited

Loading

k8s-reviewable commented Feb 27, 2017

soltysh left a comment

soltysh Feb 27, 2017

kow3ns commented Feb 27, 2017

enisoc commented Mar 3, 2017

enisoc commented Mar 6, 2017

enisoc commented Mar 6, 2017

enisoc commented Apr 10, 2017 •

edited

Loading

enisoc commented Apr 14, 2017

janetkuo left a comment

janetkuo Apr 18, 2017

enisoc Apr 19, 2017

janetkuo Apr 19, 2017

enisoc Apr 19, 2017

janetkuo left a comment

k8s-github-robot commented Apr 20, 2017

Job: Respect ControllerRef #42176

Job: Respect ControllerRef #42176

Conversation

enisoc commented Feb 27, 2017 • edited Loading

k8s-reviewable commented Feb 27, 2017

soltysh left a comment

Choose a reason for hiding this comment

soltysh Feb 27, 2017

Choose a reason for hiding this comment

kow3ns commented Feb 27, 2017

enisoc commented Mar 3, 2017

enisoc commented Mar 6, 2017

enisoc commented Mar 6, 2017

enisoc commented Apr 10, 2017 • edited Loading

enisoc commented Apr 14, 2017

janetkuo left a comment

Choose a reason for hiding this comment

janetkuo Apr 18, 2017

Choose a reason for hiding this comment

enisoc Apr 19, 2017

Choose a reason for hiding this comment

janetkuo Apr 19, 2017

Choose a reason for hiding this comment

enisoc Apr 19, 2017

Choose a reason for hiding this comment

janetkuo left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Apr 20, 2017

enisoc commented Feb 27, 2017 •

edited

Loading

enisoc commented Apr 10, 2017 •

edited

Loading