Skip to content

Commit

Permalink
feat: check ready status by using avail condition (#756)
Browse files Browse the repository at this point in the history
  • Loading branch information
@zhiying-lin
zhiying-lin authored Apr 15, 2024
1 parent 3a7b2bb commit 3bf1d3a
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 43 deletions.
18 changes: 13 additions & 5 deletions pkg/controllers/rollout/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down
75 changes: 50 additions & 25 deletions pkg/controllers/rollout/controller_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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
Expand All @@ -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]
Expand All @@ -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++ {
Expand All @@ -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 {
Expand All @@ -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))
Expand Down Expand Up @@ -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
Expand All @@ -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]
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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++ {
Expand All @@ -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
}
Expand All @@ -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),
Expand Down
40 changes: 31 additions & 9 deletions pkg/controllers/rollout/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -614,20 +614,41 @@ 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,
},
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,
},
},
},
Expand All @@ -636,20 +657,21 @@ 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,
},
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,
},
},
},
Expand All @@ -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,
Expand All @@ -668,15 +690,15 @@ 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,
},
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{
Expand Down
6 changes: 4 additions & 2 deletions pkg/controllers/work/apply_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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}
Expand Down
2 changes: 1 addition & 1 deletion pkg/controllers/work/apply_controller_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
2 changes: 1 addition & 1 deletion pkg/controllers/work/apply_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,7 @@ func TestGenerateWorkCondition(t *testing.T) {
{
Type: fleetv1beta1.WorkConditionTypeAvailable,
Status: metav1.ConditionTrue,
Reason: workNotTrackableReason,
Reason: WorkNotTrackableReason,
},
},
},
Expand Down

0 comments on commit 3bf1d3a

Please sign in to comment.