diff --git a/pkg/controller/cronjob/cronjob_controller.go b/pkg/controller/cronjob/cronjob_controller.go index 5657498d61d..03b7d0c6abb 100644 --- a/pkg/controller/cronjob/cronjob_controller.go +++ b/pkg/controller/cronjob/cronjob_controller.go @@ -250,12 +250,6 @@ func syncOne(sj *batchv2alpha1.CronJob, js []batchv1.Job, now time.Time, jc jobC return } - if err := adoptJobs(sj, js, jc); err != nil { - // This is fine. We will retry later. - // Adoption is only to advise other controllers. We don't rely on it. - glog.V(4).Infof("Unable to adopt Jobs for CronJob %v: %v", nameForLog, err) - } - if sj.Spec.Suspend != nil && *sj.Spec.Suspend { glog.V(4).Infof("Not starting job for %s because it is suspended", nameForLog) return diff --git a/pkg/controller/cronjob/utils.go b/pkg/controller/cronjob/utils.go index 08c3dd2ab9c..b283cb553e4 100644 --- a/pkg/controller/cronjob/utils.go +++ b/pkg/controller/cronjob/utils.go @@ -17,7 +17,6 @@ limitations under the License. package cronjob import ( - "encoding/json" "fmt" "time" @@ -31,7 +30,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" - utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/v1/ref" "k8s.io/kubernetes/pkg/controller" @@ -63,28 +61,18 @@ func deleteFromActiveList(sj *batchv2alpha1.CronJob, uid types.UID) { // getParentUIDFromJob extracts UID of job's parent and whether it was found func getParentUIDFromJob(j batchv1.Job) (types.UID, bool) { - creatorRefJson, found := j.ObjectMeta.Annotations[v1.CreatedByAnnotation] - if !found { - glog.V(4).Infof("Job with no created-by annotation, name %s namespace %s", j.Name, j.Namespace) - return types.UID(""), false - } - var sr v1.SerializedReference - err := json.Unmarshal([]byte(creatorRefJson), &sr) - if err != nil { - glog.V(4).Infof("Job with unparsable created-by annotation, name %s namespace %s: %v", j.Name, j.Namespace, err) - return types.UID(""), false - } - if sr.Reference.Kind != "CronJob" { - glog.V(4).Infof("Job with non-CronJob parent, name %s namespace %s", j.Name, j.Namespace) - return types.UID(""), false - } - // Don't believe a job that claims to have a parent in a different namespace. - if sr.Reference.Namespace != j.Namespace { - glog.V(4).Infof("Alleged scheduledJob parent in different namespace (%s) from Job name %s namespace %s", sr.Reference.Namespace, j.Name, j.Namespace) + controllerRef := controller.GetControllerOf(&j) + + if controllerRef == nil { return types.UID(""), false } - return sr.Reference.UID, true + if controllerRef.Kind != "CronJob" { + glog.V(4).Infof("Job with non-CronJob parent, name %s namespace %s", j.Name, j.Namespace) + return types.UID(""), false + } + + return controllerRef.UID, true } // groupJobsByParent groups jobs into a map keyed by the job parent UID (e.g. scheduledJob). @@ -283,41 +271,3 @@ func (o byJobStartTime) Less(i, j int) bool { return (*o[i].Status.StartTime).Before(*o[j].Status.StartTime) } - -// adoptJobs applies missing ControllerRefs to Jobs created by a CronJob. -// -// This should only happen if the Jobs were created by an older version of the -// CronJob controller, since from now on we add ControllerRef upon creation. -// -// CronJob doesn't do actual adoption because it doesn't use label selectors to -// find its Jobs. However, we should apply ControllerRef for potential -// server-side cascading deletion, and to advise other controllers we own these -// objects. -func adoptJobs(sj *batchv2alpha1.CronJob, js []batchv1.Job, jc jobControlInterface) error { - var errs []error - controllerRef := newControllerRef(sj) - controllerRefJSON, err := json.Marshal(controllerRef) - if err != nil { - return fmt.Errorf("can't adopt Jobs: failed to marshal ControllerRef %#v: %v", controllerRef, err) - } - - for i := range js { - job := &js[i] - controllerRef := controller.GetControllerOf(job) - if controllerRef != nil { - continue - } - controllerRefPatch := fmt.Sprintf(`{"metadata":{"ownerReferences":[%s],"uid":"%s"}}`, - controllerRefJSON, job.UID) - updatedJob, err := jc.PatchJob(job.Namespace, job.Name, types.StrategicMergePatchType, []byte(controllerRefPatch)) - if err != nil { - // If there's a ResourceVersion or other error, don't bother retrying. - // We will just try again on a subsequent CronJob sync. - errs = append(errs, err) - continue - } - // Save it back to the array for later consumers. - js[i] = *updatedJob - } - return utilerrors.NewAggregate(errs) -} diff --git a/pkg/controller/cronjob/utils_test.go b/pkg/controller/cronjob/utils_test.go index 5f9ffe356ff..eb2e12b510e 100644 --- a/pkg/controller/cronjob/utils_test.go +++ b/pkg/controller/cronjob/utils_test.go @@ -17,8 +17,6 @@ limitations under the License. package cronjob import ( - "encoding/json" - "reflect" "strings" "testing" "time" @@ -28,9 +26,10 @@ import ( "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "k8s.io/kubernetes/pkg/controller" ) +func boolptr(b bool) *bool { return &b } + func TestGetJobFromTemplate(t *testing.T) { // getJobFromTemplate() needs to take the job template and copy the labels and annotations // and other fields, and add a created-by reference. @@ -132,7 +131,7 @@ func TestGetParentUIDFromJob(t *testing.T) { }, } { - // Case 1: No UID annotation + // Case 1: No ControllerRef _, found := getParentUIDFromJob(*j) if found { @@ -140,8 +139,14 @@ func TestGetParentUIDFromJob(t *testing.T) { } } { - // Case 2: Has UID annotation - j.ObjectMeta.Annotations = map[string]string{v1.CreatedByAnnotation: `{"kind":"SerializedReference","apiVersion":"v1","reference":{"kind":"CronJob","namespace":"default","name":"pi","uid":"5ef034e0-1890-11e6-8935-42010af0003e","apiVersion":"extensions","resourceVersion":"427339"}}`} + // Case 2: Has ControllerRef + j.ObjectMeta.SetOwnerReferences([]metav1.OwnerReference{ + { + Kind: "CronJob", + UID: types.UID("5ef034e0-1890-11e6-8935-42010af0003e"), + Controller: boolptr(true), + }, + }) expectedUID := types.UID("5ef034e0-1890-11e6-8935-42010af0003e") @@ -159,10 +164,24 @@ func TestGroupJobsByParent(t *testing.T) { uid1 := types.UID("11111111-1111-1111-1111-111111111111") uid2 := types.UID("22222222-2222-2222-2222-222222222222") uid3 := types.UID("33333333-3333-3333-3333-333333333333") - createdBy1 := map[string]string{v1.CreatedByAnnotation: `{"kind":"SerializedReference","apiVersion":"v1","reference":{"kind":"CronJob","namespace":"x","name":"pi","uid":"11111111-1111-1111-1111-111111111111","apiVersion":"extensions","resourceVersion":"111111"}}`} - createdBy2 := map[string]string{v1.CreatedByAnnotation: `{"kind":"SerializedReference","apiVersion":"v1","reference":{"kind":"CronJob","namespace":"x","name":"pi","uid":"22222222-2222-2222-2222-222222222222","apiVersion":"extensions","resourceVersion":"222222"}}`} - createdBy3 := map[string]string{v1.CreatedByAnnotation: `{"kind":"SerializedReference","apiVersion":"v1","reference":{"kind":"CronJob","namespace":"y","name":"pi","uid":"33333333-3333-3333-3333-333333333333","apiVersion":"extensions","resourceVersion":"333333"}}`} - noCreatedBy := map[string]string{} + + ownerReference1 := metav1.OwnerReference{ + Kind: "CronJob", + UID: uid1, + Controller: boolptr(true), + } + + ownerReference2 := metav1.OwnerReference{ + Kind: "CronJob", + UID: uid2, + Controller: boolptr(true), + } + + ownerReference3 := metav1.OwnerReference{ + Kind: "CronJob", + UID: uid3, + Controller: boolptr(true), + } { // Case 1: There are no jobs and scheduledJobs @@ -176,7 +195,7 @@ func TestGroupJobsByParent(t *testing.T) { { // Case 2: there is one controller with one job it created. js := []batchv1.Job{ - {ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "x", Annotations: createdBy1}}, + {ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "x", OwnerReferences: []metav1.OwnerReference{ownerReference1}}}, } jobsBySj := groupJobsByParent(js) @@ -196,13 +215,13 @@ func TestGroupJobsByParent(t *testing.T) { // Case 3: Two namespaces, one has two jobs from one controller, other has 3 jobs from two controllers. // There are also two jobs with no created-by annotation. js := []batchv1.Job{ - {ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "x", Annotations: createdBy1}}, - {ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "x", Annotations: createdBy2}}, - {ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "x", Annotations: createdBy1}}, - {ObjectMeta: metav1.ObjectMeta{Name: "d", Namespace: "x", Annotations: noCreatedBy}}, - {ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "y", Annotations: createdBy3}}, - {ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "y", Annotations: createdBy3}}, - {ObjectMeta: metav1.ObjectMeta{Name: "d", Namespace: "y", Annotations: noCreatedBy}}, + {ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "x", OwnerReferences: []metav1.OwnerReference{ownerReference1}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "x", OwnerReferences: []metav1.OwnerReference{ownerReference2}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "x", OwnerReferences: []metav1.OwnerReference{ownerReference1}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "d", Namespace: "x", OwnerReferences: []metav1.OwnerReference{}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "y", OwnerReferences: []metav1.OwnerReference{ownerReference3}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "y", OwnerReferences: []metav1.OwnerReference{ownerReference3}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "d", Namespace: "y", OwnerReferences: []metav1.OwnerReference{}}}, } jobsBySj := groupJobsByParent(js) @@ -232,7 +251,6 @@ func TestGroupJobsByParent(t *testing.T) { t.Errorf("Wrong number of items in map") } } - } func TestGetRecentUnmetScheduleTimes(t *testing.T) { @@ -370,34 +388,3 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) { } } } - -func TestAdoptJobs(t *testing.T) { - sj := cronJob() - controllerRef := newControllerRef(&sj) - jc := &fakeJobControl{} - jobs := []batchv1.Job{newJob("uid0"), newJob("uid1")} - jobs[0].OwnerReferences = nil - jobs[0].Name = "job0" - jobs[1].OwnerReferences = []metav1.OwnerReference{*controllerRef} - jobs[1].Name = "job1" - - if err := adoptJobs(&sj, jobs, jc); err != nil { - t.Errorf("adoptJobs() error: %v", err) - } - if got, want := len(jc.PatchJobName), 1; got != want { - t.Fatalf("len(PatchJobName) = %v, want %v", got, want) - } - if got, want := jc.PatchJobName[0], "job0"; got != want { - t.Errorf("PatchJobName = %v, want %v", got, want) - } - if got, want := len(jc.Patches), 1; got != want { - t.Fatalf("len(Patches) = %v, want %v", got, want) - } - patch := &batchv1.Job{} - if err := json.Unmarshal(jc.Patches[0], patch); err != nil { - t.Fatalf("Unmarshal error: %v", err) - } - if got, want := controller.GetControllerOf(patch), controllerRef; !reflect.DeepEqual(got, want) { - t.Errorf("ControllerRef = %#v, want %#v", got, want) - } -} diff --git a/test/e2e/cronjob.go b/test/e2e/cronjob.go index 39f4d0f3cd7..fbf8ee73586 100644 --- a/test/e2e/cronjob.go +++ b/test/e2e/cronjob.go @@ -33,7 +33,6 @@ import ( "k8s.io/kubernetes/pkg/api" batchinternal "k8s.io/kubernetes/pkg/apis/batch" "k8s.io/kubernetes/pkg/client/clientset_generated/clientset" - "k8s.io/kubernetes/pkg/controller" "k8s.io/kubernetes/pkg/controller/job" "k8s.io/kubernetes/pkg/kubectl" "k8s.io/kubernetes/test/e2e/framework" @@ -275,53 +274,6 @@ var _ = framework.KubeDescribe("CronJob", func() { err = deleteCronJob(f.ClientSet, f.Namespace.Name, cronJob.Name) Expect(err).NotTo(HaveOccurred()) }) - - // Adopt Jobs it owns that don't have ControllerRef yet. - // That is, the Jobs were created by a pre-v1.6.0 master. - It("should adopt Jobs it owns that don't have ControllerRef yet", func() { - By("Creating a cronjob") - cronJob := newTestCronJob("adopt", "*/1 * * * ?", batchv2alpha1.ForbidConcurrent, - sleepCommand, nil) - // Replace cronJob with the one returned from Create() so it has the UID. - // Save Kind since it won't be populated in the returned cronJob. - kind := cronJob.Kind - cronJob, err := createCronJob(f.ClientSet, f.Namespace.Name, cronJob) - Expect(err).NotTo(HaveOccurred()) - cronJob.Kind = kind - - By("Ensuring a Job is running") - err = waitForActiveJobs(f.ClientSet, f.Namespace.Name, cronJob.Name, 1) - Expect(err).NotTo(HaveOccurred()) - - By("Orphaning a Job") - jobs, err := f.ClientSet.BatchV1().Jobs(f.Namespace.Name).List(metav1.ListOptions{}) - Expect(err).NotTo(HaveOccurred()) - Expect(jobs.Items).To(HaveLen(1)) - job := jobs.Items[0] - framework.UpdateJobFunc(f.ClientSet, f.Namespace.Name, job.Name, func(job *batchv1.Job) { - job.OwnerReferences = nil - }) - - By("Checking that the CronJob readopts the Job") - Expect(wait.Poll(framework.Poll, cronJobTimeout, func() (bool, error) { - job, err := framework.GetJob(f.ClientSet, f.Namespace.Name, job.Name) - if err != nil { - return false, err - } - controllerRef := controller.GetControllerOf(job) - if controllerRef == nil { - return false, nil - } - if controllerRef.Kind != cronJob.Kind || controllerRef.Name != cronJob.Name || controllerRef.UID != cronJob.UID { - return false, fmt.Errorf("Job has wrong controllerRef: got %v, want %v", controllerRef, cronJob) - } - return true, nil - })).To(Succeed(), "wait for Job %q to be readopted", job.Name) - - By("Removing CronJob") - err = deleteCronJob(f.ClientSet, f.Namespace.Name, cronJob.Name) - Expect(err).NotTo(HaveOccurred()) - }) }) // newTestCronJob returns a cronjob which does one of several testing behaviors.