Install operator instance from in-cluster operator version resource #1680

zen-dog · 2020-09-08T18:37:25Z

Summary:
The current kudo install ... command accepts either a remote repo operator name, a URL, or a local folder/tgz. This PR allows the user to install an instance from an existing in-cluster operator version using the newly introduced --in-cluster option:

❯ ./bin/kubectl-kudo install first-operator --operator-version=0.2.0 --in-cluster
operatorversion default/first-operator-0.2.0 already installed
instance default/first-operator-instance created

This way kudoctl will only look for already installed operator versions (one can list them using kudoctl get operatorversions) and does not need access to the repo/package files.

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

Fixes #1678

pkg/kudoctl/packages/resolver/resolver_incluster.go

zen-dog · 2020-09-08T18:51:05Z

pkg/kudoctl/cmd/install/install.go

@@ -73,7 +79,13 @@ func installOperator(operatorArgument string, options *Options, fs afero.Fs, set

 	clog.V(3).Printf("getting operator package")

-	resolver := pkgresolver.New(repository)
+	var resolver pkgresolver.Resolver


This if-else will probably be somewhat controversial: PackageResolver returned by the pkgresolver.New method is already on overarching resolver combining the others (repo/folder/URL) under one Resolve(...) method call. So, InClusterResolver could, at least in theory, become one of them. However, given that when the --in-cluster option is used, no other resolver should be even considered, I felt that it would make more sense to keep it separately.

zen-dog · 2020-09-08T18:53:38Z

Note, that kudo upgrade and kudo package list cli commands could also use the --in-cluster option to address the in-cluster resources (instead of packages). However, current PR does not touch these commands.

ANeumann82 · 2020-09-09T09:36:46Z

pkg/kudoctl/packages/resolver/resolver_incluster.go

+}
+
+func (r InClusterResolver) Resolve(name string, appVersion string, operatorVersion string) (*packages.Package, error) {
+	ovn := kudoapi.OperatorVersionName(name, operatorVersion)


Why, oh why, do we create an operatorVersionName just from the operator name and the operatorVersion? Shouldn't we have the appVersion in the OV-Name as well??? Not that this is an issue on this PR, I just noticed it here...

In theory, a bump in the appVersion should lead to a bump in operatorVersion so that using the latter in the OV name isn't necessary. But otherwise, "cause it was always this way" 🤷

No - appVersion and operatorVersion are independent. For example, Kafka was just bumped with a new appVersion, but the operator didn't get any new features or changes it has the same operatorVersion...

but the operator didn't get any new features or changes it has the same operatorVersion

How can it be the same operator if the underlying app has changed? Even minor app changes can bring arbitrary changes in APIs, behavior, etc. I'm not sure if that was a good idea.

It's not the same operator. That's why the appVersion was changed. An Operator can only be uniqly identified by OV-Version and AppVersion. That's why we have both in the file name in the repository.

https://github.com/kudobuilder/kudo/blob/main/keps/0019-package-api-versioning.md#example-hypothetical-timeline-for-apache-cassandra-releases

Hm, besides the fact that appVersion is optional, the OperatorVersion resource currently does not include it in the name 🤷

Yeah, i know. That's what irritated me. Seems we just didn't implement the change, it's mentioned in the KEP: https://github.com/kudobuilder/kudo/blob/main/keps/0019-package-api-versioning.md#naming-of-operatorversion-objects

I've created an issue for it, but no idea how to solve it yet.

historically it was expected that a bump in appVersion would bump operatorVersion. They are broken apart so that an operator can evolve independently without an appVersion change... exposing the appVersion was important because that is what the end user is interested in and the early days of kudo didn't expose that... just the opVersion which was frustrating from an end user stand point. IMO it is mistake to bump appVersion without bumping opVersion. This was a point of debate for the versioning kep.. as long as we use the entire combined version I guess we will be fine.

ANeumann82

I like the idea, but this PR has shown a few issues with the (new and existing) InClusterResolvers. I'm not sure if we should fix them in this PR, but on the other hand I'm not sure if it makes sense to implement the feature as-is...

pkg/kudoctl/cmd/install/install.go

pkg/kudoctl/packages/resolver/resolver_incluster.go

ANeumann82

lgtm

Summary: The current `kudo install ...` command accepts either a remote repo operator name, a URL or a local folder/tgz. This PR allows the user to install an instance of an existing in-cluster operator version using the newly introduced `--in-cluster` option: ``` ❯ ./bin/kubectl-kudo install first-operator --operator-version=0.2.0 --in-cluster operatorversion default/first-operator-0.2.0 already installed instance default/first-operator-instance created ``` This way `kudoctl` will only look for already installed operator versions (one can list them using `kudoctl get operatorversions`) and does not need access to the repo/package files. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

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

users can supply either an operator name, version and app version as with the repository Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

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

kensipe

I can't get behind this current approach... I like the idea of "incluster" resolution... but it seems to need work IMO. IF the team is for it... I will concede as long as we get the dependencies noted cleaned up and validation across all flags.

you captured the validation of --in-cluster AND --skip-instance 👏 ... but there are a number of other flags that don't make sense either like: --app-version, --operator-version, --repo

The challenge with the solution is geared around:

issues with the abstraction... either the previous resolvers need to refactor or this one... or there is a needed sub-support of both.
it isn't reasonable to need to have an abstraction like "Resolver" or "Package" and require the calling code to intimately know what is underneath the abstraction. If X then use Files, if Y then don't because they are nil.
the abstract could also be partially to blame for not previously solving the difference between "install" O and OV and "install" meaning Instance (I). I think I would rather have a different command for instantiating instances.
if this is a new code path... we should make it a new code path... not conflate it with existing code

yeah... the more I review (past my initial comments)... it looks like the resolver is getting the pkg.Resources for installs and upgrades etc. It isn't clear when it resolves to the Files... Do we have to use the resolver.Resolve for this? In the end... we need the resources for sure... but the initial "resolver" pattern was retrieving things outside the cluster (file, http, repos) for use in the cluster. It doesn't feel like "resolving" to "understand" structure in the cluster. Without deep thought... I think I would much rather have the branch code of if options.InCluster result in a a pkg instead of a resolver. In this way, we are not polluting or confusing resolver and working with the important distinction which is the pkg.

go.mod

pkg/kudoctl/packages/resolver/resolver_incluster_test.go

alenkacz

In my previous pass I did not really look at the code but I agree with Ken I am not a big fan of the in-cluster resolver as well. I know that the original concept was not introduced by this PR but I am questioning why do we need to conform to that interface when the types we return are filled only partially. Can't we just create another incluster resolved that won't lie and will really just return what it can return and won't conform to that interface? (and I am talking about both kind of)

I am 'requesting changes' just so we can talk about that. I am also fine with consolidating in another PR if that's what we decide to do.

pkg/controller/instance/resolver_incluster.go

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

zen-dog · 2020-09-17T11:09:01Z

but there are a number of other flags that don't make sense either like: --app-version, --operator-version, --repo

You're right about the --repo option (fixed). But the other two are absolutely used. See this long thread about the reasoning but essentially give a cluster with OperatorVersions:

zookeeper-0.2.0
zookeeper-0.3.0

and a command:

$ kudo install zookeeper --in-cluster

the InCluserResolver will apply the same rules as with the repo: pick the highest operatorVersion, operators without an appVersion are sorted at the back etc. You could also specify --operator-version and --app-version and they will be used during resolving. So, it actually resolves your operator, hence the name and the interface 😉

However, the original idea (set this commit) didn't need this logic and basically just fetched the right OV resource.

alenkacz · 2020-09-17T11:38:39Z

I don't have a strong opinion here so I'll let people with stronger opinions reconcile -> removing my rejection :)

