Skip to content

feat: override triggers rollout #993

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

Merged
zhiying-lin merged 2 commits into from
Dec 17, 2024
Merged

feat: override triggers rollout #993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daea2b4
feat: override triggers rollout
zhiying-lin Dec 16, 2024
9dc98fe
address comments
zhiying-lin Dec 17, 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
92 changes: 89 additions & 3 deletions pkg/controllers/rollout/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,26 @@ func (r *Reconciler) SetupWithManager(mgr runtime.Manager) error {
handleResourceSnapshot(e.Object, q)
},
}).
Watches(&fleetv1alpha1.ClusterResourceOverrideSnapshot{}, handler.Funcs{
CreateFunc: func(ctx context.Context, e event.CreateEvent, q workqueue.RateLimitingInterface) {
klog.V(2).InfoS("Handling a clusterResourceOverrideSnapshot create event", "clusterResourceOverrideSnapshot", klog.KObj(e.Object))
handleClusterResourceOverrideSnapshot(e.Object, q)
},
GenericFunc: func(ctx context.Context, e event.GenericEvent, q workqueue.RateLimitingInterface) {
klog.V(2).InfoS("Handling a clusterResourceOverrideSnapshot generic event", "clusterResourceOverrideSnapshot", klog.KObj(e.Object))
handleClusterResourceOverrideSnapshot(e.Object, q)
},
}).
Watches(&fleetv1alpha1.ResourceOverrideSnapshot{}, handler.Funcs{
CreateFunc: func(ctx context.Context, e event.CreateEvent, q workqueue.RateLimitingInterface) {
klog.V(2).InfoS("Handling a resourceOverrideSnapshot create event", "resourceOverrideSnapshot", klog.KObj(e.Object))
handleResourceOverrideSnapshot(e.Object, q)
},
GenericFunc: func(ctx context.Context, e event.GenericEvent, q workqueue.RateLimitingInterface) {
klog.V(2).InfoS("Handling a resourceOverrideSnapshot generic event", "resourceOverrideSnapshot", klog.KObj(e.Object))
handleResourceOverrideSnapshot(e.Object, q)
},
}).
Watches(&fleetv1beta1.ClusterResourceBinding{}, handler.Funcs{
CreateFunc: func(ctx context.Context, e event.CreateEvent, q workqueue.RateLimitingInterface) {
klog.V(2).InfoS("Handling a resourceBinding create event", "resourceBinding", klog.KObj(e.Object))
Expand All @@ -643,6 +663,72 @@ func (r *Reconciler) SetupWithManager(mgr runtime.Manager) error {
Complete(r)
}

// handleClusterResourceOverrideSnapshot parse the clusterResourceOverrideSnapshot label and enqueue the CRP name associated
// with the clusterResourceOverrideSnapshot if set.
func handleClusterResourceOverrideSnapshot(o client.Object, q workqueue.RateLimitingInterface) {
Copy link
Contributor

@ryanzhang-oss ryanzhang-oss Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to combine these two functions? We will have RP and CRP co exist all over the place soon.

Copy link
Contributor Author

@zhiying-lin zhiying-lin Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could, but need to rely on the if else condition to handle some cases based on the type, including logging, which hurts our readability.

For the current case, prefer to do the copy & paste.

Copy link
Contributor

@ryanzhang-oss ryanzhang-oss Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to use generic templating?

Copy link
Contributor Author

@zhiying-lin zhiying-lin Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be workable, either using https://google.github.io/styleguide/go/decisions#generics or interface.

snapshot, ok := o.(*fleetv1alpha1.ClusterResourceOverrideSnapshot)
if !ok {
klog.ErrorS(controller.NewUnexpectedBehaviorError(fmt.Errorf("non ClusterResourceOverrideSnapshot type resource: %+v", o)),
"Rollout controller received invalid ClusterResourceOverrideSnapshot event", "object", klog.KObj(o))
return
}

snapshotKRef := klog.KObj(snapshot)
// check if it is the latest resource resourceBinding
isLatest, err := strconv.ParseBool(snapshot.GetLabels()[fleetv1beta1.IsLatestSnapshotLabel])
if err != nil {
klog.ErrorS(controller.NewUnexpectedBehaviorError(fmt.Errorf("invalid label value %s : %w", fleetv1beta1.IsLatestSnapshotLabel, err)),
"Resource clusterResourceOverrideSnapshot has does not have a valid islatest label", "clusterResourceOverrideSnapshot", snapshotKRef)
return
}
if !isLatest {
// All newly created resource snapshots should start with the latest label to be true.
// However, this can happen if the label is removed fast by the time this reconcile loop is triggered.
klog.V(2).InfoS("Newly created resource clusterResourceOverrideSnapshot %s is not the latest", "clusterResourceOverrideSnapshot", snapshotKRef)
return
}
if snapshot.Spec.OverrideSpec.Placement == nil {
return
}
// enqueue the CRP to the rollout controller queue
q.Add(reconcile.Request{
NamespacedName: types.NamespacedName{Name: snapshot.Spec.OverrideSpec.Placement.Name},
})
}

// handleResourceOverrideSnapshot parse the resourceOverrideSnapshot label and enqueue the CRP name associated with the
// resourceOverrideSnapshot if set.
func handleResourceOverrideSnapshot(o client.Object, q workqueue.RateLimitingInterface) {
snapshot, ok := o.(*fleetv1alpha1.ResourceOverrideSnapshot)
if !ok {
klog.ErrorS(controller.NewUnexpectedBehaviorError(fmt.Errorf("non ResourceOverrideSnapshot type resource: %+v", o)),
"Rollout controller received invalid ResourceOverrideSnapshot event", "object", klog.KObj(o))
return
}

snapshotKRef := klog.KObj(snapshot)
// check if it is the latest resource resourceBinding
isLatest, err := strconv.ParseBool(snapshot.GetLabels()[fleetv1beta1.IsLatestSnapshotLabel])
if err != nil {
klog.ErrorS(controller.NewUnexpectedBehaviorError(fmt.Errorf("invalid label value %s : %w", fleetv1beta1.IsLatestSnapshotLabel, err)),
"Resource resourceOverrideSnapshot has does not have a valid islatest annotation", "resourceOverrideSnapshot", snapshotKRef)
return
}
if !isLatest {
// All newly created resource snapshots should start with the latest label to be true.
// However, this can happen if the label is removed fast by the time this reconcile loop is triggered.
klog.V(2).InfoS("Newly created resource resourceOverrideSnapshot %s is not the latest", "resourceOverrideSnapshot", snapshotKRef)
return
}
if snapshot.Spec.OverrideSpec.Placement == nil {
return
}
// enqueue the CRP to the rollout controller queue
q.Add(reconcile.Request{
NamespacedName: types.NamespacedName{Name: snapshot.Spec.OverrideSpec.Placement.Name},
})
}

// handleResourceSnapshot parse the resourceBinding label and annotation and enqueue the CRP name associated with the resource resourceBinding
func handleResourceSnapshot(snapshot client.Object, q workqueue.RateLimitingInterface) {
snapshotKRef := klog.KObj(snapshot)
Expand All @@ -656,14 +742,14 @@ func handleResourceSnapshot(snapshot client.Object, q workqueue.RateLimitingInte
// check if it is the latest resource resourceBinding
isLatest, err := strconv.ParseBool(snapshot.GetLabels()[fleetv1beta1.IsLatestSnapshotLabel])
if err != nil {
klog.ErrorS(controller.NewUnexpectedBehaviorError(fmt.Errorf("invalid annotation value %s : %w", fleetv1beta1.IsLatestSnapshotLabel, err)),
"Resource resourceBinding has does not have a valid islatest annotation", "clusterResourceSnapshot", snapshotKRef)
klog.ErrorS(controller.NewUnexpectedBehaviorError(fmt.Errorf("invalid label value %s : %w", fleetv1beta1.IsLatestSnapshotLabel, err)),
"Resource clusterResourceSnapshot has does not have a valid islatest annotation", "clusterResourceSnapshot", snapshotKRef)
return
}
if !isLatest {
// All newly created resource snapshots should start with the latest label to be true.
// However, this can happen if the label is removed fast by the time this reconcile loop is triggered.
klog.V(2).InfoS("Newly created resource resourceBinding %s is not the latest", "clusterResourceSnapshot", snapshotKRef)
klog.V(2).InfoS("Newly created resource clusterResourceSnapshot %s is not the latest", "clusterResourceSnapshot", snapshotKRef)
return
}
// get the CRP name from the label
Expand Down
67 changes: 67 additions & 0 deletions test/e2e/placement_cro_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,54 @@ var _ = Describe("creating clusterResourceOverride (selecting all clusters) to o
want := map[string]string{croTestAnnotationKey: croTestAnnotationValue}
checkIfOverrideAnnotationsOnAllMemberClusters(true, want)
})

It("update cro attached to this CRP only and change annotation value", func() {
Eventually(func() error {
cro := &placementv1alpha1.ClusterResourceOverride{}
if err := hubClient.Get(ctx, types.NamespacedName{Name: croName}, cro); err != nil {
return err
}
cro.Spec = placementv1alpha1.ClusterResourceOverrideSpec{
Placement: &placementv1alpha1.PlacementRef{
Name: crpName, // assigned CRP name
},
ClusterResourceSelectors: workResourceSelector(),
Policy: &placementv1alpha1.OverridePolicy{
OverrideRules: []placementv1alpha1.OverrideRule{
{
ClusterSelector: &placementv1beta1.ClusterSelector{
ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{},
},
JSONPatchOverrides: []placementv1alpha1.JSONPatchOverride{
{
Operator: placementv1alpha1.JSONPatchOverrideOpAdd,
Path: "/metadata/annotations",
// changed the annotation value to croTestAnnotationValue1
Value: apiextensionsv1.JSON{Raw: []byte(fmt.Sprintf(`{"%s": "%s"}`, croTestAnnotationKey, croTestAnnotationValue1))},
},
},
},
},
},
}
return hubClient.Update(ctx, cro)
}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update cro as expected", crpName)
})

It("should update CRP status as expected", func() {
wantCRONames := []string{fmt.Sprintf(placementv1alpha1.OverrideSnapshotNameFmt, croName, 1)}
crpStatusUpdatedActual := crpStatusWithOverrideUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, "0", wantCRONames, nil)
// use the long duration to wait until the rollout is finished.
Eventually(crpStatusUpdatedActual, longEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName)
})

// This check will ignore the annotation of resources.
It("should place the selected resources on member clusters", checkIfPlacedWorkResourcesOnAllMemberClusters)

It("should have new override annotation value on the placed resources", func() {
want := map[string]string{croTestAnnotationKey: croTestAnnotationValue1}
checkIfOverrideAnnotationsOnAllMemberClusters(true, want)
})
})

var _ = Describe("creating clusterResourceOverride with multiple jsonPatchOverrides", Ordered, func() {
Expand Down Expand Up @@ -173,6 +221,25 @@ var _ = Describe("creating clusterResourceOverride with multiple jsonPatchOverri
wantAnnotations := map[string]string{croTestAnnotationKey: croTestAnnotationValue, croTestAnnotationKey1: croTestAnnotationValue1}
checkIfOverrideAnnotationsOnAllMemberClusters(true, wantAnnotations)
})

It("update cro attached to an invalid CRP", func() {
Eventually(func() error {
cro := &placementv1alpha1.ClusterResourceOverride{}
if err := hubClient.Get(ctx, types.NamespacedName{Name: croName}, cro); err != nil {
return err
}
cro.Spec.Placement = &placementv1alpha1.PlacementRef{
Name: "invalid-crp", // assigned CRP name
}
return hubClient.Update(ctx, cro)
}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update cro as expected", crpName)
})

It("CRP status should not be changed", func() {
wantCRONames := []string{fmt.Sprintf(placementv1alpha1.OverrideSnapshotNameFmt, croName, 0)}
crpStatusUpdatedActual := crpStatusWithOverrideUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, "0", wantCRONames, nil)
Consistently(crpStatusUpdatedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "CRP %s status has been changed", crpName)
})
})

var _ = Describe("creating clusterResourceOverride with different rules for each cluster", Ordered, func() {
Expand Down
71 changes: 71 additions & 0 deletions test/e2e/placement_ro_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,55 @@ var _ = Describe("creating resourceOverride (selecting all clusters) to override
want := map[string]string{roTestAnnotationKey: roTestAnnotationValue}
checkIfOverrideAnnotationsOnAllMemberClusters(false, want)
})

It("update ro attached to this CRP only and change annotation value", func() {
Eventually(func() error {
ro := &placementv1alpha1.ResourceOverride{}
if err := hubClient.Get(ctx, types.NamespacedName{Name: roName, Namespace: roNamespace}, ro); err != nil {
return err
}
ro.Spec = placementv1alpha1.ResourceOverrideSpec{
Placement: &placementv1alpha1.PlacementRef{
Name: crpName,
},
ResourceSelectors: configMapSelector(),
Policy: &placementv1alpha1.OverridePolicy{
OverrideRules: []placementv1alpha1.OverrideRule{
{
ClusterSelector: &placementv1beta1.ClusterSelector{
ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{},
},
JSONPatchOverrides: []placementv1alpha1.JSONPatchOverride{
{
Operator: placementv1alpha1.JSONPatchOverrideOpAdd,
Path: "/metadata/annotations",
Value: apiextensionsv1.JSON{Raw: []byte(fmt.Sprintf(`{"%s": "%s"}`, roTestAnnotationKey, roTestAnnotationValue1))},
},
},
},
},
},
}
return hubClient.Update(ctx, ro)
}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update ro as expected", crpName)
})

It("should update CRP status as expected", func() {
wantRONames := []placementv1beta1.NamespacedName{
{Namespace: roNamespace, Name: fmt.Sprintf(placementv1alpha1.OverrideSnapshotNameFmt, roName, 1)},
}
crpStatusUpdatedActual := crpStatusWithOverrideUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, "0", nil, wantRONames)
// use the long duration to wait until the rollout is finished.
Eventually(crpStatusUpdatedActual, longEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName)
})

// This check will ignore the annotation of resources.
It("should place the selected resources on member clusters", checkIfPlacedWorkResourcesOnAllMemberClusters)

It("should have override annotations on the configmap", func() {
want := map[string]string{roTestAnnotationKey: roTestAnnotationValue1}
checkIfOverrideAnnotationsOnAllMemberClusters(false, want)
})
})

