Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@Arvindthiru
Arvindthiru committed Dec 6, 2024
1 parent 93f1610 commit b4e52bd
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 128 deletions.
15 changes: 6 additions & 9 deletions pkg/controllers/rollout/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,16 +369,13 @@ func (r *Reconciler) pickBindingsToRoll(ctx context.Context, allBindings []*flee
case fleetv1beta1.BindingStateScheduled:
// the scheduler has picked a cluster for this binding
schedulerTargetedBinds = append(schedulerTargetedBinds, binding)
// We need to ensure that this binding is not being deleted.
if binding.DeletionTimestamp.IsZero() {
// this binding has not been bound yet, so it is an update candidate
// pickFromResourceMatchedOverridesForTargetCluster always returns the ordered list of the overrides.
cro, ro, err := r.pickFromResourceMatchedOverridesForTargetCluster(ctx, binding, matchedCROs, matchedROs)
if err != nil {
return nil, nil, false, minWaitTime, err
}
boundingCandidates = append(boundingCandidates, createUpdateInfo(binding, crp, latestResourceSnapshot, cro, ro))
// this binding has not been bound yet, so it is an update candidate
// pickFromResourceMatchedOverridesForTargetCluster always returns the ordered list of the overrides.
cro, ro, err := r.pickFromResourceMatchedOverridesForTargetCluster(ctx, binding, matchedCROs, matchedROs)
if err != nil {
return nil, nil, false, minWaitTime, err
}
boundingCandidates = append(boundingCandidates, createUpdateInfo(binding, crp, latestResourceSnapshot, cro, ro))
case fleetv1beta1.BindingStateBound:
bindingFailed := false
schedulerTargetedBinds = append(schedulerTargetedBinds, binding)
Expand Down
2 changes: 1 addition & 1 deletion pkg/controllers/rollout/controller_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,7 @@ var _ = Describe("Test the rollout Controller", func() {
}, 5*time.Minute, interval).Should(BeTrue(), "rollout controller should roll all the bindings to use the latest resource snapshot")
})

