Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: initialize the annotation map before assigning a value #944

Merged
merged 3 commits into from
Nov 8, 2024
+236 −13
Merged

fix: initialize the annotation map before assigning a value #944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03caf65
initialize the map
Nov 5, 2024
a7f10c4
add the test
Nov 6, 2024
5a7a665
fix the test
Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions pkg/controllers/workgenerator/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand Down
214 changes: 214 additions & 0 deletions pkg/controllers/workgenerator/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand Down
Loading