feat: changes to rollout controller for eviction #959
Conversation
Arvindthiru
force-pushed
the
rolloutEviction
branch
from
6f4a9ca to
46c9946
Compare
Arvindthiru
force-pushed
the
rolloutEviction
branch
from
ec98d10 to
f9d5421
Compare
Arvindthiru
force-pushed
the
rolloutEviction
branch
from
5f5147e to
a893cce
Compare
|
Will add another PR to add more E2Es |
Arvindthiru
force-pushed
the
rolloutEviction
branch
from
8861aff to
f2f5750
Compare
| @@ -607,6 +607,202 @@ 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 - evict unscheduled binding", func() { | |||
There was a problem hiding this comment.
what about the base case that a bound binding is evicted and scheduler creates another binding, make sure we rollout that binding?
There was a problem hiding this comment.
Added as an E2E test since it involves the entire flow
Arvindthiru
force-pushed
the
rolloutEviction
branch
from
90ddef2 to
26b244e
Compare
Arvindthiru
force-pushed
the
rolloutEviction
branch
from
94e345f to
21b20a8
Compare
| @@ -205,23 +206,23 @@ func TestWaitForResourcesToCleanUp(t *testing.T) { | |||
| "test deleting binding block schedule binding on the same cluster": { | |||
| allBindings: []*fleetv1beta1.ClusterResourceBinding{ | |||
| generateClusterResourceBinding(fleetv1beta1.BindingStateScheduled, "snapshot-1", cluster1), | |||
| generateDeletingClusterResourceBinding(cluster1), | |||
| setDeletionTimeStampForBinding(generateClusterResourceBinding(fleetv1beta1.BindingStateUnscheduled, "snapshot-1", cluster1)), | |||
There was a problem hiding this comment.
why add the delete timestamp here? The function it tests doesn't care.
There was a problem hiding this comment.
We check deleting binding here https://github.com/Azure/fleet/blob/main/pkg/controllers/rollout/controller.go#L232 and this particular test expects this output https://github.com/Azure/fleet/blob/main/pkg/controllers/rollout/controller.go#L262
| }, | ||
| }, | ||
| wantTobeUpdatedBindings: []int{2}, // specified MaxSurge helps us pick one scheduled binding to rollout. we don't have any ready binding so we don't remove any binding. |
There was a problem hiding this comment.
In theory, we pick one from 2 or 3. is it guaranteed to pick 2?
| Strategy: fleetv1beta1.RolloutStrategy{ | ||
| Type: fleetv1beta1.RollingUpdateRolloutStrategyType, | ||
| RollingUpdate: &fleetv1beta1.RollingUpdateConfig{ | ||
| wantTobeUpdatedBindings: []int{1}, // specified MaxUnavailable helps us remove unscheduled binding, deleting unscheduled binding even though ready is a canBeUnavailable binding. |
There was a problem hiding this comment.
| wantTobeUpdatedBindings: []int{1}, // specified MaxUnavailable helps us remove unscheduled binding, deleting unscheduled binding even though ready is a canBeUnavailable binding. | |
| wantTobeUpdatedBindings: []int{1}, // specified MaxUnavailable helps us remove unscheduled binding, deleting unscheduled binding even though it is a canBeUnavailable binding. |
| Type: fleetv1beta1.RollingUpdateRolloutStrategyType, | ||
| RollingUpdate: &fleetv1beta1.RollingUpdateConfig{ | ||
| wantTobeUpdatedBindings: []int{1}, // specified MaxUnavailable helps us remove unscheduled binding, deleting unscheduled binding even though ready is a canBeUnavailable binding. | ||
| wantStaleUnselectedBindings: []int{2, 3}, |
There was a problem hiding this comment.
add a comment here too? something like we can't pick them because the max Surge is 0?
| @@ -397,20 +398,27 @@ func (r *Reconciler) pickBindingsToRoll(ctx context.Context, allBindings []*flee | |||
| } else { | |||
| canBeReadyBindings = append(canBeReadyBindings, binding) | |||
There was a problem hiding this comment.
if the bindingReady then it can't be failed, so shouldn't this be in line 388 block?
Description of your changes
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer