EVEREST-793: upgrade command #297

michal-kralik · 2024-02-14T17:36:20Z

CHANGE DESCRIPTION

Problem:
EVEREST-793

Support for everestctl upgrade command.
Upgrades to the next minor version based on metadata from a version service.

CHECKLIST

Jira

Is the Jira ticket created and referenced properly?

Tests

Is an Integration test/test case added for the new feature/change?
Are unit tests added where appropriate?

oksana-grishchenko · 2024-02-15T08:11:57Z

commands/install.go

+	viper.BindEnv("kubeconfig")                                                         //nolint:errcheck,gosec
+	viper.BindPFlag("kubeconfig", cmd.Flags().Lookup("kubeconfig"))                     //nolint:errcheck,gosec
+	viper.BindPFlag("namespaces", cmd.Flags().Lookup("namespaces"))                     //nolint:errcheck,gosec
+	viper.BindPFlag("version-metadata-url", cmd.Flags().Lookup("version-metadata-url")) //nolint:errcheck,gosec


I'm thinking if we really need this as a cli flag. It's not something the end user would want to configure. Should we have it in build flags then?

The purpose of this flag is to enable:

QA/devs to test custom upgrade scenarios

Air-gapped environments to be able to point to an internal version metadata service

oksana-grishchenko · 2024-02-15T08:29:24Z

commands/upgrade.go

