diff --git a/pkg/controllers/rollout/controller.go b/pkg/controllers/rollout/controller.go index 8828d8131..cc4148dcc 100644 --- a/pkg/controllers/rollout/controller.go +++ b/pkg/controllers/rollout/controller.go @@ -104,6 +104,17 @@ func (r *Reconciler) Reconcile(ctx context.Context, req runtime.Request) (runtim allBindings = append(allBindings, binding.DeepCopy()) } + // Process apply strategy updates (if any). This runs independently of the rollout process. + // + // Apply strategy changes will be immediately applied to all bindings that have not been + // marked for deletion yet. Note that even unscheduled bindings will receive this update; + // as apply strategy changes might have an effect on its Applied and Available status, and + // consequently on the rollout progress. + if err := r.processApplyStrategyUpdates(ctx, &crp, allBindings); err != nil { + klog.ErrorS(err, "Failed to process apply strategy updates", "clusterResourcePlacement", crpName) + return runtime.Result{}, err + } + // handle the case that a cluster was unselected by the scheduler and then selected again but the unselected binding is not completely deleted yet wait, err := waitForResourcesToCleanUp(allBindings, &crp) if err != nil { @@ -276,13 +287,14 @@ type toBeUpdatedBinding struct { desiredBinding *fleetv1beta1.ClusterResourceBinding // only valid for scheduled or bound binding } -func createUpdateInfo(binding *fleetv1beta1.ClusterResourceBinding, crp *fleetv1beta1.ClusterResourcePlacement, +func createUpdateInfo(binding *fleetv1beta1.ClusterResourceBinding, latestResourceSnapshot *fleetv1beta1.ClusterResourceSnapshot, cro []string, ro []fleetv1beta1.NamespacedName) toBeUpdatedBinding { desiredBinding := binding.DeepCopy() desiredBinding.Spec.State = fleetv1beta1.BindingStateBound desiredBinding.Spec.ResourceSnapshotName = latestResourceSnapshot.Name - // update the resource apply strategy when controller rolls out the new changes - desiredBinding.Spec.ApplyStrategy = crp.Spec.Strategy.ApplyStrategy + + // Apply strategy is updated separately for all bindings. + // TODO: check the size of the cro and ro to not exceed the limit desiredBinding.Spec.ClusterResourceOverrideSnapshots = cro desiredBinding.Spec.ResourceOverrideSnapshots = ro @@ -377,7 +389,7 @@ func (r *Reconciler) pickBindingsToRoll(ctx context.Context, allBindings []*flee if err != nil { return nil, nil, false, minWaitTime, err } - boundingCandidates = append(boundingCandidates, createUpdateInfo(binding, crp, latestResourceSnapshot, cro, ro)) + boundingCandidates = append(boundingCandidates, createUpdateInfo(binding, latestResourceSnapshot, cro, ro)) case fleetv1beta1.BindingStateBound: bindingFailed := false schedulerTargetedBinds = append(schedulerTargetedBinds, binding) @@ -408,7 +420,7 @@ func (r *Reconciler) pickBindingsToRoll(ctx context.Context, allBindings []*flee } // The binding needs update if it's not pointing to the latest resource resourceBinding or the overrides. if binding.Spec.ResourceSnapshotName != latestResourceSnapshot.Name || !equality.Semantic.DeepEqual(binding.Spec.ClusterResourceOverrideSnapshots, cro) || !equality.Semantic.DeepEqual(binding.Spec.ResourceOverrideSnapshots, ro) { - updateInfo := createUpdateInfo(binding, crp, latestResourceSnapshot, cro, ro) + updateInfo := createUpdateInfo(binding, latestResourceSnapshot, cro, ro) if bindingFailed { // the binding has been applied but failed to apply, we can safely update it to latest resources without affecting max unavailable count applyFailedUpdateCandidates = append(applyFailedUpdateCandidates, updateInfo) @@ -668,6 +680,15 @@ func (r *Reconciler) SetupWithManager(mgr runtime.Manager) error { handleResourceBinding(e.Object, q) }, }, builder.WithPredicates(predicate.GenerationChangedPredicate{})). + // Aside from ClusterResourceSnapshot and ClusterResourceBinding objects, the rollout + // controller also watches ClusterResourcePlacement objects, so that it can push apply + // strategy updates to all bindings right away. + Watches(&fleetv1beta1.ClusterResourcePlacement{}, handler.Funcs{ + // Ignore all Create, Delete, and Generic events; these do not concern the rollout controller. + UpdateFunc: func(ctx context.Context, e event.UpdateEvent, q workqueue.RateLimitingInterface) { + handleCRP(e.ObjectNew, e.ObjectOld, q) + }, + }). Complete(r) } @@ -839,3 +860,93 @@ func (r *Reconciler) updateBindingStatus(ctx context.Context, binding *fleetv1be klog.V(2).InfoS("Updated the status of a binding", "clusterResourceBinding", klog.KObj(binding), "condition", cond) return nil } + +// processApplyStrategyUpdates processes apply strategy updates on the CRP end; specifically +// it will push the update to all applicable bindings. +func (r *Reconciler) processApplyStrategyUpdates( + ctx context.Context, + crp *fleetv1beta1.ClusterResourcePlacement, + allBindings []*fleetv1beta1.ClusterResourceBinding, +) error { + applyStrategy := crp.Spec.Strategy.ApplyStrategy + if applyStrategy == nil { + // Initialize the apply strategy with default values; normally this would not happen + // as default values have been set up in the definitions. + // + // Note (chenyu1): at this moment, due to the fact that Fleet offers both v1 and v1beta1 + // APIs at the same time with Kubernetes favoring the v1 API by default, should the + // user chooses to use the v1 API, default values for v1beta1 exclusive fields + // might not be handled correctly, hence the default value resetting logic added here. + applyStrategy = &fleetv1beta1.ApplyStrategy{} + defaulter.SetDefaultsApplyStrategy(applyStrategy) + } + + errs, childCtx := errgroup.WithContext(ctx) + for idx := range allBindings { + binding := allBindings[idx] + if !binding.DeletionTimestamp.IsZero() { + // The binding has been marked for deletion; no need to push the apply strategy + // update there. + continue + } + + // Verify if the binding has the latest apply strategy set. + if equality.Semantic.DeepEqual(binding.Spec.ApplyStrategy, applyStrategy) { + // The binding already has the latest apply strategy set; no need to push the update. + klog.V(2).InfoS("The binding already has the latest apply strategy; skip the apply strategy update", "clusterResourceBinding", klog.KObj(binding)) + continue + } + + // Push the new apply strategy to the binding. + // + // The ApplyStrategy field on binding objects are managed exclusively by the rollout + // controller; to avoid unnecessary conflicts, Fleet will patch the field directly. + updatedBinding := binding.DeepCopy() + updatedBinding.Spec.ApplyStrategy = applyStrategy + + errs.Go(func() error { + if err := r.Client.Patch(childCtx, updatedBinding, client.MergeFrom(binding)); err != nil { + klog.ErrorS(err, "Failed to update binding with new apply strategy", "clusterResourceBinding", klog.KObj(binding)) + return controller.NewAPIServerError(false, err) + } + klog.V(2).InfoS("Updated binding with new apply strategy", "clusterResourceBinding", klog.KObj(binding)) + return nil + }) + } + + // The patches are issued in parallel; wait for all of them to complete (or the first error + // to return). + return errs.Wait() +} + +// handleCRP handles the update event of a ClusterResourcePlacement, which the rollout controller +// watches. +func handleCRP(newCRPObj, oldCRPObj client.Object, q workqueue.RateLimitingInterface) { + // Do some sanity checks. Normally these checks would never fail. + if newCRPObj == nil || oldCRPObj == nil { + klog.ErrorS(controller.NewUnexpectedBehaviorError(fmt.Errorf("CRP object is nil")), "Received an unexpected nil object in the CRP Update event", "CRP (new)", klog.KObj(newCRPObj), "CRP (old)", klog.KObj(oldCRPObj)) + } + newCRP, newOK := newCRPObj.(*fleetv1beta1.ClusterResourcePlacement) + oldCRP, oldOK := oldCRPObj.(*fleetv1beta1.ClusterResourcePlacement) + if !newOK || !oldOK { + klog.ErrorS(controller.NewUnexpectedBehaviorError(fmt.Errorf("object is not an CRP object")), "Failed to cast the new object in the CRP Update event to a CRP object", "CRP (new)", klog.KObj(newCRPObj), "CRP (old)", klog.KObj(oldCRPObj), "canCastNewObj", newOK, "canCastOldObj", oldOK) + } + + // Check if the CRP has been deleted. + if newCRPObj.GetDeletionTimestamp() != nil { + // No need to process a CRP that has been marked for deletion. + return + } + + // Check if the apply strategy has been updated. + newApplyStrategy := newCRP.Spec.Strategy.ApplyStrategy + oldApplyStrategy := oldCRP.Spec.Strategy.ApplyStrategy + if !equality.Semantic.DeepEqual(newApplyStrategy, oldApplyStrategy) { + klog.V(2).InfoS("Detected an update to the apply strategy on the CRP", "clusterResourcePlacement", klog.KObj(newCRP)) + q.Add(reconcile.Request{ + NamespacedName: types.NamespacedName{Name: newCRP.GetName()}, + }) + } + + klog.V(2).InfoS("No update to apply strategy detected; ignore the CRP Update event", "clusterResourcePlacement", klog.KObj(newCRP)) +} diff --git a/pkg/controllers/rollout/controller_integration_test.go b/pkg/controllers/rollout/controller_integration_test.go index 4aa9f9920..912e8b8cd 100644 --- a/pkg/controllers/rollout/controller_integration_test.go +++ b/pkg/controllers/rollout/controller_integration_test.go @@ -10,6 +10,8 @@ import ( "strconv" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -32,6 +34,11 @@ const ( customBindingFinalizer = "custom-binding-finalizer" ) +var ( + ignoreCRBTypeMetaAndStatusFields = cmpopts.IgnoreFields(fleetv1beta1.ClusterResourceBinding{}, "TypeMeta", "Status") + ignoreObjectMetaAutoGenFields = cmpopts.IgnoreFields(metav1.ObjectMeta{}, "CreationTimestamp", "Generation", "ResourceVersion", "SelfLink", "UID", "ManagedFields") +) + var testCRPName string var _ = Describe("Test the rollout Controller", func() { @@ -96,6 +103,136 @@ var _ = Describe("Test the rollout Controller", func() { }, timeout, interval).Should(BeTrue(), "rollout controller should roll all the bindings to Bound state") }) + It("should push apply strategy changes to all the bindings (if applicable)", func() { + // Create a CRP. + targetClusterCount := int32(3) + rolloutCRP = clusterResourcePlacementForTest( + testCRPName, + createPlacementPolicyForTest(fleetv1beta1.PickNPlacementType, targetClusterCount), + createPlacementRolloutStrategyForTest(fleetv1beta1.RollingUpdateRolloutStrategyType, generateDefaultRollingUpdateConfig(), nil)) + Expect(k8sClient.Create(ctx, rolloutCRP)).Should(Succeed(), "Failed to create CRP") + + // Create a master cluster resource snapshot. + resourceSnapshot := generateResourceSnapshot(rolloutCRP.Name, 0, true) + Expect(k8sClient.Create(ctx, resourceSnapshot)).Should(Succeed(), "Failed to create cluster resource snapshot") + + // Create all the bindings. + clusters := make([]string, targetClusterCount) + for i := 0; i < int(targetClusterCount); i++ { + clusters[i] = "cluster-" + utils.RandStr() + + // Prepare bindings of various states. + var binding *fleetv1beta1.ClusterResourceBinding + switch { + case i%3 == 0: + binding = generateClusterResourceBinding(fleetv1beta1.BindingStateScheduled, resourceSnapshot.Name, clusters[i]) + case i%3 == 1: + binding = generateClusterResourceBinding(fleetv1beta1.BindingStateBound, resourceSnapshot.Name, clusters[i]) + default: + binding = generateClusterResourceBinding(fleetv1beta1.BindingStateUnscheduled, resourceSnapshot.Name, clusters[i]) + } + Expect(k8sClient.Create(ctx, binding)).Should(Succeed(), "Failed to create cluster resource binding") + bindings = append(bindings, binding) + } + + // Verify that all the bindings are updated per rollout strategy. + Eventually(func() error { + for _, binding := range bindings { + gotBinding := &fleetv1beta1.ClusterResourceBinding{} + if err := k8sClient.Get(ctx, types.NamespacedName{Name: binding.GetName()}, gotBinding); err != nil { + return fmt.Errorf("failed to get binding %s: %w", binding.Name, err) + } + + wantBinding := &fleetv1beta1.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: binding.Name, + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + State: binding.Spec.State, + TargetCluster: binding.Spec.TargetCluster, + ApplyStrategy: &fleetv1beta1.ApplyStrategy{ + ComparisonOption: fleetv1beta1.ComparisonOptionTypePartialComparison, + WhenToApply: fleetv1beta1.WhenToApplyTypeAlways, + WhenToTakeOver: fleetv1beta1.WhenToTakeOverTypeAlways, + Type: fleetv1beta1.ApplyStrategyTypeClientSideApply, + }, + }, + } + // The bound binding will have no changes; the scheduled binding, per given + // rollout strategy, will be bound with the resource snapshot. + if binding.Spec.State == fleetv1beta1.BindingStateBound || binding.Spec.State == fleetv1beta1.BindingStateScheduled { + wantBinding.Spec.State = fleetv1beta1.BindingStateBound + wantBinding.Spec.ResourceSnapshotName = resourceSnapshot.Name + } + if diff := cmp.Diff( + gotBinding, wantBinding, + ignoreCRBTypeMetaAndStatusFields, ignoreObjectMetaAutoGenFields, + // For this spec, labels and annotations are irrelevant. + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "Labels", "Annotations"), + ); diff != "" { + return fmt.Errorf("binding diff (-got, +want):\n%s", diff) + } + } + return nil + }, timeout, interval).Should(Succeed(), "Failed to verify that all the bindings are bound") + + // Update the CRP with a new apply strategy. + rolloutCRP.Spec.Strategy.ApplyStrategy = &fleetv1beta1.ApplyStrategy{ + ComparisonOption: fleetv1beta1.ComparisonOptionTypeFullComparison, + WhenToApply: fleetv1beta1.WhenToApplyTypeIfNotDrifted, + WhenToTakeOver: fleetv1beta1.WhenToTakeOverTypeIfNoDiff, + Type: fleetv1beta1.ApplyStrategyTypeServerSideApply, + ServerSideApplyConfig: &fleetv1beta1.ServerSideApplyConfig{ + ForceConflicts: true, + }, + } + Expect(k8sClient.Update(ctx, rolloutCRP)).Should(Succeed(), "Failed to update CRP") + + // Verify that all the bindings are updated with the new apply strategy. + Eventually(func() error { + for _, binding := range bindings { + gotBinding := &fleetv1beta1.ClusterResourceBinding{} + if err := k8sClient.Get(ctx, types.NamespacedName{Name: binding.GetName()}, gotBinding); err != nil { + return fmt.Errorf("failed to get binding %s: %w", binding.Name, err) + } + + wantBinding := &fleetv1beta1.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: binding.Name, + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + State: binding.Spec.State, + TargetCluster: binding.Spec.TargetCluster, + ApplyStrategy: &fleetv1beta1.ApplyStrategy{ + ComparisonOption: fleetv1beta1.ComparisonOptionTypeFullComparison, + WhenToApply: fleetv1beta1.WhenToApplyTypeIfNotDrifted, + WhenToTakeOver: fleetv1beta1.WhenToTakeOverTypeIfNoDiff, + Type: fleetv1beta1.ApplyStrategyTypeServerSideApply, + ServerSideApplyConfig: &fleetv1beta1.ServerSideApplyConfig{ + ForceConflicts: true, + }, + }, + }, + } + // The bound binding will have no changes; the scheduled binding, per given + // rollout strategy, will be bound with the resource snapshot. + if binding.Spec.State == fleetv1beta1.BindingStateBound || binding.Spec.State == fleetv1beta1.BindingStateScheduled { + wantBinding.Spec.State = fleetv1beta1.BindingStateBound + wantBinding.Spec.ResourceSnapshotName = resourceSnapshot.Name + } + if diff := cmp.Diff( + gotBinding, wantBinding, + ignoreCRBTypeMetaAndStatusFields, ignoreObjectMetaAutoGenFields, + // For this spec, labels and annotations are irrelevant. + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "Labels", "Annotations"), + ); diff != "" { + return fmt.Errorf("binding diff (-got, +want):\n%s", diff) + } + } + return nil + }, timeout, interval).Should(Succeed(), "Failed to update all bindings with the new apply strategy") + }) + It("Should rollout all the selected bindings when the rollout strategy is not set", func() { // create CRP var targetCluster int32 = 11 diff --git a/pkg/controllers/rollout/controller_test.go b/pkg/controllers/rollout/controller_test.go index b5c9d7dfd..7f61b5a6d 100644 --- a/pkg/controllers/rollout/controller_test.go +++ b/pkg/controllers/rollout/controller_test.go @@ -41,6 +41,8 @@ var ( cluster5 = "cluster-5" cluster6 = "cluster-6" + crpName = "test-crp" + cmpOptions = []cmp.Option{ cmp.AllowUnexported(toBeUpdatedBinding{}), cmpopts.EquateEmpty(), @@ -794,9 +796,7 @@ func TestPickBindingsToRoll(t *testing.T) { State: fleetv1beta1.BindingStateBound, TargetCluster: cluster1, ResourceSnapshotName: "snapshot-2", - ApplyStrategy: &fleetv1beta1.ApplyStrategy{ - Type: fleetv1beta1.ApplyStrategyTypeServerSideApply, - }, + // The pickBindingsToRoll method no longer refreshes apply strategy. }, }, wantNeedRoll: true, @@ -2636,3 +2636,195 @@ func TestCheckAndUpdateStaleBindingsStatus(t *testing.T) { }) } } + +// TestProcessApplyStrategyUpdates tests the processApplyStrategyUpdates method. +func TestProcessApplyStrategyUpdates(t *testing.T) { + // Use RFC 3339 copy to avoid time precision issues. + now := metav1.Now().Rfc3339Copy() + + testCases := []struct { + name string + crp *fleetv1beta1.ClusterResourcePlacement + allBindings []*fleetv1beta1.ClusterResourceBinding + wantAllBindings []*fleetv1beta1.ClusterResourceBinding + }{ + { + name: "nil apply strategy", + crp: &fleetv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: crpName, + }, + Spec: fleetv1beta1.ClusterResourcePlacementSpec{ + Strategy: fleetv1beta1.RolloutStrategy{}, + }, + }, + allBindings: []*fleetv1beta1.ClusterResourceBinding{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-1", + }, + Spec: fleetv1beta1.ResourceBindingSpec{}, + }, + }, + wantAllBindings: []*fleetv1beta1.ClusterResourceBinding{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-1", + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + ApplyStrategy: &fleetv1beta1.ApplyStrategy{ + Type: fleetv1beta1.ApplyStrategyTypeClientSideApply, + ComparisonOption: fleetv1beta1.ComparisonOptionTypePartialComparison, + WhenToApply: fleetv1beta1.WhenToApplyTypeAlways, + WhenToTakeOver: fleetv1beta1.WhenToTakeOverTypeAlways, + }, + }, + }, + }, + }, + { + name: "push apply strategy to bindings of various states", + crp: &fleetv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: crpName, + }, + Spec: fleetv1beta1.ClusterResourcePlacementSpec{ + Strategy: fleetv1beta1.RolloutStrategy{ + ApplyStrategy: &fleetv1beta1.ApplyStrategy{ + Type: fleetv1beta1.ApplyStrategyTypeServerSideApply, + ComparisonOption: fleetv1beta1.ComparisonOptionTypeFullComparison, + WhenToApply: fleetv1beta1.WhenToApplyTypeIfNotDrifted, + WhenToTakeOver: fleetv1beta1.WhenToTakeOverTypeIfNoDiff, + }, + }, + }, + }, + allBindings: []*fleetv1beta1.ClusterResourceBinding{ + // A binding that has been marked for deletion. + { + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-1", + DeletionTimestamp: &now, + // The fake client requires that all objects that have been marked + // for deletion should have at least one finalizer set. + Finalizers: []string{ + "custom-deletion-blocker", + }, + }, + Spec: fleetv1beta1.ResourceBindingSpec{}, + }, + // A binding that already has the latest apply strategy. + { + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-2", + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "snapshot-2", + ApplyStrategy: &fleetv1beta1.ApplyStrategy{ + Type: fleetv1beta1.ApplyStrategyTypeServerSideApply, + ComparisonOption: fleetv1beta1.ComparisonOptionTypeFullComparison, + WhenToApply: fleetv1beta1.WhenToApplyTypeIfNotDrifted, + WhenToTakeOver: fleetv1beta1.WhenToTakeOverTypeIfNoDiff, + }, + }, + }, + // A binding that has an out of date apply strategy. + { + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-3", + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "snapshot-1", + ApplyStrategy: &fleetv1beta1.ApplyStrategy{ + Type: fleetv1beta1.ApplyStrategyTypeClientSideApply, + ComparisonOption: fleetv1beta1.ComparisonOptionTypePartialComparison, + WhenToApply: fleetv1beta1.WhenToApplyTypeAlways, + WhenToTakeOver: fleetv1beta1.WhenToTakeOverTypeAlways, + }, + }, + }, + }, + wantAllBindings: []*fleetv1beta1.ClusterResourceBinding{ + // Binding that has been marked for deletion should not be updated. + { + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-1", + DeletionTimestamp: &now, + Finalizers: []string{ + "custom-deletion-blocker", + }, + }, + Spec: fleetv1beta1.ResourceBindingSpec{}, + }, + // Binding that already has the latest apply strategy should not be updated. + { + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-2", + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "snapshot-2", + ApplyStrategy: &fleetv1beta1.ApplyStrategy{ + Type: fleetv1beta1.ApplyStrategyTypeServerSideApply, + ComparisonOption: fleetv1beta1.ComparisonOptionTypeFullComparison, + WhenToApply: fleetv1beta1.WhenToApplyTypeIfNotDrifted, + WhenToTakeOver: fleetv1beta1.WhenToTakeOverTypeIfNoDiff, + }, + }, + }, + // Binding that has an out of date apply strategy should be updated. + { + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-3", + }, + Spec: fleetv1beta1.ResourceBindingSpec{ + ResourceSnapshotName: "snapshot-1", + ApplyStrategy: &fleetv1beta1.ApplyStrategy{ + Type: fleetv1beta1.ApplyStrategyTypeServerSideApply, + ComparisonOption: fleetv1beta1.ComparisonOptionTypeFullComparison, + WhenToApply: fleetv1beta1.WhenToApplyTypeIfNotDrifted, + WhenToTakeOver: fleetv1beta1.WhenToTakeOverTypeIfNoDiff, + }, + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + objs := make([]client.Object, 0, len(tc.allBindings)) + for idx := range tc.allBindings { + objs = append(objs, tc.allBindings[idx]) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(serviceScheme(t)). + WithObjects(objs...). + Build() + r := &Reconciler{ + Client: fakeClient, + } + + if err := r.processApplyStrategyUpdates(ctx, tc.crp, tc.allBindings); err != nil { + t.Errorf("processApplyStrategyUpdates() error = %v, want no error", err) + } + + for idx := range tc.wantAllBindings { + wantBinding := tc.wantAllBindings[idx] + + gotBinding := &fleetv1beta1.ClusterResourceBinding{} + if err := fakeClient.Get(ctx, client.ObjectKey{Name: wantBinding.Name}, gotBinding); err != nil { + t.Fatalf("failed to retrieve binding: %v", err) + } + + if diff := cmp.Diff( + gotBinding, wantBinding, + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion"), + ); diff != "" { + t.Errorf("updated binding mismatch (-got, +want):\n%s", diff) + } + } + }) + } +} diff --git a/pkg/controllers/rollout/suite_test.go b/pkg/controllers/rollout/suite_test.go index f8b2fdcfe..1cd0572b3 100644 --- a/pkg/controllers/rollout/suite_test.go +++ b/pkg/controllers/rollout/suite_test.go @@ -25,9 +25,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/manager" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" - workv1alpha1 "sigs.k8s.io/work-api/pkg/apis/v1alpha1" - placementv1alpha1 "go.goms.io/fleet/apis/placement/v1alpha1" + clusterv1beta1 "go.goms.io/fleet/apis/cluster/v1beta1" + fleetv1alpha1 "go.goms.io/fleet/apis/placement/v1alpha1" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" ) @@ -73,8 +73,8 @@ var _ = BeforeSuite(func() { //+kubebuilder:scaffold:scheme By("Set all the customized scheme") Expect(fleetv1beta1.AddToScheme(scheme.Scheme)).Should(Succeed()) - Expect(workv1alpha1.AddToScheme(scheme.Scheme)).Should(Succeed()) - Expect(placementv1alpha1.AddToScheme(scheme.Scheme)).Should(Succeed()) + Expect(clusterv1beta1.AddToScheme(scheme.Scheme)).Should(Succeed()) + Expect(fleetv1alpha1.AddToScheme(scheme.Scheme)).Should(Succeed()) By("starting the controller manager") klog.InitFlags(flag.CommandLine) diff --git a/pkg/controllers/work/applier_server_side_test.go b/pkg/controllers/work/applier_server_side_test.go index 81d193e25..f4d926ae4 100644 --- a/pkg/controllers/work/applier_server_side_test.go +++ b/pkg/controllers/work/applier_server_side_test.go @@ -22,6 +22,7 @@ import ( placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" "go.goms.io/fleet/pkg/utils/controller" + "go.goms.io/fleet/pkg/utils/defaulter" ) func TestApplyUnstructured(t *testing.T) { @@ -237,6 +238,12 @@ func TestApplyUnstructured(t *testing.T) { }, AllowCoOwnership: tc.allowCoOwnership, } + // Certain path of the ApplyUnstructured method will attempt + // to set default values of the retrieved apply strategy from the mock Work object and + // compare it with the passed-in apply strategy. + // To keep things consistent, here the test spec sets the passed-in apply strategy + // as well. + defaulter.SetDefaultsApplyStrategy(applyStrategy) gvr := schema.GroupVersionResource{ Group: "apps", Version: "v1", diff --git a/pkg/controllers/work/applier_test.go b/pkg/controllers/work/applier_test.go index 27f231ab8..894ee8a50 100644 --- a/pkg/controllers/work/applier_test.go +++ b/pkg/controllers/work/applier_test.go @@ -17,6 +17,7 @@ import ( placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" "go.goms.io/fleet/pkg/utils/controller" + "go.goms.io/fleet/pkg/utils/defaulter" ) var ( @@ -206,7 +207,14 @@ func TestFindConflictedWork(t *testing.T) { WithScheme(scheme). WithObjects(objects...). Build() - got, err := findConflictedWork(ctx, fakeClient, testWorkNamespace, &tc.applyStrategy, tc.ownerRefs) + // Certain path of the findConflictedWork function will attempt + // to set default values of the retrieved apply strategy from the mock Work object and + // compare it with the passed-in apply strategy. + // To keep things consistent, here the test spec sets the passed-in apply strategy + // as well. + applyStrategy := &tc.applyStrategy + defaulter.SetDefaultsApplyStrategy(applyStrategy) + got, err := findConflictedWork(ctx, fakeClient, testWorkNamespace, applyStrategy, tc.ownerRefs) if gotErr, wantErr := err != nil, tc.wantErr != nil; gotErr != wantErr || !errors.Is(err, tc.wantErr) { t.Fatalf("findConflictedWork() got error %v, want error %v", err, tc.wantErr) } @@ -412,7 +420,14 @@ func TestValidateOwnerReference(t *testing.T) { WithScheme(scheme). WithObjects(objects...). Build() - got, err := validateOwnerReference(ctx, fakeClient, testWorkNamespace, &tc.applyStrategy, tc.ownerRefs) + // Certain path of the validateOwnerReference function will attempt + // to set default values of the retrieved apply strategy from the mock Work object and + // compare it with the passed-in apply strategy. + // To keep things consistent, here the test spec sets the passed-in apply strategy + // as well. + applyStrategy := &tc.applyStrategy + defaulter.SetDefaultsApplyStrategy(applyStrategy) + got, err := validateOwnerReference(ctx, fakeClient, testWorkNamespace, applyStrategy, tc.ownerRefs) if gotErr, wantErr := err != nil, tc.wantErr != nil; gotErr != wantErr || !errors.Is(err, tc.wantErr) { t.Fatalf("validateOwnerReference() got error %v, want error %v", err, tc.wantErr) } diff --git a/pkg/controllers/work/apply_controller_test.go b/pkg/controllers/work/apply_controller_test.go index 3a50a8439..51b8b46b4 100644 --- a/pkg/controllers/work/apply_controller_test.go +++ b/pkg/controllers/work/apply_controller_test.go @@ -53,6 +53,7 @@ import ( fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" "go.goms.io/fleet/pkg/utils" "go.goms.io/fleet/pkg/utils/controller" + "go.goms.io/fleet/pkg/utils/defaulter" testcontroller "go.goms.io/fleet/test/utils/controller" ) @@ -1803,6 +1804,7 @@ func TestApplyUnstructuredAndTrackAvailability(t *testing.T) { Spec: fleetv1beta1.WorkSpec{ ApplyStrategy: &fleetv1beta1.ApplyStrategy{ AllowCoOwnership: true, + Type: fleetv1beta1.ApplyStrategyTypeClientSideApply, }, }, } @@ -2006,6 +2008,12 @@ func TestApplyUnstructuredAndTrackAvailability(t *testing.T) { Type: fleetv1beta1.ApplyStrategyTypeClientSideApply, AllowCoOwnership: testCase.allowCoOwnership, } + // Certain path of the applyUnstructuredAndTrackAvailability method will attempt + // to set default values of the retrieved apply strategy from the mock Work object and + // compare it with the passed-in apply strategy. + // To keep things consistent, here the test spec sets the passed-in apply strategy + // as well. + defaulter.SetDefaultsApplyStrategy(strategy) applyResult, applyAction, err := r.applyUnstructuredAndTrackAvailability(context.Background(), utils.DeploymentGVR, testCase.workObj, strategy) assert.Equalf(t, testCase.resultAction, applyAction, "updated boolean not matching for Testcase %s", testName) if testCase.resultErr != nil {