Skip to content

test: Eviction E2E #996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ryanzhang-oss merged 4 commits into from
Jan 17, 2025
Merged

test: Eviction E2E #996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c754100
add E2Es
Arvindthiru Dec 19, 2024
ea2cbc7
rebase fix
Jan 9, 2025
bf82b6f
address comments
Jan 15, 2025
f3d77ce
remove FDescribe
Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/hubagent/workload/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,8 @@ func SetupControllers(ctx context.Context, wg *sync.WaitGroup, mgr ctrl.Manager,
}
klog.Info("Setting up cluster resource placement eviction controller")
if err := (&clusterresourceplacementeviction.Reconciler{
Client: mgr.GetClient(),
Client: mgr.GetClient(),
UncachedReader: mgr.GetAPIReader(),
}).SetupWithManager(mgr); err != nil {
klog.ErrorS(err, "Unable to set up cluster resource placement eviction controller")
return err
Expand Down
4 changes: 3 additions & 1 deletion pkg/controllers/clusterresourceplacementeviction/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import (
// Reconciler reconciles a ClusterResourcePlacementEviction object.
type Reconciler struct {
client.Client
// UncachedReader is only used to read disruption budget objects directly from the API server to ensure we can enforce the disruption budget for eviction.
UncachedReader client.Reader
Copy link
Contributor

@ryanzhang-oss ryanzhang-oss Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not sure why we need this

Copy link
Contributor Author

@Arvindthiru Arvindthiru Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed flaky behavior when I was writing E2Es where I create an PDB followed by an eviction, but the eviction would successfully execute saying no PDB exists. After ruling all possible E2E related flaky behavior scenarios @michaelawyu helped me figure out it's a cache miss hence the uncached reader to ensure disruption budget is found by the eviction controller by reading from API server when reconciling an eviction object

}

// Reconcile triggers a single eviction reconcile round.
Expand Down Expand Up @@ -185,7 +187,7 @@ func (r *Reconciler) executeEviction(ctx context.Context, validationResult *evic
}

var db placementv1beta1.ClusterResourcePlacementDisruptionBudget
if err := r.Client.Get(ctx, types.NamespacedName{Name: crp.Name}, &db); err != nil {
if err := r.UncachedReader.Get(ctx, types.NamespacedName{Name: crp.Name}, &db); err != nil {
if k8serrors.IsNotFound(err) {
if err = r.deleteClusterResourceBinding(ctx, evictionTargetBinding); err != nil {
return err
Expand Down
3 changes: 2 additions & 1 deletion pkg/controllers/clusterresourceplacementeviction/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,8 @@ func TestExecuteEviction(t *testing.T) {
WithObjects(objects...).
Build()
r := Reconciler{
Client: fakeClient,
Client: fakeClient,
UncachedReader: fakeClient,
}
gotErr := r.executeEviction(ctx, tc.validationResult, tc.eviction)
gotExecutedCondition := tc.eviction.GetCondition(string(placementv1beta1.PlacementEvictionConditionTypeExecuted))
Expand Down
3 changes: 2 additions & 1 deletion pkg/controllers/clusterresourceplacementeviction/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ var _ = BeforeSuite(func() {
Expect(err).Should(Succeed())

err = (&Reconciler{
Client: k8sClient,
Client: k8sClient,
UncachedReader: mgr.GetAPIReader(),
}).SetupWithManager(mgr)
Expect(err).Should(Succeed())

Expand Down
9 changes: 9 additions & 0 deletions test/e2e/actuals_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,15 @@ func crpEvictionRemovedActual(crpEvictionName string) func() error {
}
}

func crpDisruptionBudgetRemovedActual(crpDisruptionBudgetName string) func() error {
return func() error {
if err := hubClient.Get(ctx, types.NamespacedName{Name: crpDisruptionBudgetName}, &placementv1beta1.ClusterResourcePlacementDisruptionBudget{}); !errors.IsNotFound(err) {
return fmt.Errorf("CRP disruption budget still exists or an unexpected error occurred: %w", err)
}
return nil
}
}

func validateCRPSnapshotRevisions(crpName string, wantPolicySnapshotRevision, wantResourceSnapshotRevision int) error {
matchingLabels := client.MatchingLabels{placementv1beta1.CRPTrackingLabel: crpName}

Expand Down
4 changes: 2 additions & 2 deletions test/e2e/enveloped_object_placement_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ var _ = Describe("placing wrapped resources using a CRP", func() {

AfterAll(func() {
By(fmt.Sprintf("deleting placement %s and related resources", crpName))
ensureCRPAndRelatedResourcesDeletion(crpName, allMemberClusters)
ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)
})
})

Expand Down Expand Up @@ -324,7 +324,7 @@ var _ = Describe("placing wrapped resources using a CRP", func() {

AfterAll(func() {
By(fmt.Sprintf("deleting placement %s and related resources", crpName))
ensureCRPAndRelatedResourcesDeletion(crpName, allMemberClusters)
ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)
})
})
})
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/join_and_leave_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ var _ = Describe("Test member cluster join and leave flow", Ordered, Serial, fun

AfterAll(func() {
By(fmt.Sprintf("deleting placement %s and related resources", crpName))
ensureCRPAndRelatedResourcesDeletion(crpName, allMemberClusters)
ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)
})
})

Expand Down
14 changes: 7 additions & 7 deletions test/e2e/placement_cro_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ var _ = Describe("creating clusterResourceOverride (selecting all clusters) to o

AfterAll(func() {
By(fmt.Sprintf("deleting placement %s and related resources", crpName))
ensureCRPAndRelatedResourcesDeletion(crpName, allMemberClusters)
ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)

By(fmt.Sprintf("deleting clusterResourceOverride %s", croName))
cleanupClusterResourceOverride(croName)
Expand Down Expand Up @@ -205,7 +205,7 @@ var _ = Describe("creating clusterResourceOverride with multiple jsonPatchOverri

AfterAll(func() {
By(fmt.Sprintf("deleting placement %s and related resources", crpName))
ensureCRPAndRelatedResourcesDeletion(crpName, allMemberClusters)
ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)

By(fmt.Sprintf("deleting clusterResourceOverride %s", croName))
cleanupClusterResourceOverride(croName)
Expand Down Expand Up @@ -341,7 +341,7 @@ var _ = Describe("creating clusterResourceOverride with different rules for each

AfterAll(func() {
By(fmt.Sprintf("deleting placement %s and related resources", crpName))
ensureCRPAndRelatedResourcesDeletion(crpName, allMemberClusters)
ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)

By(fmt.Sprintf("deleting clusterResourceOverride %s", croName))
cleanupClusterResourceOverride(croName)
Expand Down Expand Up @@ -417,7 +417,7 @@ var _ = Describe("creating clusterResourceOverride with different rules for each

AfterAll(func() {
By(fmt.Sprintf("deleting placement %s and related resources", crpName))
ensureCRPAndRelatedResourcesDeletion(crpName, allMemberClusters)
ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)

By(fmt.Sprintf("deleting clusterResourceOverride %s", croName))
cleanupClusterResourceOverride(croName)
Expand Down Expand Up @@ -494,7 +494,7 @@ var _ = Describe("creating clusterResourceOverride with incorrect path", Ordered

AfterAll(func() {
By(fmt.Sprintf("deleting placement %s and related resources", crpName))
ensureCRPAndRelatedResourcesDeletion(crpName, allMemberClusters)
ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)

By(fmt.Sprintf("deleting clusterResourceOverride %s", croName))
cleanupClusterResourceOverride(croName)
Expand Down Expand Up @@ -564,7 +564,7 @@ var _ = Describe("creating clusterResourceOverride with and resource becomes inv

AfterAll(func() {
By(fmt.Sprintf("deleting placement %s and related resources", crpName))
ensureCRPAndRelatedResourcesDeletion(crpName, allMemberClusters)
ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)

By(fmt.Sprintf("deleting clusterResourceOverride %s", croName))
cleanupClusterResourceOverride(croName)
Expand Down Expand Up @@ -653,7 +653,7 @@ var _ = Describe("creating clusterResourceOverride with delete rules for one clu

AfterAll(func() {
By(fmt.Sprintf("deleting placement %s and related resources", crpName))
ensureCRPAndRelatedResourcesDeletion(crpName, allMemberClusters)
ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)

By(fmt.Sprintf("deleting clusterResourceOverride %s", croName))
cleanupClusterResourceOverride(croName)
Expand Down
Loading
Loading