Rename OperatorVersion CRs from `name-<operatorVersion>` to `name-<appVersion>-<operatorVersion>` by ANeumann82 · Pull Request #1684 · kudobuilder/kudo

ANeumann82 · 2020-09-15T14:06:15Z

What this PR does / why we need it:

Rename OperatorVersion CRs from name-<operatorVersion> to name-<appVersion>-<operatorVersion>
Migrate existing OperatorVersion and Instance CRs to new naming scheme

Fixes #1681

Requires adjustments in operator repository, PR is here: kudobuilder/operators#295

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed bug in version sorting Fixed panic while casting Added test for FilterByName Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 · 2020-09-16T11:53:25Z

hack/run-operator-tests.sh


 rm -rf operators
-git clone https://github.com/kudobuilder/operators
+git clone --branch "an/rename-operatorversions" https://github.com/kudobuilder/operators


This will need to be removed before merging - It requires changes on the operator tests

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

pkg/kudoctl/kudoinit/migration/01_migrate_ov_names/migrate_ov_names.go

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog · 2020-09-16T16:43:33Z

It's just an idea but if we put the operatorVersion first in the name like name-<operatorVersion>-<appVersion> we might be able to stay backward-compatible (with some extra logic) and doesn't have to rename the OVs

ANeumann82 · 2020-09-21T12:21:35Z

It's just an idea but if we put the operatorVersion first in the name like name-<operatorVersion>-<appVersion> we might be able to stay backward-compatible (with some extra logic) and doesn't have to rename the OVs

How so? The appVersion is optional in any case, and if an operator has no appVersion, the name will be name-<operatorVersion> like before, but afaik all of our operators do have appVersions...

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> # Conflicts: # pkg/kudoctl/packages/convert/resources.go

Revert server side incluster_resolver Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

nfnt

Great to see that this is introducing migration to handle these changes now and in the future gracefully without breaking existing installations 👏
I do have questions around these migrations though.

pkg/util/kudo/versions.go

pkg/apis/kudo/v1beta1/operatorversion_types_helpers.go

pkg/kudoctl/kudoinit/setup/setup.go

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog

I got through the half of it and I think, that there is potentially a way to change OV naming without the migrations. In the end, most of the code does not care whether the OV is named one way or another. The only part where we need to be able to uniquely reference the OV from operator name, operatorVersionn and the appVersion is the server-side InClusterResolver, used to check whether all dependencies exist in the cluster.
If we would bump the current API version along with the OV naming rules, and make the func OperatorVersionName return different names, according to each API version, we should be able to avoid the migration altogether. I have nothing against it, but it is always a risk.

zen-dog · 2020-09-23T11:16:37Z

pkg/util/kudo/versions.go

+	return ovVersionCompare < 0
+}
+
+// Compares two versions - tries to use semantic versioning first, falls back to string compare.


We already expect and verify semver for all versions and fail otherwise. String comparison seems unnecessary but probably doesn't hurt. I've pushed an addition to VersionVerifier to check the appVersion too.

Actually the appVersion is not required to be a SemVer: https://github.com/kudobuilder/kudo/blob/main/keps/0019-package-api-versioning.md#application-version-app-version

That's why we have string compare in there, and we shouldn't verify the appVersion to be SemVer.

Funny, I looked in that KEP and found that appVersion does follow the SemVer: https://github.com/kudobuilder/kudo/blob/main/keps/0019-package-api-versioning.md#semantic-versioning

Huh, interesting. I think the reason for having it non-semVer was that the appVersion should reflect the actual version of the used application, which may not be SemVer for all Apps.

@nfnt @zmalik Do you remember which way it actually should be? Looks like we need to fix the KEP-19 to make this clear.

pkg/apis/kudo/v1beta1/operatorversion_types_helpers.go

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

nfnt

LGTM! Some nits.

pkg/kudoctl/kube/config.go

pkg/kudoctl/kudoinit/migration/migration.go

nfnt · 2020-10-01T16:30:41Z

pkg/kudoctl/kudoinit/migration/migration_helper.go

+)
+
+// ForEachNamespace calls the given function for all namespaces in the cluster
+func ForEachNamespace(client *kube.Client, f func(ns string) error) error {


Nit: These functions would make sense in pkg/kudoctl/util/kudo as well. But with a kube.Client and kudo.Client, it looks like our usage of the Kubernetes client API will need some refactoring first.

Yeaaaah.... The usage of the different clients is something that really should be refactored and cleaned up. Same with the loggers.

We should have established rules which client(s) and which logger(s) is used where, (i.e. CLI code, Manager code and shared code), but that's a discussion for a different time :)

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

alenkacz

I have few concerns around the migration mainly that it will potentially make things worse leaving broken relations between I and OV behind with no easy way to fix it.

Since proper OV naming is only needed for dependencies and dependencies are such a new feature and not really used that much, migration would probably not be my option.

By the way - I think that if we don't do this whole migration the only problem is that I might have kafka installed and then I install something depending on kafka and the versions won't be matches and because of that I will get new kafka installed with the proper naming. As I find this scenario as a whole so unlikely I would even be fine with no migration at all :-)

