From 30bbfe31e4f401b4dc4fc18578c0106f2110453e Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Thu, 7 Mar 2024 09:11:27 +0100 Subject: [PATCH] Improve Helm uninstallation cleanup --- .../cloud_slack_dev_e2e_test.go | 21 ++++------- test/e2e/bots_test.go | 23 ++++++------ test/helmx/helm_helpers.go | 36 +++++++++++++++++++ test/helmx/redact_test.go | 1 - 4 files changed, 53 insertions(+), 28 deletions(-) diff --git a/test/cloud-slack-dev-e2e/cloud_slack_dev_e2e_test.go b/test/cloud-slack-dev-e2e/cloud_slack_dev_e2e_test.go index ca7e1fe41..35d23e5a5 100644 --- a/test/cloud-slack-dev-e2e/cloud_slack_dev_e2e_test.go +++ b/test/cloud-slack-dev-e2e/cloud_slack_dev_e2e_test.go @@ -11,6 +11,7 @@ import ( "os" "path/filepath" "strings" + "sync/atomic" "testing" "time" @@ -90,7 +91,8 @@ func TestCloudSlackE2E(t *testing.T) { cfg.Slack.Tester.CloudBasedTestEnabled = false // override property used only in the Cloud Slack E2E tests authHeaderValue := "" - helmChartUninstalled := false + var botkubeDeploymentUninstalled atomic.Bool + botkubeDeploymentUninstalled.Store(true) // not yet installed gqlEndpoint := fmt.Sprintf("%s/%s", cfg.BotkubeCloud.APIBaseURL, cfg.BotkubeCloud.APIGraphQLEndpoint) if cfg.ScreenshotsDir != "" { @@ -319,18 +321,8 @@ func TestCloudSlackE2E(t *testing.T) { t.Log("Creating deployment...") deployment := gqlCli.MustCreateBasicDeploymentWithCloudSlack(t, channel.Name(), slackWorkspace.TeamID, channel.Name()) t.Cleanup(func() { - // We have a glitch on backend side and the logic below is a workaround for that. - // Tl;dr uninstalling Helm chart reports "DISCONNECTED" status, and deplyment deletion reports "DELETED" status. - // If we do these two things too quickly, we'll run into resource version mismatch in repository logic. - // Read more here: https://github.com/kubeshop/botkube-cloud/pull/486#issuecomment-1604333794 - - for !helmChartUninstalled { - t.Log("Waiting for Helm chart uninstallation, in order to proceed with deleting the first deployment...") - time.Sleep(1 * time.Second) - } - - t.Log("Helm chart uninstalled. Waiting a bit...") - time.Sleep(3 * time.Second) // ugly, but at least we will be pretty sure we won't run into the resource version mismatch + err := helmx.WaitForUninstallation(context.Background(), t, &botkubeDeploymentUninstalled) + assert.NoError(t, err) t.Log("Deleting first deployment...") gqlCli.MustDeleteDeployment(t, graphql.ID(deployment.ID)) @@ -343,6 +335,7 @@ func TestCloudSlackE2E(t *testing.T) { gqlCli.MustDeleteDeployment(t, graphql.ID(deployment2.ID)) }) + botkubeDeploymentUninstalled.Store(false) // about to be installed params := helmx.InstallChartParams{ RepoURL: "https://storage.googleapis.com/botkube-latest-main-charts", RepoName: "botkube", @@ -355,7 +348,7 @@ func TestCloudSlackE2E(t *testing.T) { t.Cleanup(func() { t.Log("Uninstalling Helm chart...") helmInstallCallback(t) - helmChartUninstalled = true + botkubeDeploymentUninstalled.Store(true) }) t.Log("Waiting for help message...") diff --git a/test/e2e/bots_test.go b/test/e2e/bots_test.go index 8da5ab437..23a29e393 100644 --- a/test/e2e/bots_test.go +++ b/test/e2e/bots_test.go @@ -10,10 +10,13 @@ import ( "regexp" "strconv" "strings" + "sync/atomic" "testing" "time" "unicode" + "botkube.io/botube/test/helmx" + "botkube.io/botube/test/botkubex" "botkube.io/botube/test/commplatform" "botkube.io/botube/test/diff" @@ -201,7 +204,9 @@ func runBotTest(t *testing.T, deployEnvSecondaryChannelIDName, deployEnvRbacChannelIDName string, ) { - botkubeDeploymentUninstalled := false + var botkubeDeploymentUninstalled atomic.Bool + botkubeDeploymentUninstalled.Store(true) // not yet installed + t.Logf("Creating API client with provided token for %s...", driverType) botDriver, err := newBotDriver(appCfg, driverType) require.NoError(t, err) @@ -259,22 +264,14 @@ func runBotTest(t *testing.T, gqlCli.MustCreateAlias(t, alias[0], alias[1], alias[2], deployment.ID) } t.Cleanup(func() { - // We have a glitch on backend side and the logic below is a workaround for that. - // Tl;dr uninstalling Helm chart reports "DISCONNECTED" status, and deployment deletion reports "DELETED" status. - // If we do these two things too quickly, we'll run into resource version mismatch in repository logic. - // Read more here: https://github.com/kubeshop/botkube-cloud/pull/486#issuecomment-1604333794 - for !botkubeDeploymentUninstalled { - t.Log("Waiting for Helm chart uninstallation, in order to proceed with deleting Botkube Cloud instance...") - time.Sleep(1 * time.Second) - } - - t.Log("Helm chart uninstalled. Waiting a bit...") - time.Sleep(3 * time.Second) // ugly, but at least we will be pretty sure we won't run into the resource version mismatch + err := helmx.WaitForUninstallation(context.Background(), t, &botkubeDeploymentUninstalled) + assert.NoError(t, err) t.Log("Deleting Botkube Cloud instance...") gqlCli.MustDeleteDeployment(t, graphql.ID(deployment.ID)) }) + botkubeDeploymentUninstalled.Store(false) // about to be installed err = botkubex.Install(t, botkubex.InstallParams{ BinaryPath: appCfg.ConfigProvider.BotkubeCliBinaryPath, HelmRepoDirectory: appCfg.ConfigProvider.HelmRepoDirectory, @@ -291,7 +288,7 @@ func runBotTest(t *testing.T, t.Cleanup(func() { t.Log("Uninstalling Helm chart...") botkubex.Uninstall(t, appCfg.ConfigProvider.BotkubeCliBinaryPath) - botkubeDeploymentUninstalled = true + botkubeDeploymentUninstalled.Store(true) }) } diff --git a/test/helmx/helm_helpers.go b/test/helmx/helm_helpers.go index 9b6c59802..ea276cbb0 100644 --- a/test/helmx/helm_helpers.go +++ b/test/helmx/helm_helpers.go @@ -1,12 +1,17 @@ package helmx import ( + "context" "encoding/json" "fmt" "os/exec" "regexp" "strings" + "sync/atomic" "testing" + "time" + + "k8s.io/apimachinery/pkg/util/wait" "github.com/stretchr/testify/require" ) @@ -108,3 +113,34 @@ func redactAPIKey(in []string) []string { } return dst } + +const ( + uninstallPollInterval = 1 * time.Second + uninstallTimeout = 30 * time.Second +) + +// WaitForUninstallation waits until a Helm chart is uninstalled, based on the atomic value. +// It's a workaround for the Helm chart uninstallation issue. +// We have a glitch on backend side and the logic below is a workaround for that. +// Tl;dr uninstalling Helm chart reports "DISCONNECTED" status, and deployment deletion reports "DELETED" status. +// If we do these two things too quickly, we'll run into resource version mismatch in repository logic. +// Read more here: https://github.com/kubeshop/botkube-cloud/pull/486#issuecomment-1604333794 +func WaitForUninstallation(ctx context.Context, t *testing.T, alreadyUninstalled *atomic.Bool) error { + t.Helper() + t.Log("Waiting for Helm chart uninstallation, in order to proceed with deleting Botkube Cloud instance...") + err := wait.PollUntilContextTimeout(ctx, uninstallPollInterval, uninstallTimeout, false, func(ctx context.Context) (done bool, err error) { + return alreadyUninstalled.Load(), nil + }) + waitInterrupted := wait.Interrupted(err) + if err != nil && !waitInterrupted { + return err + } + + if waitInterrupted { + t.Log("Force uninstalling the Helm chart after a wait timeout") + } + + t.Log("Waiting a bit more...") + time.Sleep(3 * time.Second) // ugly, but at least we will be pretty sure we won't run into the resource version mismatch + return nil +} diff --git a/test/helmx/redact_test.go b/test/helmx/redact_test.go index 14701cfbe..34064d8d1 100644 --- a/test/helmx/redact_test.go +++ b/test/helmx/redact_test.go @@ -7,7 +7,6 @@ import ( ) func TestRedactAPIKey(t *testing.T) { - tests := []struct { name string input []string