From 52802d9f87116cf72a79ad1f7e9fc4295447df1f Mon Sep 17 00:00:00 2001 From: Alvaro Romero Date: Mon, 16 Sep 2024 12:01:45 +0200 Subject: [PATCH] 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 --- pkg/internal/checkup/checkup.go | 4 + pkg/internal/config/config.go | 31 ++++++- pkg/internal/launcher/launcher.go | 16 ++++ pkg/internal/launcher/launcher_test.go | 112 ++++++++++++++++++++++++- 4 files changed, 158 insertions(+), 5 deletions(-) diff --git a/pkg/internal/checkup/checkup.go b/pkg/internal/checkup/checkup.go index c792db28..4888db58 100644 --- a/pkg/internal/checkup/checkup.go +++ b/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 { diff --git a/pkg/internal/config/config.go b/pkg/internal/config/config.go index 05bf29e9..56fa4f69 100644 --- a/pkg/internal/config/config.go +++ b/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 } diff --git a/pkg/internal/launcher/launcher.go b/pkg/internal/launcher/launcher.go index ff9c0b8d..fef0f287 100644 --- a/pkg/internal/launcher/launcher.go +++ b/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, ", ")) diff --git a/pkg/internal/launcher/launcher_test.go b/pkg/internal/launcher/launcher_test.go index cf013357..ed795d22 100644 --- a/pkg/internal/launcher/launcher_test.go +++ b/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