diff --git a/pkg/controllers/workgenerator/controller.go b/pkg/controllers/workgenerator/controller.go index 2c9d367c1..cc3f05315 100644 --- a/pkg/controllers/workgenerator/controller.go +++ b/pkg/controllers/workgenerator/controller.go @@ -619,6 +619,9 @@ func (r *Reconciler) getConfigMapEnvelopWorkObj(ctx context.Context, workNamePre } work := workList.Items[0] work.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] = resourceSnapshot.Labels[fleetv1beta1.ResourceIndexLabel] + if work.Annotations == nil { + work.Annotations = make(map[string]string) + } work.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] = resourceBinding.Spec.ResourceSnapshotName work.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] = resourceOverrideSnapshotHash work.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] = clusterResourceOverrideSnapshotHash @@ -681,23 +684,29 @@ func (r *Reconciler) upsertWork(ctx context.Context, newWork, existingWork *flee // check if we need to update the existing work object workResourceIndex, err := labels.ExtractResourceSnapshotIndexFromWork(existingWork) if err != nil { - klog.ErrorS(err, "work has invalid parent resource index", "work", workObj) - return false, controller.NewUnexpectedBehaviorError(err) - } - // we already checked the label in fetchAllResourceSnapShots function so no need to check again - resourceIndex, _ := labels.ExtractResourceIndexFromClusterResourceSnapshot(resourceSnapshot) - if workResourceIndex == resourceIndex { - // no need to do anything if the work is generated from the same snapshots - if existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] && - existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] { - klog.V(2).InfoS("Work is not associated with the desired override snapshots", "existingROHash", existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation], - "existingCROHash", existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation], "work", workObj) - return false, nil + klog.ErrorS(controller.NewUnexpectedBehaviorError(err), "work has invalid parent resource index", "work", workObj) + } else { + // we already checked the label in fetchAllResourceSnapShots function so no need to check again + resourceIndex, _ := labels.ExtractResourceIndexFromClusterResourceSnapshot(resourceSnapshot) + if workResourceIndex == resourceIndex { + // no need to do anything if the work is generated from the same resource/override snapshots + if existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] && + existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] { + klog.V(2).InfoS("Work is associated with the desired resource/override snapshots", "existingROHash", existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation], + "existingCROHash", existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation], "work", workObj) + return false, nil + } + klog.V(2).InfoS("Work is already associated with the desired resourceSnapshot but still not having the right override snapshots", "resourceIndex", resourceIndex, "work", workObj, "resourceSnapshot", resourceSnapshotObj) } - klog.V(2).InfoS("Work is already associated with the desired resourceSnapshot but still not having the right override snapshots", "resourceIndex", resourceIndex, "work", workObj, "resourceSnapshot", resourceSnapshotObj) } // need to copy the new work to the existing work, only 5 possible changes: + if existingWork.Labels == nil { + existingWork.Labels = make(map[string]string) + } existingWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] = newWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] + if existingWork.Annotations == nil { + existingWork.Annotations = make(map[string]string) + } existingWork.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] = newWork.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] = newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] = newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] diff --git a/pkg/controllers/workgenerator/controller_test.go b/pkg/controllers/workgenerator/controller_test.go index e07952825..d706e9e45 100644 --- a/pkg/controllers/workgenerator/controller_test.go +++ b/pkg/controllers/workgenerator/controller_test.go @@ -11,7 +11,10 @@ import ( "testing" "time" + appsv1 "k8s.io/api/apps/v1" k8serrors "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" "github.com/google/go-cmp/cmp" @@ -24,6 +27,7 @@ import ( fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" "go.goms.io/fleet/pkg/controllers/work" + "go.goms.io/fleet/pkg/utils" "go.goms.io/fleet/pkg/utils/condition" "go.goms.io/fleet/pkg/utils/controller" "go.goms.io/fleet/test/utils/informer" @@ -143,6 +147,216 @@ func TestGetWorkNamePrefixFromSnapshotName(t *testing.T) { } } +func TestUpsertWork(t *testing.T) { + workName := "work" + namespace := "default" + + var cmpOptions = []cmp.Option{ + // ignore the message as we may change the message in the future + cmpopts.IgnoreFields(fleetv1beta1.Work{}, "Status"), + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "CreationTimestamp"), + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion"), + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ManagedFields"), + cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime"), + cmpopts.IgnoreFields(fleetv1beta1.WorkloadTemplate{}, "Manifests"), + } + + testDeployment := appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: utils.DeploymentKind, + APIVersion: utils.DeploymentGVK.GroupVersion().String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "testDeployment", + }, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To(int32(2)), + MinReadySeconds: 5, + }, + } + newWork := &fleetv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: workName, + Namespace: namespace, + Labels: map[string]string{ + fleetv1beta1.ParentResourceSnapshotIndexLabel: "1", + }, + Annotations: map[string]string{ + fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1", + fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: "hash1", + fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation: "hash2", + }, + }, + Spec: fleetv1beta1.WorkSpec{ + Workload: fleetv1beta1.WorkloadTemplate{ + Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Object: &testDeployment}}}, + }, + }, + } + + resourceSnapshot := &fleetv1beta1.ClusterResourceSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "snapshot-1", + Labels: map[string]string{ + fleetv1beta1.ResourceIndexLabel: "1", + }, + }, + } + + tests := []struct { + name string + existingWork *fleetv1beta1.Work + expectChanged bool + }{ + { + name: "Create new work when existing work is nil", + existingWork: nil, + expectChanged: true, + }, + { + name: "Update existing work with new annotations", + existingWork: &fleetv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: workName, + Namespace: namespace, + Labels: map[string]string{ + fleetv1beta1.ParentResourceSnapshotIndexLabel: "1", + }, + }, + Spec: fleetv1beta1.WorkSpec{ + Workload: fleetv1beta1.WorkloadTemplate{ + Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}}, + }, + }, + }, + expectChanged: true, + }, + { + name: "Update existing work even if it does not have the resource snapshot label", + existingWork: &fleetv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: workName, + Namespace: namespace, + }, + Spec: fleetv1beta1.WorkSpec{ + Workload: fleetv1beta1.WorkloadTemplate{ + Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}}, + }, + }, + }, + expectChanged: true, + }, + { + name: "Update existing work if it misses annotations even if the resource snapshot label is correct", + existingWork: &fleetv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: workName, + Namespace: namespace, + Labels: map[string]string{ + fleetv1beta1.ParentResourceSnapshotIndexLabel: "1", + }, + }, + Spec: fleetv1beta1.WorkSpec{ + Workload: fleetv1beta1.WorkloadTemplate{ + Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}}, + }, + }, + }, + expectChanged: true, + }, + { + name: "Update existing work if it does not have correct override snapshot hash", + existingWork: &fleetv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: workName, + Namespace: namespace, + Labels: map[string]string{ + fleetv1beta1.ParentResourceSnapshotIndexLabel: "1", + }, + Annotations: map[string]string{ + fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1", + fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: "wrong-hash"}, + }, + Spec: fleetv1beta1.WorkSpec{ + Workload: fleetv1beta1.WorkloadTemplate{ + Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}}, + }, + }, + }, + expectChanged: true, + }, + { + name: "Do not update the existing work if it already points to the same resource and override snapshots", + existingWork: &fleetv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: workName, + Namespace: namespace, + Labels: map[string]string{ + fleetv1beta1.ParentResourceSnapshotIndexLabel: "1", + }, + Annotations: map[string]string{ + fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1", + fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: "hash1", + fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation: "hash2", + }, + }, + Spec: fleetv1beta1.WorkSpec{ + Workload: fleetv1beta1.WorkloadTemplate{ + Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}}, + }, + }, + }, + expectChanged: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := serviceScheme(t) + objects := []client.Object{resourceSnapshot} + if tt.existingWork != nil { + objects = append(objects, tt.existingWork) + } + fakeClient := fake.NewClientBuilder(). + WithStatusSubresource(objects...). + WithScheme(scheme). + WithObjects(objects...). + Build() + // Create reconciler with custom client + reconciler := &Reconciler{ + Client: fakeClient, + recorder: record.NewFakeRecorder(10), + InformerManager: &informer.FakeManager{}, + } + changed, _ := reconciler.upsertWork(ctx, newWork, tt.existingWork, resourceSnapshot) + if changed != tt.expectChanged { + t.Fatalf("expected changed: %v, got: %v", tt.expectChanged, changed) + } + upsertedWork := &fleetv1beta1.Work{} + if fakeClient.Get(ctx, client.ObjectKeyFromObject(newWork), upsertedWork) != nil { + t.Fatalf("failed to get upserted work") + } + if diff := cmp.Diff(newWork, upsertedWork, cmpOptions...); diff != "" { + t.Errorf("upsertWork didn't update the work, mismatch (-want +got):\n%s", diff) + } + if tt.expectChanged { + // check if the deployment is applied + var u unstructured.Unstructured + if err := u.UnmarshalJSON(upsertedWork.Spec.Workload.Manifests[0].Raw); err != nil { + t.Fatalf("Failed to unmarshal the result: %v, want nil", err) + } + var deployment appsv1.Deployment + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &deployment); err != nil { + t.Fatalf("Failed to convert the result to deployment: %v, want nil", err) + } + if diff := cmp.Diff(testDeployment, deployment); diff != "" { + t.Errorf("The new Deployment mismatch (-want, +got):\n%s", diff) + } + } + }) + } +} + func TestBuildAllWorkAppliedCondition(t *testing.T) { tests := map[string]struct { works map[string]*fleetv1beta1.Work