Merge pull request #1717 from alexander-demicev/e2ecleanup

Print error in artifacts collection instead of failing the suite

Author: alexander-demicev

test/e2e/specs/import_gitops.go

@@ -375,21 +375,15 @@ func CreateUsingGitOpsSpec(ctx context.Context, inputGetter func() CreateUsingGi
 	AfterEach(func() {
 		By(fmt.Sprintf("Collecting artifacts for %s/%s", input.ClusterName, specName))
 
-		err := testenv.CollectArtifacts(ctx, testenv.CollectArtifactsInput{
+		testenv.TryCollectArtifacts(ctx, testenv.CollectArtifactsInput{
 			Path:                    input.ClusterName + "-bootstrap-" + specName,
 			BootstrapKubeconfigPath: input.BootstrapClusterProxy.GetKubeconfigPath(),
 		})
-		if err != nil {
-			log.FromContext(ctx).Error(err, "failed to collect artifacts for the bootstrap cluster")
-		}
 
-		err = testenv.CollectArtifacts(ctx, testenv.CollectArtifactsInput{
+		testenv.TryCollectArtifacts(ctx, testenv.CollectArtifactsInput{
 			KubeconfigPath: originalKubeconfig.TempFilePath,
 			Path:           input.ClusterName + "-workload-" + specName,
 		})
-		if err != nil {
-			log.FromContext(ctx).Error(err, "failed to collect artifacts for the child cluster")
-		}
 
 		// If SKIP_RESOURCE_CLEANUP=true & if the SkipDeletionTest is true, all the resources should stay as they are,
 		// nothing should be deleted. If SkipDeletionTest is true, deleting the git repo will delete the clusters too.

test/e2e/suites/v2prov/v2prov_test.go

@@ -30,7 +30,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/envtest/komega"
-	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	provisioningv1 "github.com/rancher/turtles/api/rancher/provisioning/v1"
 	"github.com/rancher/turtles/test/e2e"
@@ -171,24 +170,18 @@ var _ = Describe("[v2prov] [Azure] Creating a cluster with v2prov should still w
 	AfterEach(func() {
 		By(fmt.Sprintf("Collecting artifacts for %s/%s", clusterName, specName))
 
-		err := testenv.CollectArtifacts(ctx, testenv.CollectArtifactsInput{
+		testenv.TryCollectArtifacts(ctx, testenv.CollectArtifactsInput{
 			Path:                    clusterName + "-bootstrap-" + specName,
 			BootstrapKubeconfigPath: bootstrapClusterProxy.GetKubeconfigPath(),
 		})
-		if err != nil {
-			log.FromContext(ctx).Error(err, "failed to collect artifacts for the bootstrap cluster")
-		}
 
-		err = testenv.CollectArtifacts(ctx, testenv.CollectArtifactsInput{
+		testenv.TryCollectArtifacts(ctx, testenv.CollectArtifactsInput{
 			KubeconfigPath: rancherKubeconfig.TempFilePath,
 			Path:           clusterName + "-workload-" + specName,
 		})
-		if err != nil {
-			log.FromContext(ctx).Error(err, "failed to collect artifacts for the child cluster")
-		}
 
 		By("Deleting cluster from Rancher")
-		err = bootstrapClusterProxy.GetClient().Delete(ctx, rancherCluster)
+		err := bootstrapClusterProxy.GetClient().Delete(ctx, rancherCluster)
 		Expect(err).NotTo(HaveOccurred(), "Failed to delete rancher cluster")
 
 		By("Waiting for the rancher cluster record to be removed")

test/testenv/cleanup.go

@@ -122,9 +122,23 @@ func CollectArtifacts(ctx context.Context, input CollectArtifactsInput) error {
 
 	log.Info("Running kubectl:", "command", strings.Join(aargs, " "))
 	err := cmd.Run()
-	log.Info("stderr:", "stderr", stderr.Bytes())
-	log.Info("stdout:", "stdout", stdout.Bytes())
-	return err
+
+	stderrStr := stderr.String()
+	stdoutStr := stdout.String()
+	if strings.TrimSpace(stderrStr) != "" {
+		log.Info("stderr:", "stderr", stderrStr)
+	}
+	if strings.TrimSpace(stdoutStr) != "" {
+		log.Info("stdout:", "stdout", stdoutStr)
+	}
+
+	if err != nil {
+		if exitErr, ok := err.(*exec.ExitError); ok {
+			return fmt.Errorf("artifact collection failed, exit code: %d, error %w", exitErr.ExitCode(), err)
+		}
+		return fmt.Errorf("artifact collection failed, error: %w", err)
+	}
+	return nil
 }
 
 func DumpBootstrapCluster(ctx context.Context) {
@@ -134,3 +148,11 @@ func DumpBootstrapCluster(ctx context.Context) {
 		return
 	}
 }
+
+// TryCollectArtifacts calls CollectArtifacts and logs any error instead of returning it.
+// This is useful in AfterEach/Defer flows where artifact collection failures should not fail the suite.
+func TryCollectArtifacts(ctx context.Context, input CollectArtifactsInput) {
+	if err := CollectArtifacts(ctx, input); err != nil {
+		log.FromContext(ctx).Error(err, "artifact collection failed", "path", input.Path)
+	}
+}