kensipe

I'm in agreement with @alenkacz that we lack a user / real use case... without it there is debate around preferred approach. Aligned with @zen-dog comments and discussions. I also have the concerns I previously noted, in that I think there is a need to find a better abstraction such that it isn't necessary to known the underlying impl to know how to work with the return values of methods of the interface it implements. that said... this is a working model, it has good added test coverage .

I added 1 nit around consistency of method naming. It would be great to resolve that... otherwise /lgtm

pkg/kudoctl/cmd/get/get.go

pkg/kudoctl/util/kudo/kudo.go

Alena indicated that she removed her rejection... looks like it still needs to be missed. Please re-add if appropriate.

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

zen-dog requested review from alenkacz, gerred, kensipe and nfnt as code owners September 8, 2020 18:37

zen-dog requested a review from ANeumann82 September 8, 2020 18:37

zen-dog commented Sep 8, 2020

View reviewed changes

pkg/kudoctl/packages/resolver/resolver_incluster.go Show resolved Hide resolved

zen-dog commented Sep 8, 2020

View reviewed changes

ANeumann82 reviewed Sep 9, 2020

View reviewed changes

zen-dog requested a review from zmalik September 9, 2020 11:38

ANeumann82 reviewed Sep 9, 2020

View reviewed changes

pkg/kudoctl/cmd/install/install.go Outdated Show resolved Hide resolved

