Fix flaky e2e tests (#2539)

* Wait for target namespace deletion

This may prevent a race condition whereby cleanup for a given test case
may finish deleting a namespace _after_ the next test case starts
running, leading to that test case failing to set itself up, with
`BeforeEach` not finding the required config map.

* Retry docker pulling image in imagescan tests

In some cases, imagescan end-to-end tests fail because of a failure to
pull an image. This enables retries in such cases, in the hope of
reducing or eliminating test flakiness.

* Give integration tests more time to comply with expectations

This may fix flakiness on Github runners.

* Refine error check in Fleet controller logs

Checking logs for a single `ERROR` keyword may lead to false positives,
for instance caused by other test suites. Instead, this uses a regular
expression to look for logs related to missing bundles or bundle
deployments.

* Make drift correction end-to-end tests more robust

These tests now wait for deletion of a `GitRepo` to complete before
carrying on with the next test case, which could prevent interferences.

In case of a failure to get a bundle ready on time before running a test
case, they should also now output bundle status to ease troubleshooting.

This also improves output of drift correction tests to help detect patches
which may not have caused expected drifts.

* Clean up after tests

This ensures that more namespaces and `GitRepo`s created by
end-to-end tests are deleted after execution.

* Expose error if bundle not ready

This should ease troubleshooting of flaky tests for deploying bundles
from `GitRepo`s with multiple paths.

---------

Co-authored-by: Alejandro Ruiz <[email protected]>

e2e/drift/drift_test.go

@@ -2,6 +2,7 @@ package examples_test
 
 import (
 	"encoding/json"
+	"fmt"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -32,13 +33,24 @@ var _ = Describe("Drift", Ordered, func() {
 			b := getBundle(bundleName, k)
 			return b.Status.Summary.Ready == 1
 		}).Should(BeTrue())
+
+		defer func() {
+			if r := recover(); r != nil {
+				bundle := getBundle(bundleName, k)
+				GinkgoWriter.Printf("bundle status: %v", bundle.Status)
+			}
+		}()
 	})
 
 	AfterEach(func() {
-		out, err := k.Namespace(env.Namespace).Delete("-f", testenv.AssetPath(asset))
+		out, err := k.Namespace(env.Namespace).Delete("-f", testenv.AssetPath(asset), "--wait")
 		Expect(err).ToNot(HaveOccurred(), out)
 	})
 
