Merge pull request #312 from jottofar/cvo-bug-1794823

Bug 1794823: lib/resourcemerge: Do not attempt to change Job's immutable spec.selector

Author: openshift-merge-robot

docs/user/reconciliation.md

@@ -164,3 +164,10 @@ For subsequent updates, the builder blocks until:
 ### Job
 
 After pushing the merged Job into the cluster, the builder blocks until the Job succeeds.
+
+The cluster-version operator will panic if spec.selector is set because there are no clear use-cases for setting it in release manifests.
+
+Subsequent updates:
+
+* The cluster-version operator is currently unable to delete and recreate a Job to track changes in release manifests. Please avoid making changes to Job manifests until the cluster-version operator supports Job delete/recreate.
+* A Job's spec.selector will never be updated because spec.selector is immutable.

lib/resourcemerge/batch.go

@@ -2,22 +2,16 @@ package resourcemerge
 
 import (
 	batchv1 "k8s.io/api/batch/v1"
-	"k8s.io/apimachinery/pkg/api/equality"
 )
 
 // EnsureJob ensures that the existing matches the required.
 // modified is set to true when existing had to be updated with required.
 func EnsureJob(modified *bool, existing *batchv1.Job, required batchv1.Job) {
-	EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
 
-	if existing.Spec.Selector == nil {
-		*modified = true
-		existing.Spec.Selector = required.Spec.Selector
-	}
-	if !equality.Semantic.DeepEqual(existing.Spec.Selector, required.Spec.Selector) {
-		*modified = true
-		existing.Spec.Selector = required.Spec.Selector
+	if required.Spec.Selector != nil {
+		panic("spec.selector is not supported in Job manifests")
 	}
+	EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
 	setInt32Ptr(modified, &existing.Spec.Parallelism, required.Spec.Parallelism)
 	setInt32Ptr(modified, &existing.Spec.Completions, required.Spec.Completions)
 	setInt64Ptr(modified, &existing.Spec.ActiveDeadlineSeconds, required.Spec.ActiveDeadlineSeconds)

lib/resourcemerge/batch_test.go

@@ -0,0 +1,91 @@
+package resourcemerge
+
+import (
+	"testing"
+
+	batchv1 "k8s.io/api/batch/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	pointer "k8s.io/utils/pointer"
+)
+
+func TestEnsureJob(t *testing.T) {
+	labelSelector := metav1.LabelSelector{}
+	tests := []struct {
+		name     string
+		existing batchv1.Job
+		required batchv1.Job
+
+		expectedModified bool
+		expectedPanic    bool
+		expected         batchv1.Job
+	}{
+		{
+			name: "different backofflimit count",
+			existing: batchv1.Job{
+				Spec: batchv1.JobSpec{
+					BackoffLimit: pointer.Int32Ptr(2)}},
+			required: batchv1.Job{
+				Spec: batchv1.JobSpec{
+					BackoffLimit: pointer.Int32Ptr(3)}},
+
+			expectedModified: true,
+			expected: batchv1.Job{
+				Spec: batchv1.JobSpec{
+					BackoffLimit: pointer.Int32Ptr(3)}},
+		},
+		{
+			name: "same backofflimit count",
+			existing: batchv1.Job{
+				Spec: batchv1.JobSpec{
+					BackoffLimit: pointer.Int32Ptr(2)}},
+			required: batchv1.Job{
+				Spec: batchv1.JobSpec{
+					BackoffLimit: pointer.Int32Ptr(2)}},
+
+			expectedModified: false,
+			expected: batchv1.Job{
+				Spec: batchv1.JobSpec{
+					BackoffLimit: pointer.Int32Ptr(2)}},
+		},
+		{
+			name: "required-Selector not nil",
+			existing: batchv1.Job{
+				Spec: batchv1.JobSpec{
+					Selector:       &labelSelector,
+					ManualSelector: pointer.BoolPtr(false)}},
+			required: batchv1.Job{
+				Spec: batchv1.JobSpec{
+					Selector:       &labelSelector,
+					ManualSelector: pointer.BoolPtr(true)}},
+
+			expectedPanic: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			defer func() {
+				switch r := recover(); r {
+				case nil:
+					if test.expectedPanic {
+						t.Errorf(test.name + " should have panicked!")
+					}
+				default:
+					if !test.expectedPanic {
+						panic(r)
+					}
+				}
+			}()
+			modified := pointer.BoolPtr(false)
+			EnsureJob(modified, &test.existing, test.required)
+			if *modified != test.expectedModified {
+				t.Errorf("mismatch modified got: %v want: %v", *modified, test.expectedModified)
+			}
+
+			if !equality.Semantic.DeepEqual(test.existing, test.expected) {
+				t.Errorf("mismatch Job got: %v want: %v", test.existing, test.expected)
+			}
+		})
+	}
+}