internal/scorecard: fix cleanup skip on timeout bug (#3526)

changelog/fragments/no-pod-left-behind.yaml

@@ -0,0 +1,5 @@
+entries:
+  - description: >
+      The `scorecard` subcommand now removes existing pods if the `--wait-time` deadline is exceeded
+      and `--skip-cleanup=false` (the default). Fixes [#3419](https://github.com/operator-framework/operator-sdk/issues/3419).
+    kind: bugfix

internal/scorecard/run_test.go

@@ -27,28 +27,30 @@ import (
 )
 
 // TODO(joelanford): rewrite to use ginkgo/gomega
-func TestRunTests(t *testing.T) {
+func TestRun(t *testing.T) {
 	cases := []struct {
 		name            string
 		configPathValue string
 		selector        string
-		wantError       bool
+		timeout         time.Duration
+		wantedError     error
 		testRunner      FakeTestRunner
 		expectedState   v1alpha3.State
 	}{
 		{
-			name:            "should execute 1 fake test",
+			name:            "should execute 1 fake test successfully",
 			configPathValue: "testdata/bundle",
 			selector:        "suite=basic",
-			wantError:       false,
+			timeout:         time.Second * 7,
 			testRunner:      FakeTestRunner{},
 			expectedState:   v1alpha3.PassState,
 		},
 		{
-			name:            "should execute 1 fake test",
+			name:            "should fail to execute 1 test with short timeout",
 			configPathValue: "testdata/bundle",
 			selector:        "suite=basic",
-			wantError:       false,
+			timeout:         time.Second * 0,
+			wantedError:     context.DeadlineExceeded,
 			testRunner:      FakeTestRunner{},
 			expectedState:   v1alpha3.PassState,
 		},
@@ -79,24 +81,25 @@ func TestRunTests(t *testing.T) {
 			c.testRunner.TestStatus = &mockStatus
 			o.TestRunner = c.testRunner
 
-			ctx, cancel := context.WithTimeout(context.Background(), time.Duration(7*time.Second))
+			ctx, cancel := context.WithTimeout(context.Background(), c.timeout)
 			defer cancel()
-			var scorecardOutput v1alpha3.TestList
-			scorecardOutput, err = o.Run(ctx)
 
-			if scorecardOutput.Items[0].Status.Results[0].State != c.expectedState {
-				t.Fatalf("Wanted state %v, got %v", c.expectedState, scorecardOutput.Items[0].Status.Results[0].State)
-			}
-
-			if err == nil && c.wantError {
-				t.Fatalf("Wanted error but got no error")
+			scorecardOutput, err := o.Run(ctx)
+			if err == nil {
+				if c.wantedError != nil {
+					t.Errorf("Wanted error %s but got no error", c.wantedError)
+					return
+				}
+				if scorecardOutput.Items[0].Status.Results[0].State != c.expectedState {
+					t.Errorf("Wanted state %v, got %v", c.expectedState, scorecardOutput.Items[0].Status.Results[0].State)
+				}
 			} else if err != nil {
-				if !c.wantError {
-					t.Fatalf("Wanted result but got error: %v", err)
+				if c.wantedError == nil {
+					t.Errorf("Wanted result but got error %v", err)
+				} else if !errors.Is(err, c.wantedError) {
+					t.Errorf("Wanted error %v but got error %v", c.wantedError, err)
 				}
-				return
 			}
-
 		})
 
 	}

internal/scorecard/scorecard.go

@@ -59,9 +59,12 @@ type FakeTestRunner struct {
 	Error      error
 }
 
+// cleanupTimeout is the time given to clean up resources, regardless of how long ctx's deadline is.
+var cleanupTimeout = time.Second * 30
+
 // Run executes the scorecard tests as configured
-func (o Scorecard) Run(ctx context.Context) (v1alpha3.TestList, error) {
-	testOutput := v1alpha3.NewTestList()
+func (o Scorecard) Run(ctx context.Context) (testOutput v1alpha3.TestList, err error) {
+	testOutput = v1alpha3.NewTestList()
 
 	if err := o.TestRunner.Initialize(ctx); err != nil {
 		return testOutput, err
@@ -85,12 +88,23 @@ func (o Scorecard) Run(ctx context.Context) (v1alpha3.TestList, error) {
 		}
 	}
 
+	// Get timeout error, if any, before calling Cleanup() so deletes don't cause a timeout.
+	select {
+	case <-ctx.Done():
+		err = ctx.Err()
+	default:
+	}
+
 	if !o.SkipCleanup {
-		if err := o.TestRunner.Cleanup(ctx); err != nil {
+		// Use a separate context for cleanup, which needs to run regardless of a prior timeout.
+		clctx, cancel := context.WithTimeout(context.Background(), cleanupTimeout)
+		defer cancel()
+		if err := o.TestRunner.Cleanup(clctx); err != nil {
 			return testOutput, err
 		}
 	}
-	return testOutput, nil
+
+	return testOutput, err
 }
 
 func (o Scorecard) runStageParallel(ctx context.Context, tests []v1alpha3.TestConfiguration, results chan<- v1alpha3.Test) {

internal/scorecard/testpod.go

@@ -132,9 +132,6 @@ func getPodLog(ctx context.Context, client kubernetes.Interface, pod *v1.Pod) ([
 
 	buf := new(bytes.Buffer)
 	_, err = io.Copy(buf, podLogs)
-	if err != nil {
-		return nil, err
-	}
 	return buf.Bytes(), err
 }
 
@@ -145,7 +142,7 @@ func (r PodTestRunner) deletePods(ctx context.Context, configMapName string) err
 	lo := metav1.ListOptions{LabelSelector: selector}
 	err := r.Client.CoreV1().Pods(r.Namespace).DeleteCollection(ctx, do, lo)
 	if err != nil {
-		return fmt.Errorf("error deleting pods selector %s %w", selector, err)
+		return fmt.Errorf("error deleting pods (label selector %q): %w", selector, err)
 	}
 	return nil
 }