It("Rollout should be blocked, then unblocked by eviction", func() {
It("Rollout should be blocked, then unblocked by eviction - evict unscheduled binding", func() {
// create CRP
var targetCluster int32 = 2
rolloutCRP = clusterResourcePlacementForTest(testCRPName, createPlacementPolicyForTest(fleetv1beta1.PickNPlacementType, targetCluster))
Expand Down
118 changes: 0 additions & 118 deletions pkg/controllers/rollout/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1544,124 +1544,6 @@ func TestPickBindingsToRoll(t *testing.T) {
wantNeedRoll: true,
wantWaitTime: 25 * time.Second, // minWaitTime = (t - 35 seconds) - (t - 60 seconds) = 25 seconds
},
"test one scheduled deleting binding - rollout blocked": {
allBindings: []*fleetv1beta1.ClusterResourceBinding{
generateDeletingClusterResourceBinding(fleetv1beta1.BindingStateScheduled, cluster1),
},
crp: clusterResourcePlacementForTest("test",
createPlacementPolicyForTest(fleetv1beta1.PickAllPlacementType, 0)),
wantTobeUpdatedBindings: []int{},
wantStaleUnselectedBindings: nil,
wantNeedRoll: false,
wantWaitTime: 0,
},
"test one deleting scheduled binding, one scheduled binding - rollout allowed": {
allBindings: []*fleetv1beta1.ClusterResourceBinding{
generateDeletingClusterResourceBinding(fleetv1beta1.BindingStateScheduled, cluster1),
generateClusterResourceBinding(fleetv1beta1.BindingStateScheduled, "snapshot-1", cluster2),
},
latestResourceSnapshotName: "snapshot-1",
crp: clusterResourcePlacementForTest("test",
createPlacementPolicyForTest(fleetv1beta1.PickNPlacementType, 2)),
wantTobeUpdatedBindings: []int{1},
wantStaleUnselectedBindings: []int{},
wantDesiredBindingsSpec: []fleetv1beta1.ResourceBindingSpec{
// scheduled deleting binding does not have desired spec, so it's empty.
{},
{
State: fleetv1beta1.BindingStateBound,
TargetCluster: cluster2,
ResourceSnapshotName: "snapshot-1",
},
},
wantNeedRoll: true,
wantWaitTime: 0,
},
"test one deleting scheduled binding, one canBeReady bound binding, update candidate - rollout blocked, update stale binding status": {
allBindings: []*fleetv1beta1.ClusterResourceBinding{
canBeReadyBinding,
generateDeletingClusterResourceBinding(fleetv1beta1.BindingStateScheduled, cluster2),
},
latestResourceSnapshotName: "snapshot-2", // make canBeReadyBinding update candidate.
crp: clusterResourcePlacementForTest("test",
createPlacementPolicyForTest(fleetv1beta1.PickAllPlacementType, 0)),
wantTobeUpdatedBindings: []int{},
wantStaleUnselectedBindings: []int{0},
wantDesiredBindingsSpec: []fleetv1beta1.ResourceBindingSpec{
{
State: fleetv1beta1.BindingStateBound,
TargetCluster: cluster1,
ResourceSnapshotName: "snapshot-2",
},
// scheduled deleting binding does not have desired spec, so it's empty.
{},
},
wantNeedRoll: true,
wantWaitTime: time.Second,
},
"test one deleting scheduled binding, one ready bound binding, update candidate - rollout blocked, update stale binding status": {
allBindings: []*fleetv1beta1.ClusterResourceBinding{
generateDeletingClusterResourceBinding(fleetv1beta1.BindingStateScheduled, cluster1),
readyBoundBinding,
},
latestResourceSnapshotName: "snapshot-3", // make readyBoundBinding update candidate.
crp: clusterResourcePlacementForTest("test",
createPlacementPolicyForTest(fleetv1beta1.PickAllPlacementType, 0)),
wantTobeUpdatedBindings: []int{},
wantStaleUnselectedBindings: []int{1},
wantDesiredBindingsSpec: []fleetv1beta1.ResourceBindingSpec{
// scheduled deleting binding does not have desired spec, so it's empty.
{},
{
State: fleetv1beta1.BindingStateBound,
TargetCluster: cluster2,
ResourceSnapshotName: "snapshot-3",
},
},
wantNeedRoll: true,
wantWaitTime: 0,
},
"test one deleting scheduled binding, one unscheduled binding - rollout allowed": {
allBindings: []*fleetv1beta1.ClusterResourceBinding{
generateDeletingClusterResourceBinding(fleetv1beta1.BindingStateScheduled, cluster1),
notReadyUnscheduledBinding, // actually ready unscheduled binding since UnavailablePeriodSeconds is not specified in CRP used.
},
latestResourceSnapshotName: "snapshot-1",
crp: clusterResourcePlacementForTest("test",
createPlacementPolicyForTest(fleetv1beta1.PickAllPlacementType, 0)),
wantTobeUpdatedBindings: []int{1},
wantStaleUnselectedBindings: []int{},
wantDesiredBindingsSpec: []fleetv1beta1.ResourceBindingSpec{
// scheduled deleting binding does not have desired spec, so it's empty.
{},
{
State: fleetv1beta1.BindingStateUnscheduled,
TargetCluster: cluster1,
ResourceSnapshotName: "snapshot-1",
},
},
wantNeedRoll: true,
wantWaitTime: 0,
},
"test one deleting scheduled binding, one unscheduled binding - rollout blocked, unscheduled binding is not removed": {
allBindings: []*fleetv1beta1.ClusterResourceBinding{
generateDeletingClusterResourceBinding(fleetv1beta1.BindingStateScheduled, cluster1),
notReadyUnscheduledBinding, // actually ready binding since UnavailablePeriodSeconds is not specified in CRP used.
},
latestResourceSnapshotName: "snapshot-1",
crp: clusterResourcePlacementForTest("test",
createPlacementPolicyForTest(fleetv1beta1.PickNPlacementType, 2)),
wantTobeUpdatedBindings: []int{},
wantStaleUnselectedBindings: []int{},
wantDesiredBindingsSpec: []fleetv1beta1.ResourceBindingSpec{
// scheduled deleting binding does not have desired spec, so it's empty.
{},
// unscheduled deleting binding does not have desired spec, so it's empty.
{},
},
wantNeedRoll: true,
wantWaitTime: 0,
},
"test bound deleting binding - rollout blocked": {
allBindings: []*fleetv1beta1.ClusterResourceBinding{
generateDeletingClusterResourceBinding(fleetv1beta1.BindingStateBound, cluster1),
Expand Down

0 comments on commit b4e52bd

Please sign in to comment.