var _ = Describe("creating resourceOverride with multiple jsonPatchOverrides to override configMap", Ordered, func() {
Expand Down Expand Up @@ -181,6 +230,28 @@ var _ = Describe("creating resourceOverride with multiple jsonPatchOverrides to
wantAnnotations := map[string]string{roTestAnnotationKey: roTestAnnotationValue, roTestAnnotationKey1: roTestAnnotationValue1}
checkIfOverrideAnnotationsOnAllMemberClusters(false, wantAnnotations)
})

It("update ro attached to an invalid CRP", func() {
Eventually(func() error {
ro := &placementv1alpha1.ResourceOverride{}
if err := hubClient.Get(ctx, types.NamespacedName{Name: roName, Namespace: roNamespace}, ro); err != nil {
return err
}
ro.Spec.Placement = &placementv1alpha1.PlacementRef{
Name: "invalid-crp", // assigned CRP name
}
return hubClient.Update(ctx, ro)
}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update ro as expected", crpName)
})

It("CRP status should not be changed", func() {
wantRONames := []placementv1beta1.NamespacedName{
{Namespace: roNamespace, Name: fmt.Sprintf(placementv1alpha1.OverrideSnapshotNameFmt, roName, 0)},
}
crpStatusUpdatedActual := crpStatusWithOverrideUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, "0", nil, wantRONames)
Consistently(crpStatusUpdatedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "CRP %s status has been changed", crpName)
})

})

var _ = Describe("creating resourceOverride with different rules for each cluster to override configMap", Ordered, func() {
Expand Down
Loading