diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index 368cbacca..19ccd1526 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -39,7 +39,7 @@ func GetResources(operatorConfig config.OperatorConfig) ([]client.Object, error) substitutedObjects := common.SubstituteCommonPartsFromConfig(operatorConfig, renderedObjects) commonResources, err := common.GetCommonResources(operatorConfig) if err != nil { - klog.Errorf("can not create common resources, such as pdb: %v", err) + klog.Errorf("can not create common resources %v", err) return nil, err } substitutedObjects = append(substitutedObjects, commonResources...) diff --git a/pkg/controllers/resourceapply/resourceapply.go b/pkg/controllers/resourceapply/resourceapply.go index b8889078b..8933975cf 100644 --- a/pkg/controllers/resourceapply/resourceapply.go +++ b/pkg/controllers/resourceapply/resourceapply.go @@ -8,6 +8,8 @@ import ( "fmt" "reflect" + "k8s.io/apimachinery/pkg/api/equality" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" policyv1 "k8s.io/api/policy/v1" @@ -287,15 +289,10 @@ func applyDaemonSet(ctx context.Context, client coreclientv1.Client, recorder re func applyPodDisruptionBudget(ctx context.Context, client coreclientv1.Client, recorder record.EventRecorder, requiredOriginal *policyv1.PodDisruptionBudget) (bool, error) { required := requiredOriginal.DeepCopy() - err := SetSpecHashAnnotation(&required.ObjectMeta, required.Spec) - if err != nil { - return false, err - } existing := &policyv1.PodDisruptionBudget{} - err = client.Get(ctx, coreclientv1.ObjectKeyFromObject(required), existing) + err := client.Get(ctx, coreclientv1.ObjectKeyFromObject(required), existing) if apierrors.IsNotFound(err) { - required.Annotations[generationAnnotation] = "1" err = client.Create(ctx, required) if err != nil { recorder.Event(required, corev1.EventTypeWarning, "Create failed", err.Error()) @@ -312,13 +309,10 @@ func applyPodDisruptionBudget(ctx context.Context, client coreclientv1.Client, r modified := resourcemerge.BoolPtr(false) existingCopy := existing.DeepCopy() - expectedGeneration := "" - if _, ok := existingCopy.Annotations[generationAnnotation]; ok { - expectedGeneration = existingCopy.Annotations[generationAnnotation] - } - resourcemerge.EnsureObjectMeta(modified, &existingCopy.ObjectMeta, required.ObjectMeta) - if !*modified && expectedGeneration == fmt.Sprintf("%x", existingCopy.GetGeneration()) { + contentSame := equality.Semantic.DeepEqual(existingCopy.Spec, required.Spec) + + if !*modified && contentSame { return false, nil } @@ -326,8 +320,6 @@ func applyPodDisruptionBudget(ctx context.Context, client coreclientv1.Client, r toWrite := existingCopy // shallow copy so the code reads easier toWrite.Spec = *required.Spec.DeepCopy() - toWrite.Annotations[generationAnnotation] = fmt.Sprintf("%x", existingCopy.GetGeneration()+1) - err = client.Update(ctx, toWrite) if err != nil { recorder.Event(required, corev1.EventTypeWarning, "Update failed", err.Error()) diff --git a/pkg/controllers/resourceapply/resourceapply_test.go b/pkg/controllers/resourceapply/resourceapply_test.go index 3865f7035..4197c5032 100644 --- a/pkg/controllers/resourceapply/resourceapply_test.go +++ b/pkg/controllers/resourceapply/resourceapply_test.go @@ -8,9 +8,11 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" "k8s.io/utils/pointer" @@ -61,6 +63,10 @@ func cleanupResources(t *testing.T, g *WithT, ctx context.Context, cl client.Cli for _, obj := range typedList.Items { deleteResouce(g, &obj) } + case *policyv1.PodDisruptionBudgetList: + for _, obj := range typedList.Items { + deleteResouce(g, &obj) + } default: t.Fatal("can not cast list type for cleanup") } @@ -656,3 +662,95 @@ func workloadDaemonSetWithDefaultSpecHash() *appsv1.DaemonSet { w.Annotations[specHashAnnotation] = "eaeff6ac704fb141d5085803b5b3cc12067ef98c9f2ba8c1052df81faa53299c" return w } + +func TestApplyPDB(t *testing.T) { + cl, tearDownFn := setupEnvtest(t) + defer tearDownFn(t) + + tests := []struct { + name string + existing *policyv1.PodDisruptionBudget + input *policyv1.PodDisruptionBudget + + expectedModified bool + }{ + { + name: "create", + input: podDisruptionBudget(), + expectedModified: true, + }, + { + name: "skip on extra label", + existing: func() *policyv1.PodDisruptionBudget { + pdb := podDisruptionBudget() + pdb.Labels = map[string]string{"bar": "baz"} + return pdb + }(), + input: podDisruptionBudget(), + + expectedModified: false, + }, + { + name: "update on missing label", + existing: func() *policyv1.PodDisruptionBudget { + pdb := podDisruptionBudget() + pdb.Labels = map[string]string{"bar": "baz"} + return pdb + }(), + input: func() *policyv1.PodDisruptionBudget { + pdb := podDisruptionBudget() + pdb.Labels = map[string]string{"new": "merge"} + return pdb + }(), + + expectedModified: true, + }, + { + name: "update on mismatch data", + existing: podDisruptionBudget(), + input: func() *policyv1.PodDisruptionBudget { + pdb := podDisruptionBudget() + minAvailable := intstr.FromInt(3) + pdb.Spec.MinAvailable = &minAvailable + return pdb + }(), + + expectedModified: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + recorder := record.NewFakeRecorder(1000) + ctx := context.TODO() + defer cleanupResources(t, g, ctx, cl, &policyv1.PodDisruptionBudgetList{}) + + if tt.existing != nil { + g.Expect(cl.Create(context.TODO(), tt.existing)).To(Succeed()) + } + actualModified, err := applyPodDisruptionBudget(context.TODO(), cl, recorder, tt.input) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(tt.expectedModified).To(BeEquivalentTo(actualModified), "Resource was modified") + }) + } +} + +func podDisruptionBudget() *policyv1.PodDisruptionBudget { + minAvailable := intstr.FromInt(1) + return &policyv1.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + Kind: "PodDisruptionBudget", + APIVersion: "policy/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "pdbName", + Namespace: "default", + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + MinAvailable: &minAvailable, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo": "bar"}, + }, + }, + } +}