Merge pull request #437 from oracle/pv-fix

Fix StatefulSet controller to never attempt to patch VolumeClaimTemplates

Author: thegridman

Makefile

@@ -73,7 +73,7 @@ PRE_RELEASE      ?= true
 # Extra arguments to pass to the go test command for the various test steps.
 # For example, when running make e2e-test we can run just a single test such
 # as the zone test using the go test -run=regex argument like this
-#   make e2e-test GO_TEST_FLAGS='-run=^TestZone$$'
+#   make e2e-test GO_TEST_FLAGS_E2E='-run=^TestZone$$'
 GO_TEST_FLAGS     ?= -timeout=20m
 GO_TEST_FLAGS_E2E := -timeout=100m
 

pkg/apis/coherence/v1/coherence_types.go

@@ -2332,6 +2332,24 @@ func (in *Resource) IsDelete() bool {
 	return in.Spec == nil
 }
 
+// Convert the Spec to the specified value.
+// This is done by serializing the Spec to json and deserializing into the specified object.
+func (in *Resource) As(o runtime.Object) error {
+	if in == nil {
+		return fmt.Errorf("cannot convert to runtime.Object - resource is nil")
+	}
+	if in.Spec == nil {
+		return fmt.Errorf("cannot convert resource - spec is nil")
+	}
+
+	data, err := json.Marshal(in.Spec)
+	if err != nil {
+		return errors.Wrap(err, "marshalling spec to json")
+	}
+
+	return json.Unmarshal(data, o)
+}
+
 // Set the the controller/owner of the resource
 func (in *Resource) SetController(object runtime.Object, scheme *runtime.Scheme) error {
 	if in == nil || in.Spec == nil {

pkg/controller/reconciler/base_controller.go

@@ -328,7 +328,7 @@ func (in *CommonReconciler) ThreeWayPatchWithCallback(name string, current, orig
 
 // Create a three-way patch between the original state, the current state and the desired state of a k8s resource.
 func (in *CommonReconciler) CreateThreeWayPatch(name string, original, desired, current runtime.Object, ignore ...string) (client.Patch, error) {
-	data, err := in.CreateThreeWayPatchData(name, original, desired, current, ignore...)
+	data, err := in.CreateThreeWayPatchData(original, desired, current, ignore...)
 	if err != nil {
 		return nil, errors.Wrap(err, "creating three-way patch")
 	}
@@ -350,7 +350,7 @@ func (in *CommonReconciler) CreateThreeWayPatch(name string, original, desired,
 }
 
 // Create a three-way patch between the original state, the current state and the desired state of a k8s resource.
-func (in *CommonReconciler) CreateThreeWayPatchData(name string, original, desired, current runtime.Object, ignore ...string) ([]byte, error) {
+func (in *CommonReconciler) CreateThreeWayPatchData(original, desired, current runtime.Object, ignore ...string) ([]byte, error) {
 	originalData, err := json.Marshal(original)
 	if err != nil {
 		return nil, errors.Wrap(err, "serializing original configuration")

pkg/controller/servicemonitor/servicemonitor_controller.go

@@ -186,7 +186,7 @@ func (in *ReconcileServiceMonitor) UpdateServiceMonitor(namespace, name string,
 	// fix the CreationTimestamp so that it is not in the patch
 	desired.Spec.(metav1.Object).SetCreationTimestamp(current.(metav1.Object).GetCreationTimestamp())
 	// create the patch
-	data, err := in.CreateThreeWayPatchData(name, original.Spec, desired.Spec, current)
+	data, err := in.CreateThreeWayPatchData(original.Spec, desired.Spec, current)
 	if err != nil {
 		return errors.Wrapf(err, "failed to create patch for ServiceMonitor %s/%s", namespace, name)
 	}

pkg/controller/statefulset/statefulset_controller.go

@@ -285,16 +285,26 @@ func (in *ReconcileStatefulSet) updateStatefulSet(deployment *coh.Coherence, cur
 // Patch the StatefulSet if required, returning a bool to indicate whether a patch was applied.
 func (in *ReconcileStatefulSet) patchStatefulSet(deployment *coh.Coherence, current, desired *appsv1.StatefulSet, storage utils.Storage) (bool, error) {
 	logger := in.GetLog().WithValues("Namespace", deployment.Namespace, "Name", deployment.Name)
-	name := current.GetName()
-	original, _ := storage.GetPrevious().GetResource(coh.ResourceTypeStatefulSet, name)
+	resource, _ := storage.GetPrevious().GetResource(coh.ResourceTypeStatefulSet, current.GetName())
+	original := &appsv1.StatefulSet{}
+	err := resource.As(original)
+	if err != nil {
+		return false, err
+	}
 
 	// We NEVER change the replicas or Status in an update.
 	// Replicas is handled by scaling so we always set the desired replicas to match the current replicas
 	desired.Spec.Replicas = current.Spec.Replicas
+	original.Spec.Replicas = current.Spec.Replicas
 	// We need to ensure we do not create a patch due to differences in StatefulSet Status
-	desired.Status = current.Status
-	// If the StatefulSet has PVs we need to ensure we do not create a patch due to ignorable differences in PV
-	in.mergeVolumeClaims(desired, current)
+	desired.Status = appsv1.StatefulSetStatus{}
+	current.Status = appsv1.StatefulSetStatus{}
+	original.Status = appsv1.StatefulSetStatus{}
+
+	// The VolumeClaimTemplates of a StetfulSet cannot be changed so blank them out for the patch
+	desired.Spec.VolumeClaimTemplates = []corev1.PersistentVolumeClaim{}
+	current.Spec.VolumeClaimTemplates = []corev1.PersistentVolumeClaim{}
+	original.Spec.VolumeClaimTemplates = []corev1.PersistentVolumeClaim{}
 
 	// a callback function that the 3-way patch method will call just before it applies a patch
 	callback := func() {
@@ -304,7 +314,7 @@ func (in *ReconcileStatefulSet) patchStatefulSet(deployment *coh.Coherence, curr
 		}
 	}
 
-	patched, err := in.ThreeWayPatchWithCallback(name, current, original.Spec, desired, callback)
+	patched, err := in.ThreeWayPatchWithCallback(current.GetName(), current, original, desired, callback)
 	// log the result of patching
 	switch {
 	case err != nil:
@@ -318,37 +328,6 @@ func (in *ReconcileStatefulSet) patchStatefulSet(deployment *coh.Coherence, curr
 	return patched, err
 }
 
-// If the desired StatefulSet has PVs update the desired PV to avid needless merges
-func (in *ReconcileStatefulSet) mergeVolumeClaims(desired, current *appsv1.StatefulSet) {
-	if len(desired.Spec.VolumeClaimTemplates) == 0 {
-		return
-	}
-
-	// Make a map of current PVs by name
-	m := make(map[string]corev1.PersistentVolumeClaim)
-	for _, pv := range current.Spec.VolumeClaimTemplates {
-		m[pv.Name] = pv
-	}
-
-	dfltVolumeMode := corev1.PersistentVolumeFilesystem
-
-	for i, pv := range desired.Spec.VolumeClaimTemplates {
-		currentPV, found := m[pv.Name]
-		if found {
-			// Update the desired PV statuses
-			pv.Status = currentPV.Status
-
-			// Set the desired volume mode to the default if it is not set
-			if pv.Spec.VolumeMode == nil {
-				pv.Spec.VolumeMode = &dfltVolumeMode
-			}
-
-			// set the updated PV back into the desired array
-			desired.Spec.VolumeClaimTemplates[i] = pv
-		}
-	}
-}
-
 // Scale will scale a StatefulSet up or down
 func (in *ReconcileStatefulSet) scale(deployment *coh.Coherence, sts *appsv1.StatefulSet, current, desired int32) (reconcile.Result, error) {
 	// if the StatefulSet is not stable we cannot scale (e.g. it might already be in the middle of a rolling upgrade)