diff --git a/changelog/fragments/no-pod-left-behind.yaml b/changelog/fragments/no-pod-left-behind.yaml new file mode 100644 index 00000000000..120be32a122 --- /dev/null +++ b/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 diff --git a/internal/scorecard/run_test.go b/internal/scorecard/run_test.go index 52648b64156..8d9af799fd4 100644 --- a/internal/scorecard/run_test.go +++ b/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 } - }) } diff --git a/internal/scorecard/scorecard.go b/internal/scorecard/scorecard.go index f1c9f876842..38a3243c40b 100644 --- a/internal/scorecard/scorecard.go +++ b/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) { diff --git a/internal/scorecard/testpod.go b/internal/scorecard/testpod.go index 3ccf5033e94..9849e81b583 100644 --- a/internal/scorecard/testpod.go +++ b/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 }