Implement SkipTeardown Modes for Storage Checkups

This commit introduces a new SkipTeardown field in the configuration for storage checkups. It allows users to control whether teardown steps should be skipped, improving debugging flexibility.

The available modes are:

- SkipTeardownAlways: Always skips the teardown process.
- SkipTeardownOnFailure: Skips teardown only when a failure occurs (useful for debugging).
- SkipTeardownNever: Ensures resources are always cleaned up after a checkup run.

Signed-off-by: Alvaro Romero <[email protected]>

pkg/internal/checkup/checkup.go

@@ -780,6 +780,10 @@ func (c *Checkup) Results() status.Results {
 	return c.results
 }
 
+func (c *Checkup) Config() config.Config {
+	return c.checkupConfig
+}
+
 func (c *Checkup) checkVMIBoot(ctx context.Context, errStr *string) error {
 	log.Print("checkVMIBoot")
 	if c.goldenImagePvc == nil && c.goldenImageSnap == nil {

pkg/internal/config/config.go

@@ -36,6 +36,16 @@ const (
 	StorageClassParamName = "storageClass"
 	VMITimeoutParamName   = "vmiTimeout"
 	NumOfVMsParamName     = "numOfVMs"
+	SkipTeardownParamName = "skipTeardown"
+)
+
+// SkipTeardownMode defines the possible modes for skipping teardown.
+type SkipTeardownMode string
+
+const (
+	SkipTeardownOnFailure SkipTeardownMode = "onfailure"
+	SkipTeardownAlways    SkipTeardownMode = "always"
+	SkipTeardownNever     SkipTeardownMode = "never"
 )
 
 const (
@@ -44,8 +54,9 @@ const (
 )
 
 var (
-	ErrInvalidVMITimeout = errors.New("invalid VMI timeout")
-	ErrInvalidNumOfVMs   = errors.New("invalid number of VMIs")
+	ErrInvalidVMITimeout       = errors.New("invalid VMI timeout")
+	ErrInvalidNumOfVMs         = errors.New("invalid number of VMIs")
+	ErrInvalidSkipTeardownMode = errors.New("invalid skip teardown mode")
 )
 
 type Config struct {
@@ -54,6 +65,7 @@ type Config struct {
 	StorageClass string
 	VMITimeout   time.Duration
 	NumOfVMs     int
+	SkipTeardown SkipTeardownMode
 }
 
 func New(baseConfig kconfig.Config) (Config, error) {
@@ -89,6 +101,21 @@ func setOptionalParams(baseConfig kconfig.Config, newConfig Config) (Config, err
 		newConfig.NumOfVMs = numOfVMs
 	}
 
+	if rawVal, exists := baseConfig.Params[SkipTeardownParamName]; exists && rawVal != "" {
+		switch SkipTeardownMode(rawVal) {
+		case SkipTeardownOnFailure, SkipTeardownAlways, SkipTeardownNever:
+			newConfig.SkipTeardown = SkipTeardownMode(rawVal)
+		default:
+			skip, err := strconv.ParseBool(rawVal)
+			if err != nil {
+				return Config{}, ErrInvalidSkipTeardownMode
+			}
+			if skip {
+				newConfig.SkipTeardown = SkipTeardownAlways
+			}
+		}
+	}
+
 	return newConfig, nil
 }
 

pkg/internal/launcher/launcher.go

@@ -25,6 +25,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/kiagnose/kubevirt-storage-checkup/pkg/internal/config"
 	"github.com/kiagnose/kubevirt-storage-checkup/pkg/internal/status"
 )
 
@@ -33,6 +34,7 @@ type checkup interface {
 	Run(ctx context.Context) error
 	Teardown(ctx context.Context) error
 	Results() status.Results
+	Config() config.Config
 }
 
 type reporter interface {
@@ -74,6 +76,9 @@ func (l Launcher) Run(ctx context.Context) (runErr error) {
 	}
 
 	defer func() {
+		if l.shouldSkipTeardown(runStatus.FailureReason) {
+			return
+		}
 		if err := l.checkup.Teardown(context.Background()); err != nil {
 			runStatus.FailureReason = append(runStatus.FailureReason, err.Error())
 		}
@@ -87,6 +92,17 @@ func (l Launcher) Run(ctx context.Context) (runErr error) {
 	return nil
 }
 
+func (l Launcher) shouldSkipTeardown(failures []string) bool {
+	switch l.checkup.Config().SkipTeardown {
+	case config.SkipTeardownOnFailure:
+		return len(failures) > 0
+	case config.SkipTeardownAlways:
+		return true
+	default:
+		return false
+	}
+}
+
 func failureReason(sts status.Status) error {
 	if len(sts.FailureReason) > 0 {
 		return errors.New(strings.Join(sts.FailureReason, ", "))

pkg/internal/launcher/launcher_test.go

@@ -26,6 +26,7 @@ import (
 
 	assert "github.com/stretchr/testify/require"
 
+	"github.com/kiagnose/kubevirt-storage-checkup/pkg/internal/config"
 	"github.com/kiagnose/kubevirt-storage-checkup/pkg/internal/launcher"
 	"github.com/kiagnose/kubevirt-storage-checkup/pkg/internal/status"
 )
@@ -85,10 +86,109 @@ func TestLauncherRunShouldFailWhen(t *testing.T) {
 	}
 }
 
+func TestLauncherSkipTeardownModes(t *testing.T) {
+	// Using this custom error to now wether the teardown was called or skipped
+	teardownCalledErr := errors.New("teardown called")
+	tests := map[string]struct {
+		checkup            checkupStub
+		reporter           reporterStub
+		expectedTeardown   bool
+		expectedFailure    bool
+		expectedSkipReason config.SkipTeardownMode
+	}{
+		"skip teardown on failure, with failure": {
+			checkup: checkupStub{
+				failRun:          errRun,
+				skipTeardownMode: config.SkipTeardownOnFailure,
+				failTeardown:     teardownCalledErr,
+			},
+			reporter:           reporterStub{},
+			expectedTeardown:   false,
+			expectedFailure:    true,
+			expectedSkipReason: config.SkipTeardownOnFailure,
+		},
+		"skip teardown on failure, no failure": {
+			checkup: checkupStub{
+				skipTeardownMode: config.SkipTeardownOnFailure,
+				failTeardown:     teardownCalledErr,
+			},
+			reporter:           reporterStub{},
+			expectedTeardown:   true,
+			expectedFailure:    false,
+			expectedSkipReason: config.SkipTeardownOnFailure,
+		},
+		"always skip teardown, with failure": {
+			checkup: checkupStub{
+				failRun:          errRun,
+				skipTeardownMode: config.SkipTeardownAlways,
+				failTeardown:     teardownCalledErr,
+			},
+			reporter:           reporterStub{},
+			expectedTeardown:   false,
+			expectedFailure:    true,
+			expectedSkipReason: config.SkipTeardownAlways,
+		},
+		"always skip teardown, no failure": {
+			checkup: checkupStub{
+				skipTeardownMode: config.SkipTeardownAlways,
+				failTeardown:     teardownCalledErr,
+			},
+			reporter:           reporterStub{},
+			expectedTeardown:   false,
+			expectedFailure:    false,
+			expectedSkipReason: config.SkipTeardownAlways,
+		},
+		"never skip teardown, with failure": {
+			checkup: checkupStub{
+				failRun:          errRun,
+				skipTeardownMode: config.SkipTeardownNever,
+				failTeardown:     teardownCalledErr,
+			},
+			reporter:           reporterStub{},
+			expectedTeardown:   true,
+			expectedFailure:    true,
+			expectedSkipReason: config.SkipTeardownNever,
+		},
+		"never skip teardown, no failure": {
+			checkup: checkupStub{
+				skipTeardownMode: config.SkipTeardownNever,
+				failTeardown:     teardownCalledErr,
+			},
+			reporter:           reporterStub{},
+			expectedTeardown:   true,
+			expectedFailure:    false,
+			expectedSkipReason: config.SkipTeardownNever,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			checkup := checkupStub{
+				failSetup:        tc.checkup.failSetup,
+				failRun:          tc.checkup.failRun,
+				failTeardown:     tc.checkup.failTeardown,
+				skipTeardownMode: tc.checkup.skipTeardownMode,
+			}
+
+			testLauncher := launcher.New(checkup, &tc.reporter)
+			err := testLauncher.Run(context.Background())
+
+			if tc.expectedFailure {
+				assert.Error(t, err)
+			} else if tc.expectedTeardown {
+				assert.Equal(t, tc.checkup.failTeardown, teardownCalledErr)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
 type checkupStub struct {
-	failSetup    error
-	failRun      error
-	failTeardown error
+	failSetup        error
+	failRun          error
+	failTeardown     error
+	skipTeardownMode config.SkipTeardownMode
 }
 
 func (cs checkupStub) Setup(_ context.Context) error {
@@ -107,6 +207,12 @@ func (cs checkupStub) Results() status.Results {
 	return status.Results{}
 }
 
+func (cs checkupStub) Config() config.Config {
+	return config.Config{
+		SkipTeardown: cs.skipTeardownMode,
+	}
+}
+
 type reporterStub struct {
 	reportCalls int
 	failReport  error