+	AfterAll(func() {
+		_, _ = k.Delete("ns", namespace)
+	})
+
 	When("Drift correction is not enabled", func() {
 		BeforeEach(func() {
 			asset = "drift/correction-disabled/gitrepo.yaml"
@@ -50,10 +62,12 @@ var _ = Describe("Drift", Ordered, func() {
 				kw := k.Namespace(namespace)
 				out, err := kw.Patch(
 					"service", "nginx-service",
+					"-o=json",
 					"--type=json",
 					"-p", `[{"op": "replace", "path": "/spec/externalName", "value": "modified"}]`,
 				)
 				Expect(err).ToNot(HaveOccurred(), out)
+				GinkgoWriter.Print(out)
 			})
 
 			It("Bundle is modified", func() {
@@ -81,10 +95,12 @@ var _ = Describe("Drift", Ordered, func() {
 				kw := k.Namespace(namespace)
 				out, err := kw.Patch(
 					"service", "nginx-service",
+					"-o=json",
 					"--type=json",
 					"-p", `[{"op": "replace", "path": "/spec/externalName", "value": "modified"}]`,
 				)
 				Expect(err).ToNot(HaveOccurred(), out)
+				GinkgoWriter.Print(out)
 			})
 
 			It("Drift is corrected", func() {
@@ -107,10 +123,12 @@ var _ = Describe("Drift", Ordered, func() {
 				kw := k.Namespace(namespace)
 				out, err := kw.Patch(
 					"deployment", "nginx-deployment",
+					"-o=json",
 					"--type=json",
 					"-p", `[{"op": "replace", "path": "/spec/template/spec/containers/0/image", "value": "nginx:modified"}]`,
 				)
 				Expect(err).ToNot(HaveOccurred(), out)
+				GinkgoWriter.Print(out)
 			})
 
 			It("Drift is corrected", func() {
@@ -133,10 +151,12 @@ var _ = Describe("Drift", Ordered, func() {
 				kw := k.Namespace(namespace)
 				out, err := kw.Patch(
 					"configmap", "configmap",
+					"-o=json",
 					"--type=json",
 					"-p", `[{"op": "replace", "path": "/data/foo", "value": "modified"}]`,
 				)
 				Expect(err).ToNot(HaveOccurred(), out)
+				GinkgoWriter.Print(out)
 			})
 
 			It("Drift is corrected", func() {
@@ -169,10 +189,12 @@ var _ = Describe("Drift", Ordered, func() {
 				kw := k.Namespace(namespace)
 				out, err := kw.Patch(
 					"service", "nginx-service",
+					"-o=json",
 					"--type=json",
 					"-p", `[{"op": "replace", "path": "/spec/ports/0/port", "value": 1234}]`,
 				)
 				Expect(err).ToNot(HaveOccurred(), out)
+				GinkgoWriter.Print(out)
 			})
 
 			It("Status is modified", func() {
@@ -209,17 +231,20 @@ var _ = Describe("Drift", Ordered, func() {
 				kw := k.Namespace(namespace)
 				out, err := kw.Patch(
 					"service", "nginx-service",
+					"-o=json",
 					"--type=json",
 					"-p", `[{"op": "replace", "path": "/spec/ports/0/port", "value": 1234}]`,
 				)
 				Expect(err).ToNot(HaveOccurred(), out)
+				GinkgoWriter.Print(out)
 			})
 
 			It("Bundle Status is Ready, and changes are rolled back", func() {
-				Eventually(func() bool {
-					b := getBundle(bundleName, k)
-					return b.Status.Summary.Ready == 1
-				}).Should(BeTrue())
+				var bundle fleet.Bundle
+				Eventually(func() int {
+					bundle = getBundle(bundleName, k)
+					return bundle.Status.Summary.Ready
+				}).Should(Equal(1), fmt.Sprintf("Summary: %+v", bundle.Status.Summary))
 				Eventually(func() bool {
 					kw := k.Namespace(namespace)
 					out, _ := kw.Get("services", "nginx-service", "-o=json")

e2e/metrics/bundledeployment_test.go

@@ -24,14 +24,16 @@ var _ = Describe("BundleDeployment Metrics", Label("bundledeployment"), func() {
 		// random namespace, like it is the case for the other tests.
 		objName string
 
+		targetNS string
+
 		r = rand.New(rand.NewSource(GinkgoRandomSeed()))
 	)
 
 	BeforeEach(func() {
 		k = env.Kubectl.Namespace(env.Namespace)
 		kw = k.Namespace(namespace)
 		objName = testenv.AddRandomSuffix("metrics", r)
-		targetNS := testenv.NewNamespaceName("metrics", r)
+		targetNS = testenv.NewNamespaceName("metrics", r)
 
 		err := testenv.CreateGitRepo(
 			kw,
@@ -51,6 +53,9 @@ var _ = Describe("BundleDeployment Metrics", Label("bundledeployment"), func() {
 		DeferCleanup(func() {
 			out, err := k.Delete("gitrepo", objName)
 			Expect(err).ToNot(HaveOccurred(), out)
+
+			out, err = k.Delete("ns", targetNS, "--wait=false")
+			Expect(err).ToNot(HaveOccurred(), out)
 		})
 	})
 

e2e/single-cluster/delete_namespaces_test.go

@@ -30,7 +30,7 @@ var _ = Describe("delete namespaces", func() {
 		deleteNamespace = false
 
 		DeferCleanup(func() {
-			_, _ = k.Delete("ns", "my-custom-namespace", "--wait=false")
+			_, _ = k.Delete("ns", "my-custom-namespace")
 		})
 	})
 

e2e/single-cluster/finalizers_test.go

@@ -45,6 +45,7 @@ var _ = Describe("Deleting a resource with finalizers", func() {
 
 		_, _ = k.Delete("gitrepo", gitrepoName)
 		_, _ = k.Delete("bundle", fmt.Sprintf("%s-%s", gitrepoName, path))
+		_, _ = k.Delete("ns", targetNamespace, "--wait=false")
 	})
 
 	When("deleting an existing GitRepo", func() {
@@ -277,8 +278,8 @@ var _ = Describe("Deleting a resource with finalizers", func() {
 			Expect(out).ToNot(BeZero())
 
 			By("checking that the configmap created by the bundle deployment still exists")
-			_, err = k.Namespace(targetNamespace).Get("configmap", "test-simple-chart-config")
-			Expect(err).ToNot(HaveOccurred())
+			out, err = k.Namespace(targetNamespace).Get("configmap", "test-simple-chart-config")
+			Expect(err).ToNot(HaveOccurred(), out)
 		})
 	})
 })

e2e/single-cluster/gitrepo_polling_disabled_test.go

@@ -67,7 +67,15 @@ var _ = Describe("GitRepoPollingDisabled", Label("infra-setup"), func() {
 			"fleet-controller",
 		)
 		Expect(err).ToNot(HaveOccurred())
-		Expect(out).ToNot(ContainSubstring("ERROR"))
+
+		// Errors about resources other than bundles or bundle deployments not being found at deletion time
+		// should be ignored, as they may result from other test suites.
+		Expect(out).ToNot(MatchRegexp(
+			`ERROR.*Reconciler error.*Bundle(Deployment)?.fleet.cattle.io \\".*\\" not found`,
+		))
+
+		_, err = k.Delete("ns", targetNamespace)
+		Expect(err).ToNot(HaveOccurred())
 	})
 
 	When("applying a gitrepo with disable polling", func() {

e2e/single-cluster/gitrepo_test.go

@@ -49,6 +49,7 @@ var _ = Describe("Monitoring Git repos via HTTP for change", Label("infra-setup"
 	})
 
 	JustBeforeEach(func() {
+
 		// Build git repo URL reachable _within_ the cluster, for the GitRepo
 		host, err := githelper.BuildGitHostname(env.Namespace)
 		Expect(err).ToNot(HaveOccurred())
@@ -87,7 +88,15 @@ var _ = Describe("Monitoring Git repos via HTTP for change", Label("infra-setup"
 			"fleet-controller",
 		)
 		Expect(err).ToNot(HaveOccurred())
-		Expect(out).ToNot(ContainSubstring("ERROR"))
+
+		// Errors about resources other than bundles or bundle deployments not being found at deletion time
+		// should be ignored, as they may result from other test suites.
+		Expect(out).ToNot(MatchRegexp(
+			`ERROR.*Reconciler error.*Bundle(Deployment)?.fleet.cattle.io \\".*\\" not found`,
+		))
+
+		_, err = k.Delete("ns", targetNamespace)
+		Expect(err).ToNot(HaveOccurred())
 	})
 
 	When("updating a git repository monitored via polling", func() {

e2e/single-cluster/helm_auth_test.go

@@ -1,6 +1,7 @@
 package singlecluster_test
 
 import (
+	"fmt"
 	"os"
 	"path"
 
@@ -14,10 +15,11 @@ import (
 
 var _ = Describe("Single Cluster Examples", Label("infra-setup"), func() {
 	var (
-		gitRepoPath string
-		tmpdir      string
-		k           kubectl.Command
-		gh          *githelper.Git
+		gitRepoPath     string
+		tmpdir          string
+		k               kubectl.Command
+		gh              *githelper.Git
+		targetNamespace string
 	)
 
 	JustBeforeEach(func() {
@@ -47,44 +49,47 @@ var _ = Describe("Single Cluster Examples", Label("infra-setup"), func() {
 		Context("containing a private OCI-based helm chart", Label("oci-registry"), func() {
 			BeforeEach(func() {
 				gitRepoPath = "oci-with-auth"
+				targetNamespace = fmt.Sprintf("fleet-helm-%s", gitRepoPath)
 				k = env.Kubectl.Namespace(env.Namespace)
 
 				tmpdir, gh = setupGitRepo(gitRepoPath, repoName, port)
 			})
 
 			It("deploys the helm chart", func() {
 				Eventually(func() string {
-					out, _ := k.Namespace("fleet-helm-oci-with-auth").Get("pods", "--field-selector=status.phase==Running")
+					out, _ := k.Namespace(targetNamespace).Get("pods", "--field-selector=status.phase==Running")
 					return out
 				}).Should(ContainSubstring("sleeper-"))
 			})
 		})
 		Context("containing a private HTTP-based helm chart with repo path", Label("helm-registry"), func() {
 			BeforeEach(func() {
 				gitRepoPath = "http-with-auth-repo-path"
+				targetNamespace = fmt.Sprintf("fleet-helm-%s", gitRepoPath)
 				k = env.Kubectl.Namespace(env.Namespace)
 
 				tmpdir, gh = setupGitRepo(gitRepoPath, repoName, port)
 			})
 
 			It("deploys the helm chart", func() {
 				Eventually(func() string {
-					out, _ := k.Namespace("fleet-helm-http-with-auth-repo-path").Get("pods", "--field-selector=status.phase==Running")
+					out, _ := k.Namespace(targetNamespace).Get("pods", "--field-selector=status.phase==Running")
 					return out
 				}).Should(ContainSubstring("sleeper-"))
 			})
 		})
 		Context("containing a private HTTP-based helm chart with chart path", Label("helm-registry"), func() {
 			BeforeEach(func() {
 				gitRepoPath = "http-with-auth-chart-path"
+				targetNamespace = fmt.Sprintf("fleet-helm-%s", gitRepoPath)
 				k = env.Kubectl.Namespace(env.Namespace)
 
 				tmpdir, gh = setupGitRepo(gitRepoPath, repoName, port)
 			})
 
 			It("deploys the helm chart", func() {
 				Eventually(func() string {
-					out, _ := k.Namespace("fleet-helm-http-with-auth-chart-path").Get("pods", "--field-selector=status.phase==Running")
+					out, _ := k.Namespace(targetNamespace).Get("pods", "--field-selector=status.phase==Running")
 					return out
 				}).Should(ContainSubstring("sleeper-"))
 			})
@@ -95,6 +100,7 @@ var _ = Describe("Single Cluster Examples", Label("infra-setup"), func() {
 		os.RemoveAll(tmpdir)
 
 		_, _ = k.Delete("gitrepo", "helm")
+		_, _ = k.Delete("ns", targetNamespace, "--wait=false")
 	})
 })
 

e2e/single-cluster/imagescan_test.go

@@ -279,9 +279,13 @@ func tagAndPushImage(baseImage, image, tag string) string {
 }
 
 func initRegistryWithImageAndTag(baseImage string, tag string) (string, string) {
-	cmd := exec.Command("docker", "pull", baseImage)
-	err := cmd.Run()
-	Expect(err).ToNot(HaveOccurred())
+	Eventually(func() error {
+		cmd := exec.Command("docker", "pull", baseImage)
+		err := cmd.Run()
+
+		return err
+	}, 20*time.Second, 1*time.Second).Should(Succeed())
+
 	// generate a new uuid for this test
 	uuid := uuid.NewUUID()
 	image := fmt.Sprintf("ttl.sh/%s-fleet-test", uuid)

e2e/single-cluster/sharding_test.go

@@ -115,6 +115,7 @@ var _ = Describe("Filtering events by shard", Label("sharding"), Ordered, func()
 
 			AfterEach(func() {
 				_, _ = k.Delete("gitrepo", gitrepoName)
+				_, _ = k.Delete("ns", targetNamespace, "--wait=false")
 			})
 		})
 	}
@@ -178,6 +179,7 @@ var _ = Describe("Filtering events by shard", Label("sharding"), Ordered, func()
 
 		AfterEach(func() {
 			_, _ = k.Delete("gitrepo", gitrepoName)
+			_, _ = k.Delete("ns", targetNamespace, "--wait=false")
 		})
 	})
 })

e2e/single-cluster/single_cluster_test.go

@@ -29,6 +29,7 @@ var _ = Describe("Single Cluster Deployments", func() {
 		out, err := k.Delete("-f", testenv.AssetPath(asset))
 		Expect(err).ToNot(HaveOccurred(), out)
 
+		_, _ = k.Delete("ns", "helm-kustomize-disabled")
 	})
 
 	When("creating a gitrepo resource", func() {
@@ -37,6 +38,10 @@ var _ = Describe("Single Cluster Deployments", func() {
 				asset = "single-cluster/helm-oci.yaml"
 			})
 
+			AfterEach(func() {
+				_, _ = k.Delete("ns", "fleet-helm-oci-example")
+			})
+
 			It("deploys the helm chart", func() {
 				Eventually(func() string {
 					out, _ := k.Namespace("fleet-helm-oci-example").Get("configmaps")
@@ -63,6 +68,11 @@ var _ = Describe("Single Cluster Deployments", func() {
 				asset = "single-cluster/multiple-paths.yaml"
 			})
 
+			AfterEach(func() {
+				_, _ = k.Delete("ns", "test-fleet-mp-config")
+				_, _ = k.Delete("ns", "test-fleet-mp-service")
+			})
+
 			It("sets status fields for gitrepo on deployment", func() {
 				Eventually(func() bool {
 					out, err := k.Get("gitrepo", "multiple-paths", "-n", "fleet-local", "-o", "jsonpath='{.status.summary}'")
@@ -80,11 +90,19 @@ var _ = Describe("Single Cluster Deployments", func() {
 					ContainSubstring("multiple-paths-multiple-paths-service"),
 				))
 
-				Eventually(func() bool {
-					out, err := k.Get("bundle", "multiple-paths-multiple-paths-config", "-n", "fleet-local", "-o", "jsonpath='{.status.summary}'")
-					Expect(err).ToNot(HaveOccurred(), out)
-					return strings.Contains(out, "\"ready\":1")
-				}).Should(BeTrue())
+				Eventually(func(g Gomega) {
+					out, err := k.Get(
+						"bundle",
+						"multiple-paths-multiple-paths-config",
+						"-n",
+						"fleet-local",
+						"-o",
+						"jsonpath='{.status.summary}'",
+					)
+					g.Expect(err).ToNot(HaveOccurred(), out)
+
+					g.Expect(out).To(ContainSubstring(`"ready":1`))
+				}).Should(Succeed())
 
 				Eventually(func() bool {
 					out, err := k.Get("bundle", "multiple-paths-multiple-paths-service", "-n", "fleet-local", "-o", "jsonpath='{.status.summary}'")