V2 upgrades without manager #1757

sgalsaleh · 2025-01-29T00:01:50Z

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR require a test?

Does this PR require a release note?

Does this PR require documentation?

github-actions · 2025-01-29T00:17:53Z

This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID.

Online Installer:

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci/appver-dev-85c9c0c" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Airgap Installer (may take a few minutes before the airgap bundle is built):

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci-airgap/appver-dev-85c9c0c?airgap=true" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Happy debugging!

* remove TestInstallWithoutEmbed * use new install * check postupgrade override labels * fix extension upgrade messages * further improve wording * fix nil map issue when upgrading operator * f * Update pkg/extensions/upgrade.go * Update pkg/extensions/upgrade.go --------- Co-authored-by: Salah Al Saleh <sg.alsaleh@gmail.com>

* add comments to ignore deprecated BuiltinConfigs field * fix linting

* chore: no spinner if no terminal (#1778) * fix panic in TestHostPreflightCustomSpec * fix compile * require license for install2 * f * f * use single-node-install.sh in LAM e2e test * improve sudo test logging * ??? * see if it's just one thing failing in sudo test * use join2 command in 'node join'. Hide 'node' commands * improve how we are requiring the license flag for installation --------- Co-authored-by: Ethan Mosbaugh <ethan@replicated.com>

* feat(v2): fix e2e TestVersion * f * f

emosbaugh · 2025-02-04T19:41:57Z

cmd/installer/cli/install2.go

-		Short:  fmt.Sprintf("Experimental installer for %s", name),
-		Hidden: true,
+		Use:   "install",
+		Short: fmt.Sprintf("Experimental installer for %s", name),


emosbaugh · 2025-02-04T19:42:15Z

cmd/installer/cli/install2.go

@@ -214,6 +220,8 @@ func preRunInstall2(cmd *cobra.Command, flags *Install2CmdFlags) error {

 	flags.isAirgap = flags.airgapBundle != ""

+	flags.isAirgap = flags.airgapBundle != ""


this is duplicated

emosbaugh · 2025-02-04T19:48:38Z

pkg/extensions/upgrade.go

+	return nil
+}
+
+func handleExtensionInstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) (finalErr error) {


Suggested change

func handleExtensionInstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) (finalErr error) {

func handleExtensionInstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) error {

emosbaugh · 2025-02-04T19:49:02Z

pkg/extensions/upgrade.go

+	})
+}
+
+func handleExtensionUpgrade(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart, needsUpgrade bool) (finalErr error) {


Suggested change

func handleExtensionUpgrade(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart, needsUpgrade bool) (finalErr error) {

func handleExtensionUpgrade(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart, needsUpgrade bool) error {

emosbaugh · 2025-02-04T19:49:19Z

pkg/extensions/upgrade.go

+	})
+}
+
+func handleExtensionUninstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) (finalErr error) {


Suggested change

func handleExtensionUninstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) (finalErr error) {

func handleExtensionUninstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) error {

emosbaugh · 2025-02-04T20:16:26Z

cmd/installer/cli/version.go

 			for _, k := range keys {
-				version := applierVersions[k]
+				version := versionsMap[k]
 				if !strings.HasPrefix(version, "v") {
 					version = fmt.Sprintf("v%s", version)


are we sure we want to prepend a v to extensions versions?

emosbaugh · 2025-02-04T20:30:45Z

operator/pkg/cli/upgrade_job.go

-					continue
-				}
-				break
+				return err


this masks the error. should return upgradeErr

emosbaugh · 2025-02-04T20:39:41Z

operator/pkg/cli/version.go

 	for _, k := range keys {
-		version := applierVersions[k]
+		version := versionsMap[k]
 		if !strings.HasPrefix(version, "v") {
 			version = fmt.Sprintf("v%s", version)


are we sure we want to prepend a v to extensions versions?

emosbaugh · 2025-02-04T20:42:13Z

operator/pkg/k8sutil/installation.go

+
+func SetConditionStatus(ctx context.Context, cli client.Client, in *ecv1beta1.Installation, condition metav1.Condition) error {
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		var copy ecv1beta1.Installation


Do we still need this to be a copy or can we use in directly? This seems like a misuse of the controller runtime client.

emosbaugh · 2025-02-04T21:47:09Z

operator/pkg/upgrade/upgrade.go

 	err = reApplyInstallation(ctx, cli, in)
 	if err != nil {
 		return fmt.Errorf("unlock installation: %w", err)
 	}

 	err = support.CreateHostSupportBundle()
 	if err != nil {
-		logrus.Warnf("Failed to upgrade host support bundle: %v", err)
+		logrus.Warnf("upgrade host support bundle: %v", err)


Should put this back as it was more readable before

Suggested change

logrus.Warnf("upgrade host support bundle: %v", err)

logrus.Warnf("Failed to upgrade host support bundle: %v", err)

This should also probably be slog

emosbaugh · 2025-02-04T21:59:54Z

pkg/addons2/adminconsole/upgrade.go

 		Force:        false,
 	})
 	if err != nil {
-		return errors.Wrap(err, "upgrade admin console")
+		return errors.Wrap(err, "upgrade")


Suggested change

return errors.Wrap(err, "upgrade")

return errors.Wrap(err, "helm upgrade")

i feel like "upgrade admin console" is more accurate here though

i dont think so because that will be duplicated if you look up the stack. this is calling helm client upgrade so we should wrap it with exactly what it does

looks like you comment was on old code. it only says upgrade in the final version

In my opinion we should wrap with the addon name as far up the stack as possible #1757 (comment)

emosbaugh · 2025-02-04T22:02:01Z

pkg/addons2/embeddedclusteroperator/install.go

 	})
 	if err != nil {
-		return errors.Wrap(err, "install metrics operator")
+		return errors.Wrap(err, "install")


Suggested change

return errors.Wrap(err, "install")

return errors.Wrap(err, "helm install")

emosbaugh · 2025-02-04T22:02:11Z

pkg/addons2/adminconsole/install.go

 	})
 	if err != nil {
-		return errors.Wrap(err, "install admin console")
+		return errors.Wrap(err, "install")


Suggested change

return errors.Wrap(err, "install")

return errors.Wrap(err, "helm install")

emosbaugh · 2025-02-04T22:04:20Z

pkg/addons2/embeddedclusteroperator/metadata.go

+		Version:      Metadata.Version,
+		Values:       string(values),
+		TargetNS:     namespace,
+		ForceUpgrade: ptr.To(false),


This is set to false while it is set to true in pkg/addons2/embeddedclusteroperator/upgrade.go on line 37

_, err = hcli.Upgrade(ctx, helm.UpgradeOptions{ ReleaseName: releaseName, ChartPath: e.ChartLocation(), ChartVersion: e.ChartVersion(), Values: values, Namespace: namespace, Labels: getBackupLabels(), Force: true, })

oh interesting. we should probably change that to false.

emosbaugh · 2025-02-04T22:05:24Z

pkg/addons2/embeddedclusteroperator/upgrade.go

+		Force:        true,
+	})
+	if err != nil {
+		return errors.Wrap(err, "upgrade")


Suggested change

return errors.Wrap(err, "upgrade")

return errors.Wrap(err, "helm upgrade")

emosbaugh · 2025-02-04T22:07:41Z

pkg/addons2/velero/upgrade.go

+		Force:        false,
+	})
+	if err != nil {
+		return errors.Wrap(err, "upgrade")


Suggested change

return errors.Wrap(err, "upgrade")

return errors.Wrap(err, "helm upgrade")

emosbaugh · 2025-02-04T22:07:50Z

pkg/addons2/seaweedfs/upgrade.go

+		Force:        false,
+	})
+	if err != nil {
+		return errors.Wrap(err, "upgrade")


Suggested change

return errors.Wrap(err, "upgrade")

return errors.Wrap(err, "helm upgrade")

emosbaugh · 2025-02-04T22:07:59Z

pkg/addons2/registry/upgrade.go

 		Force:        false,
 	})
 	if err != nil {
-		return errors.Wrap(err, "upgrade registry")
+		return errors.Wrap(err, "upgrade")


Suggested change

return errors.Wrap(err, "upgrade")

return errors.Wrap(err, "helm upgrade")

emosbaugh · 2025-02-04T22:08:08Z

pkg/addons2/openebs/upgrade.go

+		Force:        false,
+	})
+	if err != nil {
+		return errors.Wrap(err, "upgrade")


Suggested change

return errors.Wrap(err, "upgrade")

return errors.Wrap(err, "helm upgrade")

emosbaugh · 2025-02-04T22:08:41Z

pkg/addons2/openebs/install.go

 		Namespace:    namespace,
 	})
 	if err != nil {
-		return errors.Wrap(err, "install openebs")
+		return errors.Wrap(err, "install")


Suggested change

return errors.Wrap(err, "install")

return errors.Wrap(err, "helm install")

emosbaugh · 2025-02-04T22:08:55Z

pkg/addons2/registry/install.go

 	})
 	if err != nil {
-		return errors.Wrap(err, "install registry")
+		return errors.Wrap(err, "install")


Suggested change

return errors.Wrap(err, "install")

return errors.Wrap(err, "helm install")

emosbaugh · 2025-02-04T22:09:03Z

pkg/addons2/seaweedfs/install.go

 	})
 	if err != nil {
-		return errors.Wrap(err, "install seaweedfs")
+		return errors.Wrap(err, "install")


Suggested change

return errors.Wrap(err, "install")

return errors.Wrap(err, "helm install")

emosbaugh · 2025-02-04T22:09:15Z

pkg/addons2/velero/install.go

 		Namespace:    namespace,
 	})
 	if err != nil {
-		return errors.Wrap(err, "install velero")
+		return errors.Wrap(err, "install")


Suggested change

return errors.Wrap(err, "install")

return errors.Wrap(err, "helm install")

emosbaugh · 2025-02-04T22:20:26Z

pkg/addons2/embeddedclusteroperator/upgrade.go

+		Values:       values,
+		Namespace:    namespace,
+		Labels:       getBackupLabels(),
+		Force:        true,


This is backwards

emosbaugh · 2025-02-04T22:59:36Z

pkg/addons2/registry/values.go

+	// only add tls secret value if the secret exists
+	// this is for backwards compatibility when the registry was deployed without TLS
+	var secret corev1.Secret
+	if err := kcli.Get(ctx, k8stypes.NamespacedName{Namespace: namespace, Name: tlsSecretName}, &secret); err == nil {


we should return all errors other than not found error here

emosbaugh · 2025-02-04T23:00:55Z

pkg/addons2/seaweedfs/seaweedfs.go

@@ -6,7 +6,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/replicatedhq/embedded-cluster/pkg/release"
 	"github.com/replicatedhq/embedded-cluster/pkg/runtimeconfig"
-	"gopkg.in/yaml.v3"
+	"gopkg.in/yaml.v2"


why revert this?

emosbaugh · 2025-02-04T23:04:46Z

pkg/addons2/upgrade.go

+	}
+	for _, addon := range addons {
+		if err := upgradeAddOn(ctx, hcli, kcli, in, addon); err != nil {
+			return err


Suggested change

return err

return errors.Wrap(err, "addon %s", addon.Name())

emosbaugh · 2025-02-04T23:09:58Z

pkg/addons2/velero/install.go

@@ -71,7 +72,7 @@ func createCredentialsSecret(ctx context.Context, kcli client.Client) error {
 		},
 		Type: "Opaque",
 	}
-	if err := kcli.Create(ctx, &credentialsSecret); err != nil {
+	if err := kcli.Create(ctx, &credentialsSecret); err != nil && !k8serrors.IsAlreadyExists(err) {


can we add a comment as to why we create an empty secret here

emosbaugh · 2025-02-04T23:10:44Z

pkg/addons2/velero/velero.go

@@ -7,7 +7,7 @@ import (
 	ecv1beta1 "github.com/replicatedhq/embedded-cluster/kinds/apis/v1beta1"
 	"github.com/replicatedhq/embedded-cluster/pkg/release"
 	"github.com/replicatedhq/embedded-cluster/pkg/runtimeconfig"
-	"gopkg.in/yaml.v3"
+	"gopkg.in/yaml.v2"


why revert?

emosbaugh · 2025-02-04T23:12:29Z

pkg/extensions/install.go

+		loading.Infof("Installing %s", ext.Name)
+
+		if err := install(ctx, hcli, ext); err != nil {
+			return errors.Wrap(err, "install extension")


Suggested change

return errors.Wrap(err, "install extension")

return errors.Wrapf(err, "install extension %s", ext.Name)

sgalsaleh added the type::feature label Jan 29, 2025

sgalsaleh force-pushed the v2-upgrades-without-manager branch 3 times, most recently from 4925045 to da76031 Compare January 30, 2025 21:46

V2 upgrades without manager

2fcf916

sgalsaleh force-pushed the v2-upgrades-without-manager branch from da76031 to 2fcf916 Compare January 30, 2025 22:02

sgalsaleh and others added 23 commits January 30, 2025 14:07

overrides for ha migration

ccb0f7e

fix build

5fee73f

clean error message when upgrading addons and extensions

2c6a65d

include logs in support bundle

08b1c19

record upgrade failures

e59eee0

yaml v3

4928d13

Merge branch 'main' into v2-upgrades-without-manager

0c6293d

handle panics

9c695a3

use slog

55022a0

replace join with join2 (#1780)

66c267f

fix build

997e67b

fix ha join

4cdd659

fix lint (#1781)

5ab49b7

* add comments to ignore deprecated BuiltinConfigs field * fix linting

mount charts dir

b511d51

feat(v2): enable v2 restore (#1776)

7266fe1

fix upgrades

84dd073

chore(ci): fix TestGetInstallation migrate data dirs (#1786)

2f7229a

feat(v2): fix e2e TestVersion (#1784)

221241c

* feat(v2): fix e2e TestVersion * f * f

fix operator upgrade in airgap

461657a

fix velero airgap upgrades

3a6848c

fix airgap upgrades

fbdee2d

emosbaugh reviewed Feb 4, 2025

View reviewed changes

operator/pkg/cli/upgrade_job.go

continue

}

break

return err

Copy link

Member

emosbaugh Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this masks the error. should return upgradeErr

emosbaugh reviewed Feb 4, 2025

View reviewed changes

		@@ -214,6 +220,8 @@ func preRunInstall2(cmd cobra.Command, flags Install2CmdFlags) error {

		flags.isAirgap = flags.airgapBundle != ""

		flags.isAirgap = flags.airgapBundle != ""

	func handleExtensionInstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) (finalErr error) {
	func handleExtensionInstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) error {

	func handleExtensionUpgrade(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart, needsUpgrade bool) (finalErr error) {
	func handleExtensionUpgrade(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart, needsUpgrade bool) error {

	func handleExtensionUninstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) (finalErr error) {
	func handleExtensionUninstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) error {

	logrus.Warnf("upgrade host support bundle: %v", err)
	logrus.Warnf("Failed to upgrade host support bundle: %v", err)

	return errors.Wrap(err, "upgrade")
	return errors.Wrap(err, "helm upgrade")

	return errors.Wrap(err, "install")
	return errors.Wrap(err, "helm install")

	return errors.Wrap(err, "install extension")
	return errors.Wrapf(err, "install extension %s", ext.Name)

V2 upgrades without manager #1757

V2 upgrades without manager #1757

Conversation

sgalsaleh commented Jan 29, 2025

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR require a test?

Does this PR require a release note?

Does this PR require documentation?

github-actions bot commented Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emosbaugh Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emosbaugh Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emosbaugh Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emosbaugh Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2025 •

edited

Loading

emosbaugh Feb 4, 2025 •

edited

Loading

emosbaugh Feb 4, 2025 •

edited

Loading

emosbaugh Feb 5, 2025 •

edited

Loading

emosbaugh Feb 4, 2025 •

edited

Loading