diff --git a/bindata/network/frr-k8s/node-status-cleaner.yaml b/bindata/network/frr-k8s/node-status-cleaner.yaml index c14786c1f8..cdd94396cd 100644 --- a/bindata/network/frr-k8s/node-status-cleaner.yaml +++ b/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: diff --git a/pkg/apply/apply.go b/pkg/apply/apply.go index ee732fc10d..b2ba5762c5 100644 --- a/pkg/apply/apply.go +++ b/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 diff --git a/pkg/apply/apply_test.go b/pkg/apply/apply_test.go new file mode 100644 index 0000000000..e069591583 --- /dev/null +++ b/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") +} diff --git a/pkg/client/fake/fake_client.go b/pkg/client/fake/fake_client.go index 1203333a92..4f165a2cd4 100644 --- a/pkg/client/fake/fake_client.go +++ b/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!") diff --git a/pkg/names/names.go b/pkg/names/names.go index d409253adc..df438403d0 100644 --- a/pkg/names/names.go +++ b/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" diff --git a/pkg/network/render_test.go b/pkg/network/render_test.go index a47a661be8..0e45ec1b92 100644 --- a/pkg/network/render_test.go +++ b/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)) }) }