pkg/kudoctl/packages/resolver/resolver_incluster.go

alenkacz · 2020-10-06T06:58:27Z

pkg/kudoctl/packages/resolver/resolver_incluster.go

+	newOvList.Sort()
+
+	// Find first matching OV
+	ov, _ := newOvList.FindFirstMatch(name, operatorVersion, appVersion).(*kudoapi.OperatorVersion) // nolint:errcheck


since you're ignoring errors here, please add an comment why that's ok to do :)

isn't it possible that FilterByName still does not yield anything and this ends with error? I don't know, I would rather handle it :D

I've added a comment.
FindFirstMatch does not return an error - the error could happen when casting to the OperatorVersion. But we handle the nil case anyway two lines below, so there's no need to really handle the error.

alenkacz · 2020-10-06T07:04:26Z

pkg/kudoctl/resources/dependencies/resolve.go

 func fullyQualifiedName(kt kudoapi.KudoOperatorTaskSpec) string {
-	return fmt.Sprintf("%s-%s", kudoapi.OperatorVersionName(kt.Package, kt.OperatorVersion), kt.AppVersion)
+	operatorVersion := kt.OperatorVersion
+	if operatorVersion == "" {


the dependency can really depend on ANY OV? 🤔

I would assume so. I think it's perfectly fine to depend on "package: zookeeper, appVersion: 2.5, operatorVersion: any"

pkg/kudoctl/resources/dependencies/resolve_test.go

alenkacz · 2020-10-06T07:10:30Z

pkg/kudoctl/kudoinit/migration/01_migrate_ov_names/migrate_ov_names.go

+	return migration.ForEachNamespace(m.client, m.migrateInNamespace)
+}
+
+func (m *MigrateOvNames) migrateInNamespace(ns string) error {


so this will be run when you upgrade to 0.17 but will this be run again when migrating from 0.17 to 0.18? I don't think we want to do that

It would, yes - And it doesn't matter, the migrations are (or should be) idempotent. You can re-run them again and again, they shouldn't modify anything after the first run.

alenkacz · 2020-10-06T07:10:56Z

pkg/kudoctl/kudoinit/migration/01_migrate_ov_names/migrate_ov_names.go

+
+func (m *MigrateOvNames) migrateInNamespace(ns string) error {
+	clog.V(1).Printf("Run OperatorVersion name migration on namespace %q", ns)
+	if err := migration.ForEachOperatorVersion(m.client, ns, m.migrateOperatorVersion); err != nil {


this is not really atomic because before you migrate instance, it has no OV linked or the link is rather broken :/

so what if this fails in the middle? we'll leave the cluster in broken state with no easy way to fix this :/

maybe the re-run of upgrade would actually fix itself? 🤔 but will it even work if manager is down and everything is "in between"?

Yeah - the migration could have been fixed to be completely reentrant, the code that you reviewed wasn't though. I've removed the migration now, but we should keep that in mind for future migrations.

pkg/apis/kudo/v1beta1/operatorversion_types_helpers.go

porridge · 2020-10-06T08:36:32Z

pkg/kudoctl/kudoinit/migration/01_migrate_ov_names/migrate_ov_names.go

+		clog.V(1).Printf("Adjust OperatorVersion of Instance %q", i.Name)
+
+		// Set OperatorVersion
+		i.Spec.OperatorVersion.Name = newOwner.Name


Hm, I'm not sure operators are prepared to deal with such change at runtime. If {{ .OperatorName }} was used by the deploy plan, the value might have trickled down deep into resources, get exposed in UIs, URLs, etc 🤔
Does this change, when applied, cause the update or deploy plan to be triggered?

That's a good point. I didn't even think about the "OperatorName" variable -.-

I have removed the migration for now - We'll still have to test an upgrade with more complex operators that use dependencies.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 · 2020-10-08T09:20:57Z

I have few concerns around the migration mainly that it will potentially make things worse leaving broken relations between I and OV behind with no easy way to fix it.

Since proper OV naming is only needed for dependencies and dependencies are such a new feature and not really used that much, migration would probably not be my option.

By the way - I think that if we don't do this whole migration the only problem is that I might have kafka installed and then I install something depending on kafka and the versions won't be matches and because of that I will get new kafka installed with the proper naming. As I find this scenario as a whole so unlikely I would even be fine with no migration at all :-)

I kind of agree - The migration in the current state is not safe enough and could lead to a broken state. I think this could be fixed though.

The bigger questions seems to be be: Do we want a migration here or not? On one hand I think you're right, it's probably not that big of a deal, and we could probably get away without a migration. On the other hand, it feels wrong to keep an installation in a "wrong" condition.

I have removed the migration for now. Let's see how this works out :)

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> # Conflicts: # go.mod # go.sum

alenkacz

I think I prefer no migration to "not always safe migration" so 👍 We should take time to document this though in the release notes cc. @nfnt since he's a release manager for the next release :)

ANeumann82 added 9 commits September 10, 2020 15:31

Added migration, adjusted incluster_resolver

d45938c

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Extract operator version sorting into kudo package

8d231ff

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Started to add tests

90399cb

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed migration, adjusted upgrade test to confirm migration

83e386c

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Merge branch 'main' into an/rename-operatorversions

d2a04f5

Adjusted migrations

0bd2cc9

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Use adjusted operator version tests

7683c13

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed integration test

88701b4

Fixed bug in version sorting Fixed panic while casting Added test for FilterByName Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fix e2e-tests

4733914

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 commented Sep 16, 2020

View reviewed changes

ANeumann82 added 2 commits September 16, 2020 14:01

Small cleanup

be11d6d

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Merge branch 'main' into an/rename-operatorversions

0fe273c

ANeumann82 commented Sep 16, 2020

View reviewed changes

pkg/kudoctl/kudoinit/migration/01_migrate_ov_names/migrate_ov_names.go Outdated Show resolved Hide resolved

Cleanup migrations

d54c556

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 mentioned this pull request Sep 16, 2020

Adjust operatorversion names in tests kudobuilder/operators#295

Open

ANeumann82 added 3 commits September 21, 2020 14:27

Merge branch 'main' into an/rename-operatorversions

d2aab43

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> # Conflicts: # pkg/kudoctl/packages/convert/resources.go

Adjustments for merged main

b1f5b4f

Revert server side incluster_resolver Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Small cleanup

c5f7030

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 marked this pull request as ready for review September 22, 2020 13:16

ANeumann82 requested review from alenkacz, gerred, kensipe, nfnt and zen-dog as code owners September 22, 2020 13:16

nfnt requested changes Sep 22, 2020

View reviewed changes

pkg/util/kudo/versions.go Outdated Show resolved Hide resolved

pkg/apis/kudo/v1beta1/operatorversion_types_helpers.go Show resolved Hide resolved

pkg/kudoctl/kudoinit/setup/setup.go Show resolved Hide resolved

ANeumann82 and others added 3 commits September 22, 2020 16:14

Use SemVer v3

ec415a5

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Updated documentation

108c854

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Verify that OV appVersion is also a semver (if present)

553a1fa

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog reviewed Sep 23, 2020

View reviewed changes

ANeumann82 added 2 commits September 25, 2020 12:41

Usre more go-like var names

971404a

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Add SemVer verify for appVersion back in.

11dd956

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

nfnt approved these changes Oct 1, 2020

View reviewed changes

Small changes from review

13ffcd7

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 added release/blocker release/breaking-change This PR contains breaking changes and is marked in the release notes labels Oct 2, 2020

alenkacz requested changes Oct 6, 2020

View reviewed changes

porridge reviewed Oct 6, 2020

View reviewed changes

alenkacz modified the milestones: 0.15.0, 0.17.0 Oct 7, 2020

ANeumann82 added 3 commits October 8, 2020 10:46

Code review requests

48304db

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Adjusted migration

9c8b72b

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Removed migration

45366f1

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Merge branch 'main' into an/rename-operatorversions

fb3f2a8

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> # Conflicts: # go.mod # go.sum

alenkacz approved these changes Oct 9, 2020

View reviewed changes

ANeumann82 merged commit 5e7546b into main Oct 9, 2020

ANeumann82 deleted the an/rename-operatorversions branch October 9, 2020 13:28

Conversation

ANeumann82 commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zen-dog commented Sep 16, 2020

Uh oh!

ANeumann82 commented Sep 21, 2020

Uh oh!

nfnt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zen-dog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nfnt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alenkacz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ANeumann82 commented Oct 8, 2020

Uh oh!

alenkacz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

ANeumann82 commented Sep 15, 2020 •

edited

Loading