diff --git a/pkg/controllers/rollout/controller.go b/pkg/controllers/rollout/controller.go index 6f6ad9cb5..6f5664d3f 100644 --- a/pkg/controllers/rollout/controller.go +++ b/pkg/controllers/rollout/controller.go @@ -32,6 +32,7 @@ import ( fleetv1alpha1 "go.goms.io/fleet/apis/placement/v1alpha1" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" + "go.goms.io/fleet/pkg/controllers/work" "go.goms.io/fleet/pkg/utils/condition" "go.goms.io/fleet/pkg/utils/controller" "go.goms.io/fleet/pkg/utils/defaulter" @@ -473,19 +474,26 @@ func (r *Reconciler) pickBindingsToRoll(ctx context.Context, allBindings []*flee } // isBindingReady checks if a binding is considered ready. -// A binding is considered ready if the binding's current spec has been applied before the ready cutoff time. +// A binding with not trackable resources is considered ready if the binding's current spec has been available before +// the ready cutoff time. func isBindingReady(binding *fleetv1beta1.ClusterResourceBinding, readyTimeCutOff time.Time) (time.Duration, bool) { // find the latest applied condition that has the same generation as the binding - appliedCondition := binding.GetCondition(string(fleetv1beta1.ResourceBindingApplied)) - if condition.IsConditionStatusTrue(appliedCondition, binding.GetGeneration()) { - waitTime := appliedCondition.LastTransitionTime.Time.Sub(readyTimeCutOff) + availableCondition := binding.GetCondition(string(fleetv1beta1.ResourceBindingAvailable)) + if condition.IsConditionStatusTrue(availableCondition, binding.GetGeneration()) { + if availableCondition.Reason != work.WorkNotTrackableReason { + return 0, true + } + + // For the not trackable work, the available condition should be set to true when the work has been applied. + // So here we check the available condition transition time. + waitTime := availableCondition.LastTransitionTime.Time.Sub(readyTimeCutOff) if waitTime < 0 { return 0, true } // return the time we need to wait for it to be ready in this case return waitTime, false } - // we don't know when the current spec is applied yet, return a negative wait time + // we don't know when the current spec is available yet, return a negative wait time return -1, false } diff --git a/pkg/controllers/rollout/controller_integration_test.go b/pkg/controllers/rollout/controller_integration_test.go index 952f328e1..8e311f93e 100644 --- a/pkg/controllers/rollout/controller_integration_test.go +++ b/pkg/controllers/rollout/controller_integration_test.go @@ -19,6 +19,7 @@ import ( "k8s.io/apimachinery/pkg/types" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" + "go.goms.io/fleet/pkg/controllers/work" "go.goms.io/fleet/pkg/utils" ) @@ -127,7 +128,7 @@ var _ = Describe("Test the rollout Controller", func() { }, timeout, interval).Should(BeTrue(), "rollout controller should roll all the bindings to Bound state") }) - It("Should rollout the selected and unselected bindings", func() { + It("Should rollout the selected and unselected bindings (not trackable resources)", func() { // create CRP var targetCluster int32 = 11 rolloutCRP = clusterResourcePlacementForTest(testCRPName, createPlacementPolicyForTest(fleetv1beta1.PickNPlacementType, targetCluster)) @@ -158,10 +159,10 @@ var _ = Describe("Test the rollout Controller", func() { } return true }, timeout, interval).Should(BeTrue(), "rollout controller should roll all the bindings to Bound state") - // simulate that some of the bindings are applied + // simulate that some of the bindings are available and not trackable. firstApplied := 3 for i := 0; i < firstApplied; i++ { - markBindingApplied(bindings[i], true) + markBindingAvailable(bindings[i], false) } // simulate another scheduling decision, pick some cluster to unselect from the bottom of the list var newTarget int32 = 9 @@ -170,10 +171,10 @@ var _ = Describe("Test the rollout Controller", func() { secondRoundBindings := make([]*fleetv1beta1.ClusterResourceBinding, 0) deletedBindings := make([]*fleetv1beta1.ClusterResourceBinding, 0) stillScheduled := 6 - // simulate that some of the bindings are applied - // moved to before being set to unscheduled, otherwise, the rollout controller will try to delete the bindings before we mark them as applied. + // simulate that some of the bindings are available + // moved to before being set to unscheduled, otherwise, the rollout controller will try to delete the bindings before we mark them as available. for i := int(newTarget); i < int(targetCluster); i++ { - markBindingApplied(bindings[i], true) + markBindingAvailable(bindings[i], false) } for i := int(targetCluster - 1); i >= stillScheduled; i-- { binding := bindings[i] @@ -185,9 +186,9 @@ var _ = Describe("Test the rollout Controller", func() { for i := 0; i < stillScheduled; i++ { secondRoundBindings = append(secondRoundBindings, bindings[i]) } - // simulate that some of the bindings are applied + // simulate that some of the bindings are available and not trackable for i := firstApplied; i < int(newTarget); i++ { - markBindingApplied(bindings[i], true) + markBindingAvailable(bindings[i], false) } newScheduled := int(newTarget) - stillScheduled for i := 0; i < newScheduled; i++ { @@ -210,9 +211,9 @@ var _ = Describe("Test the rollout Controller", func() { } return true }, timeout, interval).Should(BeTrue(), "rollout controller should roll all the bindings to Bound state") - // simulate that the new bindings are applied + // simulate that the new bindings are available and not trackable for i := 0; i < len(secondRoundBindings); i++ { - markBindingApplied(secondRoundBindings[i], true) + markBindingAvailable(secondRoundBindings[i], false) } // check that the unselected bindings are deleted Eventually(func() bool { @@ -225,7 +226,7 @@ var _ = Describe("Test the rollout Controller", func() { }, timeout, interval).Should(BeTrue(), "rollout controller should delete all the unselected bindings") }) - It("Should rollout both the new scheduling and the new resources", func() { + It("Should rollout both the new scheduling and the new resources (trackable)", func() { // create CRP var targetCluster int32 = 11 rolloutCRP = clusterResourcePlacementForTest(testCRPName, createPlacementPolicyForTest(fleetv1beta1.PickNPlacementType, targetCluster)) @@ -256,10 +257,10 @@ var _ = Describe("Test the rollout Controller", func() { } return true }, timeout, interval).Should(BeTrue(), "rollout controller should roll all the bindings to Bound state") - // simulate that some of the bindings are applied + // simulate that some of the bindings are available firstApplied := 3 for i := 0; i < firstApplied; i++ { - markBindingApplied(bindings[i], true) + markBindingAvailable(bindings[i], true) } // simulate another scheduling decision, pick some cluster to unselect from the bottom of the list var newTarget int32 = 9 @@ -269,9 +270,9 @@ var _ = Describe("Test the rollout Controller", func() { deletedBindings := make([]*fleetv1beta1.ClusterResourceBinding, 0) stillScheduled := 6 // simulate that some of the bindings are applied - // moved to before being set to unscheduled, otherwise, the rollout controller will try to delete the bindings before we mark them as applied. + // moved to before being set to unscheduled, otherwise, the rollout controller will try to delete the bindings before we mark them as available. for i := int(newTarget); i < int(targetCluster); i++ { - markBindingApplied(bindings[i], true) + markBindingAvailable(bindings[i], true) } for i := int(targetCluster - 1); i >= stillScheduled; i-- { binding := bindings[i] @@ -284,9 +285,9 @@ var _ = Describe("Test the rollout Controller", func() { for i := 0; i < stillScheduled; i++ { secondRoundBindings = append(secondRoundBindings, bindings[i]) } - // simulate that some of the bindings are applied + // simulate that some of the bindings are available for i := firstApplied; i < int(newTarget); i++ { - markBindingApplied(bindings[i], true) + markBindingAvailable(bindings[i], true) } // create the newly scheduled bindings newScheduled := int(newTarget) - stillScheduled @@ -319,9 +320,9 @@ var _ = Describe("Test the rollout Controller", func() { } return true }, timeout, interval).Should(BeTrue(), "rollout controller should roll all the bindings to Bound state") - // simulate that the new bindings are applied + // simulate that the new bindings are available for i := 0; i < len(secondRoundBindings); i++ { - markBindingApplied(secondRoundBindings[i], true) + markBindingAvailable(secondRoundBindings[i], true) } // check that the unselected bindings are deleted Eventually(func() bool { @@ -343,8 +344,8 @@ var _ = Describe("Test the rollout Controller", func() { return false } if binding.Spec.ResourceSnapshotName == newMasterSnapshot.Name { - // simulate the work generator to make the newly updated bindings to be applied - markBindingApplied(binding, true) + // simulate the work generator to make the newly updated bindings to be available + markBindingAvailable(binding, true) } else { misMatch = true } @@ -485,10 +486,10 @@ var _ = Describe("Test the rollout Controller", func() { } return true }, timeout, interval).Should(BeTrue(), "rollout controller should roll all the bindings to Bound state") - // simulate that some of the bindings are applied successfully + // simulate that some of the bindings are available successfully applySuccessfully := 3 for i := 0; i < applySuccessfully; i++ { - markBindingApplied(bindings[i], true) + markBindingAvailable(bindings[i], true) } // simulate that some of the bindings fail to apply for i := applySuccessfully; i < int(targetCluster); i++ { @@ -511,8 +512,8 @@ var _ = Describe("Test the rollout Controller", func() { allMatch = false } if binding.Spec.ResourceSnapshotName == newMasterSnapshot.Name { - // simulate the work generator to make the newly updated bindings to be applied successfully - markBindingApplied(binding, true) + // simulate the work generator to make the newly updated bindings to be available + markBindingAvailable(binding, true) } else { allMatch = false } @@ -527,6 +528,30 @@ var _ = Describe("Test the rollout Controller", func() { }) +func markBindingAvailable(binding *fleetv1beta1.ClusterResourceBinding, trackable bool) { + Eventually(func() error { + reason := "trackable" + if !trackable { + reason = work.WorkNotTrackableReason + } + binding.SetConditions(metav1.Condition{ + Type: string(fleetv1beta1.ResourceBindingAvailable), + Status: metav1.ConditionTrue, + Reason: reason, + ObservedGeneration: binding.Generation, + }) + if err := k8sClient.Status().Update(ctx, binding); err != nil { + if apierrors.IsConflict(err) { + // get the binding again to avoid conflict + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) + } + return err + } + return nil + }, timeout, interval).Should(Succeed(), "should update the binding status successfully") + By(fmt.Sprintf("resource binding `%s` is marked as available", binding.Name)) +} + func markBindingApplied(binding *fleetv1beta1.ClusterResourceBinding, success bool) { applyCondition := metav1.Condition{ Type: string(fleetv1beta1.ResourceBindingApplied), diff --git a/pkg/controllers/rollout/controller_test.go b/pkg/controllers/rollout/controller_test.go index da3c0a5f6..37883feec 100644 --- a/pkg/controllers/rollout/controller_test.go +++ b/pkg/controllers/rollout/controller_test.go @@ -22,11 +22,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllertest" - "go.goms.io/fleet/pkg/utils/condition" - clusterv1beta1 "go.goms.io/fleet/apis/cluster/v1beta1" fleetv1alpha1 "go.goms.io/fleet/apis/placement/v1alpha1" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" + "go.goms.io/fleet/pkg/controllers/work" + "go.goms.io/fleet/pkg/utils/condition" "go.goms.io/fleet/pkg/utils/controller" ) @@ -614,7 +614,27 @@ func TestIsBindingReady(t *testing.T) { wantReady bool wantWaitTime time.Duration }{ - "binding applied before the ready time cut off should return ready": { + "binding available (trackable) is ready": { + binding: &fleetv1beta1.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 10, + }, + Status: fleetv1beta1.ResourceBindingStatus{ + Conditions: []metav1.Condition{ + { + Type: string(fleetv1beta1.ResourceBindingAvailable), + Status: metav1.ConditionTrue, + ObservedGeneration: 10, + Reason: "any", + }, + }, + }, + }, + readyTimeCutOff: now, + wantReady: true, + wantWaitTime: 0, + }, + "binding available (not trackable) before the ready time cut off should return ready": { binding: &fleetv1beta1.ClusterResourceBinding{ ObjectMeta: metav1.ObjectMeta{ Generation: 10, @@ -622,12 +642,13 @@ func TestIsBindingReady(t *testing.T) { Status: fleetv1beta1.ResourceBindingStatus{ Conditions: []metav1.Condition{ { - Type: string(fleetv1beta1.ResourceBindingApplied), + Type: string(fleetv1beta1.ResourceBindingAvailable), Status: metav1.ConditionTrue, ObservedGeneration: 10, LastTransitionTime: metav1.Time{ Time: now.Add(-time.Millisecond), }, + Reason: work.WorkNotTrackableReason, }, }, }, @@ -636,7 +657,7 @@ func TestIsBindingReady(t *testing.T) { wantReady: true, wantWaitTime: 0, }, - "binding applied after the ready time cut off should return not ready with a wait time": { + "binding available (not trackable) after the ready time cut off should return not ready with a wait time": { binding: &fleetv1beta1.ClusterResourceBinding{ ObjectMeta: metav1.ObjectMeta{ Generation: 10, @@ -644,12 +665,13 @@ func TestIsBindingReady(t *testing.T) { Status: fleetv1beta1.ResourceBindingStatus{ Conditions: []metav1.Condition{ { - Type: string(fleetv1beta1.ResourceBindingApplied), + Type: string(fleetv1beta1.ResourceBindingAvailable), Status: metav1.ConditionTrue, ObservedGeneration: 10, LastTransitionTime: metav1.Time{ Time: now.Add(time.Millisecond), }, + Reason: work.WorkNotTrackableReason, }, }, }, @@ -658,7 +680,7 @@ func TestIsBindingReady(t *testing.T) { wantReady: false, wantWaitTime: time.Millisecond, }, - "binding not applied should return not ready with a negative wait time": { + "binding not available should return not ready with a negative wait time": { binding: &fleetv1beta1.ClusterResourceBinding{ ObjectMeta: metav1.ObjectMeta{ Generation: 10, @@ -668,7 +690,7 @@ func TestIsBindingReady(t *testing.T) { wantReady: false, wantWaitTime: -1, }, - "binding applied for a previous generation should return not ready with a negative wait time": { + "binding available for a previous generation should return not ready with a negative wait time": { binding: &fleetv1beta1.ClusterResourceBinding{ ObjectMeta: metav1.ObjectMeta{ Generation: 10, @@ -676,7 +698,7 @@ func TestIsBindingReady(t *testing.T) { Status: fleetv1beta1.ResourceBindingStatus{ Conditions: []metav1.Condition{ { - Type: string(fleetv1beta1.ResourceBindingApplied), + Type: string(fleetv1beta1.ResourceBindingAvailable), Status: metav1.ConditionTrue, ObservedGeneration: 9, //not the current generation LastTransitionTime: metav1.Time{ diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 21e80e0fc..eef64b7cb 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -69,7 +69,9 @@ const ( workNotAvailableYetReason = "WorkNotAvailableYet" workAvailabilityUnknownReason = "WorkAvailabilityUnknown" workAvailableReason = "WorkAvailable" - workNotTrackableReason = "WorkNotTrackable" + // WorkNotTrackableReason is the reason string of condition when the manifest is already up to date but we don't have + // a way to track its availabilities. + WorkNotTrackableReason = "WorkNotTrackable" // ManifestApplyFailedReason is the reason string of condition when it failed to apply manifest. ManifestApplyFailedReason = "ManifestApplyFailed" // ApplyConflictBetweenPlacementsReason is the reason string of condition when the manifest is owned by multiple placements, @@ -879,7 +881,7 @@ func buildWorkCondition(manifestConditions []fleetv1beta1.ManifestCondition, obs availableCondition.Reason = workAvailableReason availableCondition.Message = "Work is available now" } else { - availableCondition.Reason = workNotTrackableReason + availableCondition.Reason = WorkNotTrackableReason availableCondition.Message = "Work's availability is not trackable" } return []metav1.Condition{applyCondition, availableCondition} diff --git a/pkg/controllers/work/apply_controller_integration_test.go b/pkg/controllers/work/apply_controller_integration_test.go index f1e2c07dc..dbb2a72ec 100644 --- a/pkg/controllers/work/apply_controller_integration_test.go +++ b/pkg/controllers/work/apply_controller_integration_test.go @@ -110,7 +110,7 @@ var _ = Describe("Work Controller", func() { { Type: fleetv1beta1.WorkConditionTypeAvailable, Status: metav1.ConditionTrue, - Reason: workNotTrackableReason, + Reason: WorkNotTrackableReason, }, } Expect(controller.CompareConditions(expected, resultWork.Status.Conditions)).Should(BeEmpty()) diff --git a/pkg/controllers/work/apply_controller_test.go b/pkg/controllers/work/apply_controller_test.go index 54ff26e0e..ed991399e 100644 --- a/pkg/controllers/work/apply_controller_test.go +++ b/pkg/controllers/work/apply_controller_test.go @@ -766,7 +766,7 @@ func TestGenerateWorkCondition(t *testing.T) { { Type: fleetv1beta1.WorkConditionTypeAvailable, Status: metav1.ConditionTrue, - Reason: workNotTrackableReason, + Reason: WorkNotTrackableReason, }, }, },