test: Drain tool tests by Arvindthiru · Pull Request #1104 · Azure/fleet

Arvindthiru · 2025-04-02T17:48:10Z

Description of your changes

Fixes #

I have:

Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

michaelawyu

Added some comments, PTAL

michaelawyu · 2025-04-14T22:53:17Z

 		}
+
+		// Build drain binary.
+		buildCmd := exec.Command("go", "build", "-o", drainBinaryPath, filepath.Join("../../", "tools", "draincluster"))


Hi Arvind! We have a setup.sh file; it might make a lot more sense to just call the build commands there rather than shell out in this Go source code.

In fact these test specs might warrant an environment of its own -> but I guess we could make the move if our test specs continue to grow.

michaelawyu · 2025-04-14T22:57:14Z

 const (
 	uuidLength                  = 8
-	drainEvictionNameFormat     = "drain-eviction-%s-%s-%s"
+	drainEvictionNameFormat     = "drain-eviction-%s"


Hi Arvind! Collision chances aside (which is minimal I assume), I do think that the prev. format might be more clear to the end users.

michaelawyu · 2025-04-14T23:19:43Z

+
+	It("drain cluster using binary, should succeed", func() { runDrainClusterBinary(hubClusterName, memberCluster1EastProdName, true) })
+
+	It("should ensure no resources exist on drained clusters", func() {


Hi Arvind! I suppose we also need a check on the taints (here or as a separate test spec).

And if we would like to check this a bit closer, an inspection on the Eviction object would also be nice.

michaelawyu · 2025-04-14T23:21:52Z

+		"--clusterName", memberClusterName)
+	output, err := cmd.CombinedOutput()
+	Expect(err).ToNot(HaveOccurred(), "Uncordon command failed with error: %v\nOutput: %s", err, string(output))
+	Expect(strings.Contains(string(output), "uncordoned member cluster")).To(BeTrue(), "Expected uncordon to be successful")


Hi Arvind! IIRC the error check part already guarantees that the exit code must be 0; using the string match on logs to verify if it has run successfully is a bit error prone I am afraid.

michaelawyu · 2025-04-14T23:27:20Z

+		"--clusterName", memberClusterName)
+	output, err := cmd.CombinedOutput()
+	Expect(err).ToNot(HaveOccurred(), "Drain command failed with error: %v\nOutput: %s", err, string(output))
+	if isSuccess {


Same issue here; I am wondering if there is a better way to validate this rather than relying on log string matches.

michaelawyu · 2025-04-14T23:30:42Z

+
+	AfterAll(func() {
+		// Uncordon member cluster 1 again to guarantee clean up of cordon taint on test failure.
+		runUncordonClusterBinary(hubClusterName, memberCluster1EastProdName)


Hi Arvind! Just a nit: since the cordon binary itself is a test target, it might be best not to rely it on the cleanup part, just in case.

Same applies for a few other test specs I assume.

michaelawyu · 2025-04-14T23:32:51Z

+		Expect(hubClient.Create(ctx, &crpdb)).To(Succeed(), "Failed to create CRP Disruption Budget %s", crpName)
+	})
+
+	It("drain cluster using binary, should fail due to CRPDB", func() { runDrainClusterBinary(hubClusterName, memberCluster1EastProdName, false) })


Hi Arvind! Some of the issues mentioned earlier apply to this part as well.

It probably would be better if we do a closer inspection (taints, eviction objects) to make sure that the tool is functioning normally rather than just check the logs.

michaelawyu · 2025-04-14T23:35:25Z

+
+	It("uncordon cluster using binary", func() { runUncordonClusterBinary(hubClusterName, memberCluster1EastProdName) })
+
+	It("should update cluster resource placement status as expected", func() {


Hi Arvind! Is this step necessary? We really couldn't tell whether uncordoning works even with this step passing.

michaelawyu · 2025-04-14T23:40:31Z

+
+	It("should place resources on all available member clusters", checkIfPlacedWorkResourcesOnAllMemberClusters)
+})
+


Just my $0.02 but it would be interesting to see how this tool (and Fleet) can actually help DR in an actual workflow of migrating workloads between clusters. It's a good showcase too.

But obviously we could always add more later.

michaelawyu · 2025-04-14T23:46:58Z

+			gotMap, gotErr := h.fetchClusterResourcePlacementNamesToEvict(context.Background())
+			if tc.wantErr == nil {
+				if gotErr != nil {
+					t.Errorf("fetchClusterResourcePlacementNamesToEvict test %s failed, got error %v, want error %v", tc.name, gotErr, tc.wantErr)


Hi Arvind! Style concerns aside, there are a few branches that really do not need to continue once failure are found (even though they can proceed).

Something like

if wantErred if gotErr != nil -> Errorf and return compareErrMessages -> Errorf if needed and return if gotErr -> Errorf and return cmpMap

would be clearer I assume.

This would also apply to other cases I guess.

michaelawyu · 2025-04-15T15:37:22Z

Hi Arvind! I am closing this PR as part of the CNCF repo migration process; please consider moving (re-creating) this PR in the new repo once the sync PR is merged. If there's any question/concern, please let me know. Thanks 🙏

Arvindthiru force-pushed the drain-tool-test branch from ed8f6a2 to 584f822 Compare April 4, 2025 19:05

Arvind Thirumurugan added 2 commits April 10, 2025 15:20

add tests

0d5ea43

fix drain UTs

c3aa1d9

Arvindthiru force-pushed the drain-tool-test branch from 370ea71 to c3aa1d9 Compare April 10, 2025 23:28

Arvind Thirumurugan added 7 commits April 10, 2025 17:31

compile and use tool binaries in E2E

bed3630

add more E2Es

5a33bca

modify generateDrainEvictionName and add UT

3db6e24

minor changes

f03f3e7

add UT

add1089

additional check in E2E

16d249f

remove FDescribe

309b83f

Arvindthiru marked this pull request as ready for review April 14, 2025 21:54

minor changes

80ba76e

michaelawyu reviewed Apr 14, 2025

View reviewed changes

michaelawyu closed this Apr 15, 2025


		It("drain cluster using binary, should succeed", func() { runDrainClusterBinary(hubClusterName, memberCluster1EastProdName, true) })

		It("should ensure no resources exist on drained clusters", func() {


		It("uncordon cluster using binary", func() { runUncordonClusterBinary(hubClusterName, memberCluster1EastProdName) })

		It("should update cluster resource placement status as expected", func() {


		It("should place resources on all available member clusters", checkIfPlacedWorkResourcesOnAllMemberClusters)
		})

Conversation

Arvindthiru commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

How has this code been tested

Special notes for your reviewer

Uh oh!

michaelawyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelawyu commented Apr 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Arvindthiru commented Apr 2, 2025 •

edited

Loading