chore: fix check kb upgrade release status in 0.9 (#493)

Author: yipeng1030

addons

@@ -28,7 +28,6 @@ import (
 	"github.com/hashicorp/go-version"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
-	"helm.sh/helm/v3/pkg/release"
 	appsv1 "k8s.io/api/apps/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -103,26 +102,12 @@ func (o *InstallOptions) Upgrade() error {
 		}
 		o.HelmCfg.SetNamespace(ns)
 	}
-
-	// check helm release status
+	// get helm release
 	KBRelease, err := helm.GetHelmRelease(o.HelmCfg, types.KubeBlocksChartName)
 	if err != nil {
 		return fmt.Errorf("failed to get Helm release: %v", err)
 	}
 
-	// intercept status of pending, unknown, uninstalling and uninstalled.
-	var status release.Status
-	if KBRelease != nil && KBRelease.Info != nil {
-		status = KBRelease.Info.Status
-	} else {
-		return fmt.Errorf("failed to get Helm release status: release or release info is nil")
-	}
-	if status.IsPending() {
-		return fmt.Errorf("helm release status is %s. Please wait until the release status changes to ‘deployed’ before upgrading KubeBlocks", status.String())
-	} else if status != release.StatusDeployed && status != release.StatusFailed && status != release.StatusSuperseded {
-		return fmt.Errorf("helm release status is %s. Please fix the release before upgrading KubeBlocks", status.String())
-	}
-
 	o.Version = util.TrimVersionPrefix(o.Version)
 	// check flags already been set
 	if o.Version == "" && helm.ValueOptsIsEmpty(&o.ValueOpts) {

pkg/cmd/kubeblocks/upgrade.go

@@ -22,11 +22,10 @@ package kubeblocks
 import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/spf13/cobra"
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/release"
-
-	"github.com/spf13/cobra"
 	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/cli-runtime/pkg/genericiooptions"
 	clientfake "k8s.io/client-go/rest/fake"
@@ -176,77 +175,4 @@ var _ = Describe("kubeblocks upgrade", func() {
 		})
 	})
 
-	Context("upgrade from different status", func() {
-		BeforeEach(func() {
-			streams, _, _, _ = genericiooptions.NewTestIOStreams()
-			tf = cmdtesting.NewTestFactory().WithNamespace(namespace)
-			tf.Client = &clientfake.RESTClient{}
-			cfg = helm.NewFakeConfig(namespace)
-			actionCfg, _ = helm.NewActionConfig(cfg)
-		})
-
-		AfterEach(func() {
-			helm.ResetFakeActionConfig()
-			tf.Cleanup()
-		})
-
-		mockKubeBlocksDeploy := func() *appsv1.Deployment {
-			deploy := &appsv1.Deployment{}
-			deploy.SetLabels(map[string]string{
-				"app.kubernetes.io/component": "apps",
-				"app.kubernetes.io/name":      types.KubeBlocksChartName,
-				"app.kubernetes.io/version":   "0.3.0",
-			})
-			return deploy
-		}
-		It("run upgrade", func() {
-			testCase := []struct {
-				status      release.Status
-				checkResult bool
-			}{
-				{release.StatusDeployed, true},
-				{release.StatusSuperseded, true},
-				{release.StatusFailed, true},
-				{release.StatusUnknown, false},
-				{release.StatusUninstalled, false},
-				{release.StatusUninstalling, false},
-				{release.StatusPendingInstall, false},
-				{release.StatusPendingUpgrade, false},
-				{release.StatusPendingRollback, false},
-			}
-
-			for i := range testCase {
-				actionCfg, _ = helm.NewActionConfig(cfg)
-				err := actionCfg.Releases.Create(&release.Release{
-					Name:      testing.KubeBlocksChartName,
-					Namespace: namespace,
-					Version:   1,
-					Info: &release.Info{
-						Status: testCase[i].status,
-					},
-					Chart: &chart.Chart{},
-				})
-				Expect(err).Should(BeNil())
-				o := &InstallOptions{
-					Options: Options{
-						IOStreams: streams,
-						HelmCfg:   cfg,
-						Namespace: "default",
-						Client:    testing.FakeClientSet(mockKubeBlocksDeploy()),
-						Dynamic:   testing.FakeDynamicClient(),
-					},
-					Version: version.DefaultKubeBlocksVersion,
-					Check:   false,
-				}
-				if testCase[i].checkResult {
-					Expect(o.Upgrade()).Should(Succeed())
-				} else {
-					Expect(o.Upgrade()).Should(HaveOccurred())
-				}
-				helm.ResetFakeActionConfig()
-			}
-
-		})
-	})
-
 })

pkg/cmd/kubeblocks/upgrade_test.go

