OCPBUGS-64582: Drop strategy.rollingUpdate and switch strategy.type to Recreate via pre-patch in frr-k8s-statuscleaner deployments on SNO

bindata/network/frr-k8s/node-status-cleaner.yaml

@@ -8,10 +8,19 @@ metadata:
     component: frr-k8s-statuscleaner
   annotations:
     release.openshift.io/version: "{{.ReleaseVersion}}"
+{{- if .IsSNO }}
+    networkoperator.openshift.io/pre-patch: '{"spec":{"strategy":{"type":"Recreate","rollingUpdate":null}}}'
+{{- end }}
 spec:
 {{- if .IsSNO }}
   strategy:
     type: Recreate
+{{- else }}
+  strategy:
+    type: RollingUpdate
+    rollingUpdate:
+      maxSurge: 25%
+      maxUnavailable: 25%
 {{- end }}
   selector:
     matchLabels:

pkg/apply/apply.go

@@ -108,6 +108,23 @@ func ApplyObject(ctx context.Context, client cnoclient.Client, obj Object, subco
 		fieldManager = fmt.Sprintf("%s/%s", fieldManager, subcontroller)
 	}
 
+	// If pre-patch is specified, apply it as a strategic-merge-patch to the live
+	// object before SSA. This handles cases where SSA cannot remove a field it
+	// does not own (e.g. defaulted rollingUpdate when switching to Recreate).
+	// The pre-patch is silently skipped if the object does not exist yet, making
+	// this mainly relevant on upgrades where defaulted fields must be removed.
+	if prePatch, ok := obj.GetAnnotations()[names.PrePatchAnnotation]; ok {
+		log.Printf("Object %s has pre-patch annotation, attempting strategic-merge-patch before SSA", objDesc)
+		patchOptions := metav1.PatchOptions{FieldManager: fieldManager}
+		_, err := clusterClient.Dynamic().Resource(rm.Resource).Namespace(namespace).Patch(
+			ctx, name, types.StrategicMergePatchType, []byte(prePatch), patchOptions)
+		if apierrors.IsNotFound(err) {
+			log.Printf("Object %s not found, skipping pre-patch", objDesc)
+		} else if err != nil {
+			return fmt.Errorf("failed to pre-patch %s: %w", objDesc, err)
+		}
+	}
+
 	// Use server-side apply to merge the desired object with the object on disk
 	patchOptions := metav1.PatchOptions{
 		// It is considered best-practice for controllers to force

pkg/apply/apply_test.go

@@ -0,0 +1,131 @@
+package apply
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/openshift/cluster-network-operator/pkg/client/fake"
+	"github.com/openshift/cluster-network-operator/pkg/names"
+
+	. "github.com/onsi/gomega"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
+	fakedynamic "k8s.io/client-go/dynamic/fake"
+	clienttesting "k8s.io/client-go/testing"
+)
+
+const prePatchValue = `{"spec":{"strategy":{"type":"Recreate","rollingUpdate":null}}}`
+
+func newTestDeployment(annotations map[string]string) *unstructured.Unstructured {
+	obj := &unstructured.Unstructured{}
+	obj.SetGroupVersionKind(schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"})
+	obj.SetName("test-deployment")
+	obj.SetNamespace("test-ns")
+	obj.SetAnnotations(annotations)
+	return obj
+}
+
+func patchRecorder(patchTypes *[]types.PatchType, prePatchBody *string, prePatchErr error) func(action clienttesting.Action) (bool, runtime.Object, error) {
+	return func(action clienttesting.Action) (bool, runtime.Object, error) {
+		patchAction := action.(clienttesting.PatchAction)
+		*patchTypes = append(*patchTypes, patchAction.GetPatchType())
+		if patchAction.GetPatchType() == types.StrategicMergePatchType {
+			*prePatchBody = string(patchAction.GetPatch())
+			if prePatchErr != nil {
+				return true, nil, prePatchErr
+			}
+		}
+		return true, newTestDeployment(nil), nil
+	}
+}
+
+func TestApplyObjectPrePatchRunsBeforeSSA(t *testing.T) {
+	g := NewWithT(t)
+
+	client := fake.NewFakeClient()
+
+	var patchTypes []types.PatchType
+	var prePatchBody string
+	pr := patchRecorder(&patchTypes, &prePatchBody, nil)
+	client.Default().Dynamic().(*fakedynamic.FakeDynamicClient).PrependReactor("patch", "deployments", pr)
+
+	obj := newTestDeployment(map[string]string{
+		names.PrePatchAnnotation: prePatchValue,
+	})
+
+	err := ApplyObject(context.Background(), client, obj, "test-controller")
+	g.Expect(err).To(Succeed())
+	g.Expect(patchTypes).To(HaveLen(2))
+	g.Expect(patchTypes[0]).To(Equal(types.StrategicMergePatchType), "pre-patch should be strategic-merge-patch")
+	g.Expect(patchTypes[1]).To(Equal(types.ApplyPatchType), "second patch should be SSA apply")
+	g.Expect(prePatchBody).To(Equal(prePatchValue), "pre-patch body should match annotation value")
+}
+
+func TestApplyObjectPrePatchNotFoundAllowsSSA(t *testing.T) {
+	g := NewWithT(t)
+
+	client := fake.NewFakeClient()
+
+	var patchTypes []types.PatchType
+	var prePatchBody string
+	pr := patchRecorder(&patchTypes, &prePatchBody, apierrors.NewNotFound(
+		schema.GroupResource{Group: "apps", Resource: "deployments"}, "test-deployment"))
+	client.Default().Dynamic().(*fakedynamic.FakeDynamicClient).PrependReactor("patch", "deployments", pr)
+
+	obj := newTestDeployment(map[string]string{
+		names.PrePatchAnnotation: prePatchValue,
+	})
+
+	err := ApplyObject(context.Background(), client, obj, "test-controller")
+	g.Expect(err).To(Succeed())
+	g.Expect(patchTypes).To(HaveLen(2))
+	g.Expect(patchTypes[0]).To(Equal(types.StrategicMergePatchType), "pre-patch was attempted")
+	g.Expect(patchTypes[1]).To(Equal(types.ApplyPatchType), "SSA apply still proceeded")
+	g.Expect(prePatchBody).To(Equal(prePatchValue), "pre-patch body should match annotation value")
+}
+
+func TestApplyObjectPrePatchErrorStopsReconciliation(t *testing.T) {
+	g := NewWithT(t)
+
+	client := fake.NewFakeClient()
+
+	var patchTypes []types.PatchType
+	var prePatchBody string
+	pr := patchRecorder(&patchTypes, &prePatchBody, fmt.Errorf("API server error"))
+	client.Default().Dynamic().(*fakedynamic.FakeDynamicClient).PrependReactor("patch", "deployments", pr)
+
+	obj := newTestDeployment(map[string]string{
+		names.PrePatchAnnotation: prePatchValue,
+	})
+
+	err := ApplyObject(context.Background(), client, obj, "test-controller")
+	g.Expect(err).To(HaveOccurred())
+	g.Expect(err.Error()).To(ContainSubstring("failed to pre-patch"))
+	g.Expect(patchTypes).To(HaveLen(1), "SSA apply should not have been reached")
+	g.Expect(patchTypes[0]).To(Equal(types.StrategicMergePatchType), "pre-patch should be strategic-merge-patch")
+	g.Expect(prePatchBody).To(Equal(prePatchValue), "pre-patch body should match annotation value")
+}
+
+func TestApplyObjectNoPrePatchAnnotationSkipsPrePatch(t *testing.T) {
+	g := NewWithT(t)
+
+	client := fake.NewFakeClient()
+
+	var patchTypes []types.PatchType
+	var prePatchBody string
+	pr := patchRecorder(&patchTypes, &prePatchBody, nil)
+	client.Default().Dynamic().(*fakedynamic.FakeDynamicClient).PrependReactor("patch", "deployments", pr)
+
+	obj := newTestDeployment(nil)
+
+	err := ApplyObject(context.Background(), client, obj, "test-controller")
+	g.Expect(err).To(Succeed())
+	g.Expect(patchTypes).To(HaveLen(1))
+	g.Expect(patchTypes[0]).To(Equal(types.ApplyPatchType), "only SSA apply should have run")
+	g.Expect(prePatchBody).To(BeEmpty(), "no pre-patch should have been sent")
+}

pkg/client/fake/fake_client.go

@@ -5,9 +5,9 @@ import (
 	"strings"
 
 	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/api/meta/testrestmapper"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/dynamic"
 	fakedynamic "k8s.io/client-go/dynamic/fake"
 	"k8s.io/client-go/kubernetes"
@@ -125,42 +125,6 @@ func NewFakeClient(objs ...crclient.Object) cnoclient.Client {
 	}
 }
 
-type fakeRESTMapper struct {
-	kindForInput schema.GroupVersionResource
-}
-
-func (f *fakeRESTMapper) KindFor(resource schema.GroupVersionResource) (schema.GroupVersionKind, error) {
-	f.kindForInput = resource
-	return schema.GroupVersionKind{
-		Group:   "test",
-		Version: "test",
-		Kind:    "test"}, nil
-}
-
-func (f *fakeRESTMapper) KindsFor(resource schema.GroupVersionResource) ([]schema.GroupVersionKind, error) {
-	return nil, nil
-}
-
-func (f *fakeRESTMapper) ResourceFor(input schema.GroupVersionResource) (schema.GroupVersionResource, error) {
-	return schema.GroupVersionResource{}, nil
-}
-
-func (f *fakeRESTMapper) ResourcesFor(input schema.GroupVersionResource) ([]schema.GroupVersionResource, error) {
-	return nil, nil
-}
-
-func (f *fakeRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (*meta.RESTMapping, error) {
-	return nil, nil
-}
-
-func (f *fakeRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) ([]*meta.RESTMapping, error) {
-	return nil, nil
-}
-
-func (f *fakeRESTMapper) ResourceSingularizer(resource string) (singular string, err error) {
-	return "", nil
-}
-
 func (fc *FakeClusterClient) Kubernetes() kubernetes.Interface {
 	return fc.kClient
 }
@@ -182,11 +146,11 @@ func (fc *FakeClusterClient) CRClient() crclient.Client {
 }
 
 func (fc *FakeClusterClient) RESTMapper() meta.RESTMapper {
-	return &fakeRESTMapper{}
+	return testrestmapper.TestOnlyStaticRESTMapper(scheme.Scheme)
 }
 
 func (fc *FakeClusterClient) Scheme() *runtime.Scheme {
-	panic("not implemented!")
+	return scheme.Scheme
 }
 func (fc *FakeClusterClient) OperatorHelperClient() operatorv1helpers.OperatorClient {
 	panic("not implemented!")

pkg/names/names.go

@@ -48,6 +48,12 @@ const CreateOnlyAnnotation = "networkoperator.openshift.io/create-only"
 // tells the CNO reconciliation engine to ignore creating this object until conditions are met.
 const CreateWaitAnnotation = "networkoperator.openshift.io/create-wait"
 
+// PrePatchAnnotation is an annotation whose value is a JSON object applied as a
+// strategic-merge-patch to the live object before the SSA apply. This is needed
+// when SSA cannot remove a field it does not own (e.g. removing defaulted
+// rollingUpdate when switching a Deployment strategy to Recreate).
+const PrePatchAnnotation = "networkoperator.openshift.io/pre-patch"
+
 // NonCriticalAnnotation is an annotation on Deployments/DaemonSets to indicate
 // that they are not critical to the functioning of the pod network
 const NonCriticalAnnotation = "networkoperator.openshift.io/non-critical"

pkg/network/render_test.go

@@ -21,6 +21,7 @@ import (
 	apifeatures "github.com/openshift/api/features"
 	operv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-network-operator/pkg/bootstrap"
+	"github.com/openshift/cluster-network-operator/pkg/names"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -702,12 +703,19 @@ func Test_renderFRRStatusCleanerStrategy(t *testing.T) {
 		g := NewWithT(t)
 		d := render(1)
 		g.Expect(d.Spec.Strategy.Type).To(Equal(appsv1.RecreateDeploymentStrategyType))
+		g.Expect(d.Annotations).To(
+			HaveKeyWithValue(
+				names.PrePatchAnnotation,
+				`{"spec":{"strategy":{"type":"Recreate","rollingUpdate":null}}}`,
+			),
+		)
 	})
 
-	t.Run("HA: no strategy override", func(t *testing.T) {
+	t.Run("HA: strategy is RollingUpdate", func(t *testing.T) {
 		g := NewWithT(t)
 		d := render(3)
-		g.Expect(d.Spec.Strategy.Type).To(BeEmpty())
+		g.Expect(d.Spec.Strategy.Type).To(Equal(appsv1.RollingUpdateDeploymentStrategyType))
+		g.Expect(d.Annotations).NotTo(HaveKey(names.PrePatchAnnotation))
 	})
 }