From b1818c6f5fa63143b7f8c1f7e4426c6ab3e31c5c Mon Sep 17 00:00:00 2001
From: Nan Yu <nanyu@google.com>
Date: Fri, 8 Sep 2023 19:39:34 +0000
Subject: [PATCH] Merge reset and printTestLogs into a single cleanup block
 (#861)

Previously, printTestLogs is in its own cleanup block to catch fatal
errors from the reset block.

Now we decided to reset after `printTestLogs`, so we can combine them
into a single block for simplicity.

It also prints test logs if reset fails.

The `testPods` function is also renamed to `listAndDescribePods` to
print the pod describe info, which shows the reason for pod restarts.
---
 e2e/nomostest/new.go |  8 ++++----
 e2e/nomostest/nt.go  | 19 ++++++++++++-------
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/e2e/nomostest/new.go b/e2e/nomostest/new.go
index a429389dec..353f2735f9 100644
--- a/e2e/nomostest/new.go
+++ b/e2e/nomostest/new.go
@@ -181,23 +181,23 @@ func SharedTestEnv(t nomostesting.NTB, opts *ntopts.New) *NT {
 
 	nt.detectGKEAutopilot(opts.SkipAutopilot)
 
-	// Print container logs in its own cleanup block to catch fatal errors from
-	// tests and test setup (including resetSyncedRepos).
 	t.Cleanup(func() {
 		if t.Failed() {
 			nt.printTestLogs()
 		}
-	})
-	t.Cleanup(func() {
+		// Reset after `printTestLogs` to catch pods status and logs before resetting.
 		nt.T.Log("[RESET] SharedTestEnv after test")
 		if err := Reset(nt); err != nil {
 			nt.T.Errorf("[RESET] Failed to reset test environment: %v", err)
+			// Print test logs if reset fails.
+			nt.printTestLogs()
 		}
 	})
 
 	nt.T.Log("[RESET] SharedTestEnv before test")
 	if err := Reset(nt); err != nil {
 		nt.T.Fatalf("[RESET] Failed to reset test environment: %v", err)
+		// No need to print test logs here because the cleanup block will print them.
 	}
 	// a previous e2e test may stop the Config Sync webhook, so always call `installWebhook` here to make sure the test starts
 	// with the webhook enabled.
diff --git a/e2e/nomostest/nt.go b/e2e/nomostest/nt.go
index 1242055177..512840915a 100644
--- a/e2e/nomostest/nt.go
+++ b/e2e/nomostest/nt.go
@@ -467,11 +467,7 @@ func (nt *NT) printTestLogs() {
 	nt.testLogs(true)
 	nt.T.Log("[CLEANUP] Printing test logs for running pods")
 	for _, ns := range CSNamespaces {
-		nt.testPods(ns)
-	}
-	nt.T.Log("[CLEANUP] Describing not-ready pods")
-	for _, ns := range CSNamespaces {
-		nt.describeNotRunningTestPods(ns)
+		nt.listAndDescribePods(ns)
 	}
 }
 
@@ -499,12 +495,14 @@ func (nt *NT) testLogs(previousPodLog bool) {
 	}
 }
 
-// testPods prints the output of `kubectl get pods`, which includes a 'RESTARTS' column
+// listAndDescribePods prints the output of `kubectl get pods`, which includes a 'RESTARTS' column
 // indicating how many times each pod has restarted. If a pod has restarted, the following
 // two commands can be used to get more information:
 //  1. kubectl get pods -n config-management-system -o yaml
 //  2. kubectl logs deployment/<deploy-name> <container-name> -n config-management-system -p
-func (nt *NT) testPods(ns string) {
+//
+// It also prints the output of `kubectl describe pods` for PodSpec.
+func (nt *NT) listAndDescribePods(ns string) {
 	out, err := nt.Shell.Kubectl("get", "pods", "-n", ns)
 	// Print a standardized header before each printed log to make ctrl+F-ing the
 	// log you want easier.
@@ -512,6 +510,13 @@ func (nt *NT) testPods(ns string) {
 	if err != nil {
 		nt.T.Log("error running `kubectl get pods`:", err)
 	}
+	out, err = nt.Shell.Kubectl("describe", "pods", "-n", ns)
+	// Print a standardized header before each printed log to make ctrl+F-ing the
+	// log you want easier.
+	nt.T.Logf("kubectl describe pods -n %s: \n%s", ns, string(out))
+	if err != nil {
+		nt.T.Log("error running `kubectl describe pods`:", err)
+	}
 }
 
 func (nt *NT) describeNotRunningTestPods(namespace string) {