@@ -33,6 +33,7 @@ import (
 // Implementing errors should be more friendly to downstream handlers
 
 var ErrReleaseNotDeployed = fmt.Errorf("release: not in deployed status")
+var ErrReleaseNotReadyForUpgrade = fmt.Errorf("release: not in deployed, failed or superseded status")
 
 func ReleaseNotFound(err error) bool {
 	if err == nil {

pkg/util/helm/errors.go

@@ -156,6 +156,30 @@ func RemoveRepo(r *repo.Entry) error {
 	return nil
 }
 
+// GetInstalledForUpgrade gets helm package release info and check the status.
+func (i *InstallOpts) GetInstalledForUpgrade(cfg *action.Configuration) (*release.Release, error) {
+	res, err := action.NewGet(cfg).Run(i.Name)
+	if err != nil {
+		return nil, err
+	}
+	if res == nil {
+		return nil, driver.ErrReleaseNotFound
+	}
+	// intercept status of pending, unknown, uninstalling and uninstalled.
+	var status release.Status
+	if res.Info != nil {
+		status = res.Info.Status
+	} else {
+		return nil, fmt.Errorf("failed to get Helm release status: release or release info is nil")
+	}
+	if status.IsPending() {
+		return nil, errors.Wrapf(ErrReleaseNotReadyForUpgrade, "helm release status is %s. Please wait until the release status changes to ‘deployed’ before upgrading KubeBlocks", status.String())
+	} else if status != release.StatusDeployed && status != release.StatusFailed && status != release.StatusSuperseded {
+		return nil, errors.Wrapf(ErrReleaseNotReadyForUpgrade, "helm release status is %s. Please fix the release before upgrading KubeBlocks,uninstall and install kubeblocks could be a way to fix error", status.String())
+	}
+	return res, nil
+}
+
 // GetInstalled gets helm package release info if installed.
 func (i *InstallOpts) GetInstalled(cfg *action.Configuration) (*release.Release, error) {
 	res, err := action.NewGet(cfg).Run(i.Name)
@@ -510,7 +534,7 @@ func (i *InstallOpts) Upgrade(cfg *Config) error {
 }
 
 func (i *InstallOpts) tryUpgrade(cfg *action.Configuration) (*release.Release, error) {
-	installed, err := i.GetInstalled(cfg)
+	installed, err := i.GetInstalledForUpgrade(cfg)
 	if err != nil {
 		return nil, err
 	}

pkg/util/helm/helm.go

@@ -132,34 +132,42 @@ var _ = Describe("helm util", func() {
 			Expect(o.Uninstall(cfg)).Should(HaveOccurred()) // release not found
 		})
 
-		It("should fail at fetching charts when release is already deployed", func() {
-			err := actionCfg.Releases.Create(&release.Release{
-				Name:    o.Name,
-				Version: 1,
-				Info: &release.Info{
-					Status: release.StatusDeployed,
-				},
-				Chart: &chart.Chart{},
-			})
-			Expect(err).Should(BeNil())
-			_, err = o.tryUpgrade(actionCfg)
-			Expect(err).Should(HaveOccurred())                // failed at fetching charts
-			Expect(o.tryUninstall(actionCfg)).Should(BeNil()) // release exists
-		})
+		It("should fail when status is not one of deployed, failed and superseded.", func() {
+			testCase := []struct {
+				status      release.Status
+				checkResult bool
+			}{
+				// deployed, failed and superseded
+				{release.StatusDeployed, true},
+				{release.StatusSuperseded, true},
+				{release.StatusFailed, true},
+				// others
+				{release.StatusUnknown, false},
+				{release.StatusUninstalled, false},
+				{release.StatusUninstalling, false},
+				{release.StatusPendingInstall, false},
+				{release.StatusPendingUpgrade, false},
+				{release.StatusPendingRollback, false},
+			}
 
-		It("should fail when chart is already deployed", func() {
-			err := actionCfg.Releases.Create(&release.Release{
-				Name:    o.Name,
-				Version: 1,
-				Info: &release.Info{
-					Status: release.StatusFailed,
-				},
-				Chart: &chart.Chart{},
-			})
-			Expect(err).Should(BeNil())
-			_, err = o.tryUpgrade(actionCfg)
-			Expect(errors.Is(err, ErrReleaseNotDeployed)).Should(BeTrue())
-			Expect(o.tryUninstall(actionCfg)).Should(BeNil()) // release exists
+			for i := range testCase {
+				err := actionCfg.Releases.Create(&release.Release{
+					Name:    o.Name,
+					Version: 1,
+					Info: &release.Info{
+						Status: testCase[i].status,
+					},
+					Chart: &chart.Chart{},
+				})
+				Expect(err).Should(BeNil())
+				_, err = o.tryUpgrade(actionCfg)
+				if testCase[i].checkResult {
+					Expect(errors.Is(err, ErrReleaseNotReadyForUpgrade)).Should(BeFalse())
+				} else {
+					Expect(errors.Is(err, ErrReleaseNotReadyForUpgrade)).Should(BeTrue())
+				}
+				Expect(o.tryUninstall(actionCfg)).Should(BeNil()) // release exists
+			}
 		})
 	})