-func parseConfig() (*upgrade.Config, error) {
-	c := &upgrade.Config{}
+func parseUpgradeConfig() (*upgrade.UpgradeConfig, error) {
+	c := &upgrade.UpgradeConfig{}


will there config types other than the upgrade in this package so we need to specify the type? upgrade.UpgradeConfig looks redundant which doesn't follow best practices

Yes, there could be, as we have with other commands.

oksana-grishchenko · 2024-02-15T08:49:22Z

pkg/upgrade/upgrade_test.go

+		wantErr        bool
+		wantErrIs      error


looks like err error should be enough here. if no error wanted the field remains nil.

oksana-grishchenko · 2024-02-15T09:03:08Z

pkg/version/metadata.go

+	if err != nil {
+		return nil, errors.Join(err, errors.New("could not parse version metadata URL"))
+	}
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, p.JoinPath("metadata/v1/everest").String(), nil)


is this an existing path? if it's under consideration still, I would suggest "everest/v1" so we had version-service-url/product-name/version

This is defined by the version service.
It follows the current structure which is versions/v1/{product}

oksana-grishchenko · 2024-02-15T09:13:06Z

pkg/install/install.go

 	o.l.Info("Installing Operator Lifecycle Manager")
+	// TODO: do we upgrade OLM if the version is too old?


I think we should. Another question is how we define if it's too old. Should it be also some kind of mapping for every everest version to have a list of supported OLM versions? Or just hardcoded versions like - Everest 0.8.0 works with the olm X.Y.Z. Once the Everest version is upgraded, olm should be upgraded (if needed) to the version that this version of everest works with.

Per discussion, OLM upgrade will be postponed.

recharte · 2024-02-27T14:15:13Z

go.mod

+replace github.com/Percona-Lab/percona-version-service => ../percona-version-service
+


Leaving a comment so we don't forget about this.

recharte · 2024-02-27T14:25:05Z

pkg/install/install.go

+	// TODO: we shall probably split this into "install" and "add namespaces"
+	// Otherwise the logic is hard to maintain - we need to make sure not to,
+	// for example, install a different version of operators per namespace, if
+	// we are always installing the "latest" version.


I already proposed this to Peter and he agreed with the approach as long as users aren't forced to run 2 commands in the initial installation. He wants users to be able to run a single command and have everything set up for them.
In subsequent runs of the install command we can even check and return without doing anything, just saying "Everest is already installed". Adding extra namespaces can be done in a separate command like you suggested.

recharte · 2024-02-27T14:26:32Z

pkg/install/install.go

@@ -236,6 +287,7 @@ func (o *Install) provisionMonitoringStack(ctx context.Context) error {
 	}

 	l.Info("Preparing k8s cluster for monitoring")
+	// TODO: shall we grab VM operator version from metadata?


I don't think so. This info lives in the catalog.

The catalog is moving towards having all available versions listed. How do we know which version to use if it's not coming from the metadata service?

recharte · 2024-02-27T14:29:42Z

pkg/install/install.go

+	if err := o.provisionDBNamespaces(ctx, recVer); err != nil {
 		return err
 	}

-	if err := o.provisionEverest(ctx); err != nil {
+	if err := o.provisionEverestOperator(ctx, recVer); err != nil {
 		return err
 	}


I'm not sure we should get these version from the metadata, this info is part of the catalog.
I thought that the metadata would be used primarily for the everest version itself, the operators' version already exist as part of the catalog, shouldn't we be using that instead?

The catalog is moving towards having all available versions listed. How do we know which version to use if it's not coming from the metadata service?

The catalog is moving towards having all available versions listed.

We agreed on having just 2 versions of operator for a given everest version, right? Since we release a new catalog version with each everest version we would only ever have 2 versions of the operators in the catalog, correct?

commands/upgrade.go

pkg/upgrade/upgrade.go

recharte · 2024-02-27T14:41:15Z

pkg/upgrade/upgrade.go

+	// We cannot use the latest version of catalog yet since
+	// at the time of writing, each catalog version supports only one Everest version.


Just trying to understand this better. When we update the catalog itself, this doesn't have any side-effects directly right? The subscription would pick up the new versions from the new catalog and would create new install plans that would be waiting approval.
I agree that the most correct way would be to have 2 everest operator versions in the new catalog but is there really any problems with having just one now?

When upgrading catalog, we need to use a specific version to be able to upgrade. Otherwise the catalog will not offer correct versions for Everest.
Once the catalog includes all versions of operators, we can always update to the latest one.

michal-kralik added 6 commits February 9, 2024 16:53

EVEREST-793: upgrade

cfb044c

Merge branch 'main' into EVEREST-793-upgrade

3714488

Progress

70bbf28

Merge branch 'main' into EVEREST-793-upgrade

75c0781

go mod tidy

134b47a

cleanup

8db3369

oksana-grishchenko reviewed Feb 15, 2024

View reviewed changes

michal-kralik added 3 commits February 21, 2024 08:53

Install proper versions

c652985

Resolve a couple TODOs

52a8044

Merge branch 'main' into EVEREST-793-upgrade

ded2d9c

recharte reviewed Feb 27, 2024

View reviewed changes

michal-kralik added 2 commits February 28, 2024 08:40

Refactore test

9eeb4b9

Remove namespace flag

66ce00b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVEREST-793: upgrade command #297

EVEREST-793: upgrade command #297

michal-kralik commented Feb 14, 2024 •

edited

Loading

oksana-grishchenko Feb 15, 2024

michal-kralik Feb 28, 2024

oksana-grishchenko Feb 15, 2024

michal-kralik Feb 28, 2024

oksana-grishchenko Feb 15, 2024

michal-kralik Feb 28, 2024

oksana-grishchenko Feb 15, 2024

michal-kralik Feb 28, 2024

oksana-grishchenko Feb 15, 2024

michal-kralik Feb 28, 2024

recharte Feb 27, 2024

recharte Feb 27, 2024

recharte Feb 27, 2024

michal-kralik Feb 28, 2024

recharte Feb 27, 2024

michal-kralik Feb 28, 2024

recharte Mar 1, 2024 •

edited

Loading

recharte Feb 27, 2024

michal-kralik Feb 28, 2024

		o.l.Info("Installing Operator Lifecycle Manager")
		// TODO: do we upgrade OLM if the version is too old?

		replace github.com/Percona-Lab/percona-version-service => ../percona-version-service

		// We cannot use the latest version of catalog yet since
		// at the time of writing, each catalog version supports only one Everest version.

EVEREST-793: upgrade command #297

Are you sure you want to change the base?

EVEREST-793: upgrade command #297

Conversation

michal-kralik commented Feb 14, 2024 • edited Loading

CHANGE DESCRIPTION

CHECKLIST

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

Choose a reason for hiding this comment

recharte Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michal-kralik commented Feb 14, 2024 •

edited

Loading

recharte Mar 1, 2024 •

edited

Loading