feat: changes to rollout controller for eviction#959
Conversation
Arvindthiru
force-pushed
the
rolloutEviction
branch
from
6f4a9ca to
46c9946
Compare
ryanzhang-oss
left a comment
ryanzhang-oss
left a comment
There was a problem hiding this comment.
We need to add a few e2e test for the eviction cases
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
| }, 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
| 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
| ResourceSnapshotName: "snapshot-1", | ||
| }, | ||
| }, | ||
| 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?
There was a problem hiding this comment.
It's guaranteed we pick 2 before 3, we create bounding candidates based on order from allBindings https://github.com/Azure/fleet/blob/main/pkg/controllers/rollout/controller.go#L380 and then we chose update bindings from bounding candidates in the same order https://github.com/Azure/fleet/blob/main/pkg/controllers/rollout/controller.go#L477
| // Create the CRP. | ||
| crp := &placementv1beta1.ClusterResourcePlacement{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: crpName, | ||
| // Add a custom finalizer; this would allow us to better observe | ||
| // the behavior of the controllers. | ||
| Finalizers: []string{customDeletionBlockerFinalizer}, | ||
| }, | ||
| Spec: placementv1beta1.ClusterResourcePlacementSpec{ | ||
| ResourceSelectors: workResourceSelector(), | ||
| }, | ||
| } | ||
| Expect(hubClient.Create(ctx, crp)).To(Succeed(), "Failed to create CRP %s", crpName) |
There was a problem hiding this comment.
there is a createCRP function in test/e2e/utils_test.go
| createWorkResources() | ||
|
|
||
| // Create the CRP. | ||
| crp := &placementv1beta1.ClusterResourcePlacement{ |
2 participants
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