pkg/kudoctl/packages/resolver/resolver_incluster.go Show resolved Hide resolved

pkg/kudoctl/packages/resolver/resolver_incluster.go Show resolved Hide resolved

ANeumann82 approved these changes Sep 16, 2020

View reviewed changes

zen-dog added 5 commits September 16, 2020 18:35

make InClusterResolver field private

faf8c63

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

making operator-version mandatory when in-cluster option is used

5aa83e9

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

InClusterResolver now uses the same logic as the repo resolver

75d929e

users can supply either an operator name, version and app version as with the repository Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Added tests for the InClusterResolver and updated the docs

9362f65

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

zen-dog force-pushed the ad/install-incluster-operator branch from 9ec9662 to 9362f65 Compare September 16, 2020 16:37

kensipe requested changes Sep 16, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/kudoctl/packages/resolver/resolver_incluster_test.go Outdated Show resolved Hide resolved

alenkacz previously requested changes Sep 17, 2020

View reviewed changes

pkg/controller/instance/resolver_incluster.go Show resolved Hide resolved

zen-dog added 2 commits September 17, 2020 12:59

Using the right assert

e0cade5

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

Don't use --repo option with --in-cluster

b9aef5e

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

alenkacz self-requested a review September 17, 2020 11:38

kensipe approved these changes Sep 17, 2020

View reviewed changes

pkg/kudoctl/cmd/get/get.go Show resolved Hide resolved

pkg/kudoctl/cmd/get/get.go Show resolved Hide resolved

pkg/kudoctl/util/kudo/kudo.go Show resolved Hide resolved

Consistent naming of kudo client methods

92ecf6d

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

zen-dog merged commit 248272f into main Sep 18, 2020

zen-dog deleted the ad/install-incluster-operator branch September 18, 2020 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install operator instance from in-cluster operator version resource #1680

Install operator instance from in-cluster operator version resource #1680

zen-dog commented Sep 8, 2020

zen-dog Sep 8, 2020

zen-dog commented Sep 8, 2020

ANeumann82 Sep 9, 2020

zen-dog Sep 9, 2020 •

edited

Loading

ANeumann82 Sep 9, 2020

zen-dog Sep 9, 2020

ANeumann82 Sep 9, 2020

zen-dog Sep 9, 2020

ANeumann82 Sep 9, 2020

kensipe Sep 16, 2020

ANeumann82 left a comment

ANeumann82 left a comment

kensipe left a comment •

edited

Loading

alenkacz left a comment

zen-dog commented Sep 17, 2020 •

edited

Loading

alenkacz commented Sep 17, 2020

kensipe left a comment

Install operator instance from in-cluster operator version resource #1680

Install operator instance from in-cluster operator version resource #1680

Conversation

zen-dog commented Sep 8, 2020

Choose a reason for hiding this comment

zen-dog commented Sep 8, 2020

Choose a reason for hiding this comment

zen-dog Sep 9, 2020 • 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

ANeumann82 left a comment

Choose a reason for hiding this comment

ANeumann82 left a comment

Choose a reason for hiding this comment

kensipe left a comment • edited Loading

Choose a reason for hiding this comment

alenkacz left a comment

Choose a reason for hiding this comment

zen-dog commented Sep 17, 2020 • edited Loading

alenkacz commented Sep 17, 2020

kensipe left a comment

Choose a reason for hiding this comment

zen-dog Sep 9, 2020 •

edited

Loading

kensipe left a comment •

edited

Loading

zen-dog commented Sep 17, 2020 •

edited

Loading