Merge pull request #47469 from crimsonfaith91/created

Automatic merge from submit-queue (batch tested with PRs 47993, 47892, 47591, 47469, 47845)

deprecate created-by annotation for cronjob

**What this PR does / why we need it**: This PR deprecates created-by annotation for cronjob. This is needed as we now have ControllerRef.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: xref #44407

**Special notes for your reviewer**: I will create 3 PRs to fix the issue as the annotation is used in various parts of the codebase: cronjob, pod drain, and e2e test framework. This is the first PR. Other PRs can be found here: #47471, #47475

**Release note**:

```release-note
```

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

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)
-}

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)
-	}
-}

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.