Merge pull request #2044 from jsafrane/csidriver-apply-cleared

STOR-2627: Re-apply CSIDriver after disabled fields were cleared

Author: openshift-merge-bot[bot]

pkg/operator/resource/resourceapply/storage.go

@@ -135,9 +135,10 @@ func storageClassNeedsRecreate(oldSC, newSC *storagev1.StorageClass) bool {
 	return false
 }
 
-// ApplyCSIDriver merges objectmeta, does not worry about anything else
+// ApplyCSIDriver merges objectmeta and tries to update spec if any of the required fields were cleared by the API server.
+// It assumes they were cleared due to a feature gate not enabled in the API server and it will be enabled soon.
+// When used by StaticResourceController, it will retry periodically and eventually save the spec with the field.
 func ApplyCSIDriver(ctx context.Context, client storageclientv1.CSIDriversGetter, recorder events.Recorder, requiredOriginal *storagev1.CSIDriver) (*storagev1.CSIDriver, bool, error) {
-
 	required := requiredOriginal.DeepCopy()
 	if required.Annotations == nil {
 		required.Annotations = map[string]string{}
@@ -173,31 +174,57 @@ func ApplyCSIDriver(ctx context.Context, client storageclientv1.CSIDriversGetter
 		}
 	}
 
-	metadataModified := false
+	needsUpdate := false
+	// Most CSIDriver fields are immutable. Any change to them should trigger Delete() + Create() calls.
+	needsRecreate := false
+
 	existingCopy := existing.DeepCopy()
-	resourcemerge.EnsureObjectMeta(&metadataModified, &existingCopy.ObjectMeta, required.ObjectMeta)
+	// Metadata change should need just Update() call.
+	resourcemerge.EnsureObjectMeta(&needsUpdate, &existingCopy.ObjectMeta, required.ObjectMeta)
 
 	requiredSpecHash := required.Annotations[specHashAnnotation]
 	existingSpecHash := existing.Annotations[specHashAnnotation]
-	sameSpec := requiredSpecHash == existingSpecHash
-	if sameSpec && !metadataModified {
+	// Assume whole re-create is needed on any spec change.
+	// We don't keep a track of which field is mutable.
+	needsRecreate = requiredSpecHash != existingSpecHash
+
+	// TODO: remove when CSIDriver spec.nodeAllocatableUpdatePeriodSeconds is enabled by default
+	// (https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/4876-mutable-csinode-allocatable)
+	if !needsRecreate && !alphaFieldsSaved(existingCopy, required) {
+		// The required spec is the same as in previous succesful call, however,
+		// the API server must have cleared some alpha/beta fields in it.
+		// Try to save the object again. In case the fields are cleared again,
+		// the caller (typically StaticResourceController) must retry periodically.
+		klog.V(4).Infof("Detected CSIDriver %q field cleared by the API server, updating", required.Name)
+
+		// Assumption: the alpha fields are **mutable**, so only Update() is needed.
+		// Update() with the same spec as before + the field cleared by the API server
+		// won't generate any informer events. StaticResourceController will retry with
+		// periodic retry (1 minute.)
+		// We cannot use needsRecreate=true, as it will generate informer events and
+		// StaticResourceController will retry immediately, leading to a busy loop.
+		needsUpdate = true
+		existingCopy.Spec = required.Spec
+	}
+
+	if !needsUpdate && !needsRecreate {
 		return existing, false, nil
 	}
 
 	if klog.V(2).Enabled() {
 		klog.Infof("CSIDriver %q changes: %v", required.Name, JSONPatchNoError(existing, existingCopy))
 	}
 
-	if sameSpec {
-		// Update metadata by a simple Update call
+	if !needsRecreate {
+		// only needsUpdate is true, update the object by a simple Update call
 		actual, err := client.CSIDrivers().Update(ctx, existingCopy, metav1.UpdateOptions{})
 		resourcehelper.ReportUpdateEvent(recorder, required, err)
 		return actual, true, err
 	}
 
+	// needsRecreate is true, needsUpdate does not matter. Delete and re-create the object.
 	existingCopy.Spec = required.Spec
 	existingCopy.ObjectMeta.ResourceVersion = ""
-	// Spec is read-only after creation. Delete and re-create the object
 	err = client.CSIDrivers().Delete(ctx, existingCopy.Name, metav1.DeleteOptions{})
 	resourcehelper.ReportDeleteEvent(recorder, existingCopy, err, "Deleting CSIDriver to re-create it with updated parameters")
 	if err != nil && !apierrors.IsNotFound(err) {
@@ -214,10 +241,17 @@ func ApplyCSIDriver(ctx context.Context, client storageclientv1.CSIDriversGetter
 	} else if err != nil {
 		err = fmt.Errorf("failed to re-create CSIDriver %s: %s", existingCopy.Name, err)
 	}
-	resourcehelper.ReportCreateEvent(recorder, existingCopy, err)
+	resourcehelper.ReportCreateEvent(recorder, actual, err)
 	return actual, true, err
 }
 
+// alphaFieldsSaved checks that all required fields in the CSIDriver required spec are present and equal in the actual spec.
+func alphaFieldsSaved(actual, required *storagev1.CSIDriver) bool {
+	// DeepDerivative checks that all fields in "required" are present and equal in "actual"
+	// Fields not present in "required" are ignored.
+	return equality.Semantic.DeepDerivative(required.Spec, actual.Spec)
+}
+
 func validateRequiredCSIDriverLabels(required *storagev1.CSIDriver) error {
 	supportsEphemeralVolumes := false
 	for _, mode := range required.Spec.VolumeLifecycleModes {

pkg/operator/resource/resourceapply/storage_test.go

@@ -434,6 +434,9 @@ func TestApplyCSIDriver(t *testing.T) {
 		name     string
 		existing []*storagev1.CSIDriver
 		input    *storagev1.CSIDriver
+		// Compute hash of input.Spec and use it as the existing.annotations[specHashAnnotation]
+		// Useful for simulating the API server clearing some alpha/beta fields in existing.Spec.
+		existingHashFromInput bool
 
 		expectedModified bool
 		expectedError    error
@@ -757,13 +760,61 @@ func TestApplyCSIDriver(t *testing.T) {
 				}
 			},
 		},
+		{
+			name: "alphaFieldsSaved: same spec hash but missing alpha field triggers update",
+			existing: []*storagev1.CSIDriver{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "foo",
+					},
+					Spec: storagev1.CSIDriverSpec{
+						AttachRequired: ptr.To(true),
+						PodInfoOnMount: ptr.To(true),
+						// NodeAllocatableUpdatePeriodSeconds is missing (simulating API server cleared it)
+					},
+				},
+			},
+			input: &storagev1.CSIDriver{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: storagev1.CSIDriverSpec{
+					AttachRequired:                     ptr.To(true),
+					PodInfoOnMount:                     ptr.To(true),
+					NodeAllocatableUpdatePeriodSeconds: ptr.To[int64](30),
+				},
+			},
+			existingHashFromInput: true,
+			expectedModified:      true,
+			verifyActions: func(actions []clienttesting.Action, t *testing.T) {
+				if len(actions) != 2 {
+					t.Fatal(spew.Sdump(actions))
+				}
+				if !actions[0].Matches("get", "csidrivers") || actions[0].(clienttesting.GetAction).GetName() != "foo" {
+					t.Error(spew.Sdump(actions))
+				}
+				if !actions[1].Matches("update", "csidrivers") {
+					t.Error(spew.Sdump(actions))
+				}
+				// Verify that the update includes the alpha field
+				actual := actions[1].(clienttesting.UpdateAction).GetObject().(*storagev1.CSIDriver)
+				if actual.Spec.NodeAllocatableUpdatePeriodSeconds == nil || *actual.Spec.NodeAllocatableUpdatePeriodSeconds != 30 {
+					t.Errorf("expected NodeAllocatableUpdatePeriodSeconds to be 30, got %v", actual.Spec.NodeAllocatableUpdatePeriodSeconds)
+				}
+			},
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			objs := make([]runtime.Object, len(test.existing))
 			for i, csiDriver := range test.existing {
 				// Add spec hash annotation
-				SetSpecHashAnnotation(&csiDriver.ObjectMeta, csiDriver.Spec)
+				if test.existingHashFromInput {
+					SetSpecHashAnnotation(&csiDriver.ObjectMeta, test.input.Spec)
+				} else {
+					// Add spec hash annotation based on existing spec
+					SetSpecHashAnnotation(&csiDriver.ObjectMeta, csiDriver.Spec)
+				}
 				// Convert *CSIDriver to *Object
 				objs[i] = csiDriver
 			}