run bundle: OperatorGroup Install modes by jmrodri · Pull Request #3861 · operator-framework/operator-sdk

jmrodri · 2020-09-08T13:53:12Z

Description of the change:
Handle the run bundle OperatorGroup install mode scenarios.

Motivation for the change:
Support run bundle.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
Add or update relevant sections of the docs website in website/content/en/docs

estroz

There's a bunch of static validation being added to ensureOperatorGroup() that should be done before anything is created in-cluster. Can we move that elsewhere?

jmrodri · 2020-09-15T18:08:38Z

/cc @estroz @joelanford

joelanford

One question about error checking. Otherwise logic looks good (haven't reviewed the tests yet).

camilamacedo86

It. shows are addressing its requirements. So,

/lgtm

estroz

This is almost ready, just one question about logic locality.

estroz · 2020-09-18T16:00:56Z

+	// single namespace and targetns == opname
+	if i.InstallModeType == v1alpha1.InstallModeTypeSingleNamespace {
+		if len(i.TargetNamespaces) < 1 || i.TargetNamespaces[0] == operatorNamespace {
+			return fmt.Errorf("use install mode %q to watch operator's namespace %q", v1alpha1.InstallModeTypeOwnNamespace, i.TargetNamespaces[0])
+		}
+	}


I think using the controller's namespace with SingleNamespace is valid. However if we are going to perform this check, move it (and the one above it) below the check for support (line 123) so we make sure the mode is supported first.

I can be invalid. Developing kubectl operator I was noticing some behavior around this that I thought was odd, but I confirmed with @njhale that OLM requires an operator to support OwnNamespace if it is watching the namespace in which it is deployed.

Also, it looks like this is actually checking two different cases, right?

len(i.TargetNamespaces) < 1 should be checked in Validate() and error with "SingleNamespace requires exactly one target namespace"

i.TargetNamespaces[0] == operatorNamespace is the piece that goes with the error message you return here.

@jmrodri I know we talked about this a bit last week. Can you remind me if (and why) we settled on keeping the check for OwnNamespace having a target namespace list equal to the install namespace, which therefore requires us to set that correctly before calling this check?

I'm kind of thinking that it makes more sense to check OwnNamespace has an empty target namespace list (which we should do in Validate(), and only when we are creating the OperatorGroup in the install namespace do we make the inference that OwnNamespace means to use the install namespace. It also seems logical at that point because we must align the OperatorGroup.metadata.namespace with the OperatorGroup.spec.targetNamespaces. WDYT?

estroz · 2020-09-18T16:12:53Z

+	supported := o.SupportedInstallModes
+
+	// --install-mode was given
+	if !o.InstallMode.IsEmpty() {


Why are we still checking install mode compatibility in this method? This should all be validated by the time ensureOperatorGroup is called. Is there a reason why you're checking them here and CheckCompatibility?

I think I agree @estroz, but sanity check me here @jmrodri. There are three main things we need to check, and I think we can do ALL of it before creating any resources in the cluster:

The actual format and content of the --install-mode flag. We can check this during flag parsing with InstallMode.Set().

CSV / --install-mode compatibility:

if the --install-mode flag is set, make sure it is supported by the CSV.

if the --install-mode flag is not set, make sure the CSV supports one of AllNamespaces or OwnNamespace (SingleNamespace requires an explicit target namespace from the --install-mode flag).

There can only be 0 or 1 operator groups in the namespace. If the namespace has an existing operator group, is it compatible with the intersection of the CSV supported install modes and the --install-mode flag (if set).

@estroz @joelanford you're literally saying do not make these checks in ensureOperatorGroup? I realize they are being done in CheckCompatibility which is the earliest entrypoint where the users input can be checked. But checking it there does not guarantee that some other code can't call ensureOperatorGroup without having gone through CheckCompatibility. Or are we saying that OperatorInstaller does NOT care about the rules it only cares about passing the data that was verified BEFORE it was called?

I guess it feels weird not doing any checks in ensureOperatorGroup because OperatorInstaller does not know that all its attributes were set correctly, so it seems like it should ensure they're okay before doing something with them.

joelanford

I think all of my comments can be follow-ups (not positive), but I'll clear my "Request changes" review, so I'm not holding it up.

I would say that if this implements the proposal and works as expected for both bundles and packagemanifests, lets merge it and come back to make some improvements later (but still soon).

jmrodri · 2020-09-22T18:26:04Z

OWN NAMESPACE

[jesusr@transam operator-sdk{install-modes}]$ k get operatorgroups
No resources found in default namespace.
[jesusr@transam operator-sdk{install-modes}]$ grep -A8 -i installmodes bundle-example-operator/manifests/example-operator.clusterserviceversion.yaml 
  installModes:
  - supported: true
    type: OwnNamespace
  - supported: true
    type: SingleNamespace
  - supported: false
    type: MultiNamespace
  - supported: true
    type: AllNamespaces
[jesusr@transam operator-sdk{install-modes}]$ build/operator-sdk run bundle quay.io/joelanford/example-operator-bundle:0.1.0 --install-mode OwnNamespace
INFO[0002] Successfully created registry pod: quay-io-joelanford-example-operator-bundle-0-1-0 
INFO[0002] Created CatalogSource: example-operator-catalog 
INFO[0003] OperatorGroup "operator-sdk-og" created      
INFO[0003] Created Subscription: example-operator-v0-1-0-sub 
INFO[0011] Approved InstallPlan install-mdwb4 for the Subscription: example-operator-v0-1-0-sub 
INFO[0011] Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to reach 'Succeeded' phase 
INFO[0011]   Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to appear 
INFO[0018]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Pending 
INFO[0019]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Installing 
INFO[0025]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Succeeded 
INFO[0025] OLM has successfully installed "example-operator.v0.1.0" 
[jesusr@transam operator-sdk{install-modes}]$ k get operatorgroup operator-sdk-og -o json  | jq '. | {namespace: .metadata.namespace,  targetNamespaces: .spec.targetNamespaces}'
{
  "namespace": "default",
  "targetNamespaces": [
    "default"
  ]
}

jmrodri · 2020-09-22T18:26:31Z

SINGLE NAMESPACE

[jesusr@transam operator-sdk{install-modes}]$ k get operatorgroups
No resources found in default namespace.
[jesusr@transam operator-sdk{install-modes}]$ grep -A8 -i installmodes bundle-example-operator/manifests/example-operator.clusterserviceversion.yaml 
  installModes:
  - supported: true
    type: OwnNamespace
  - supported: true
    type: SingleNamespace
  - supported: false
    type: MultiNamespace
  - supported: true
    type: AllNamespaces
[jesusr@transam operator-sdk{install-modes}]$ k create ns foo
namespace/foo created
[jesusr@transam operator-sdk{install-modes}]$ build/operator-sdk run bundle quay.io/joelanford/example-operator-bundle:0.1.0 --install-mode SingleNamespace=foo
INFO[0003] Successfully created registry pod: quay-io-joelanford-example-operator-bundle-0-1-0 
INFO[0003] Created CatalogSource: example-operator-catalog 
INFO[0003] OperatorGroup "operator-sdk-og" created      
INFO[0003] Created Subscription: example-operator-v0-1-0-sub 
INFO[0011] Approved InstallPlan install-7gs64 for the Subscription: example-operator-v0-1-0-sub 
INFO[0011] Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to reach 'Succeeded' phase 
INFO[0011]   Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to appear 
INFO[0023]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Pending 
INFO[0024]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Installing 
INFO[0030]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Succeeded 
INFO[0030] OLM has successfully installed "example-operator.v0.1.0" 
[jesusr@transam operator-sdk{install-modes}]$ k get operatorgroup operator-sdk-og -o json  | jq '. | {namespace: .metadata.namespace,  targetNamespaces: .spec.targetNamespaces}'
{
  "namespace": "default",
  "targetNamespaces": [
    "foo"
  ]
}

jmrodri · 2020-09-22T18:26:49Z

ALL NAMESPACES

[jesusr@transam operator-sdk{install-modes}]$ k get operatorgroups
No resources found in default namespace.
[jesusr@transam operator-sdk{install-modes}]$ grep -A8 -i installmodes bundle-example-operator/manifests/example-operator.clusterserviceversion.yaml 
  installModes:
  - supported: true
    type: OwnNamespace
  - supported: true
    type: SingleNamespace
  - supported: false
    type: MultiNamespace
  - supported: true
    type: AllNamespaces
[jesusr@transam operator-sdk{install-modes}]$ build/operator-sdk run bundle quay.io/joelanford/example-operator-bundle:0.1.0 --install-mode AllNamespaces
INFO[0003] Successfully created registry pod: quay-io-joelanford-example-operator-bundle-0-1-0 
INFO[0003] Created CatalogSource: example-operator-catalog 
INFO[0003] OperatorGroup "operator-sdk-og" created      
INFO[0003] Created Subscription: example-operator-v0-1-0-sub 
INFO[0011] Approved InstallPlan install-74kz4 for the Subscription: example-operator-v0-1-0-sub 
INFO[0011] Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to reach 'Succeeded' phase 
INFO[0011]   Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to appear 
INFO[0023]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Pending 
INFO[0024]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Installing 
INFO[0030]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Succeeded 
INFO[0030] OLM has successfully installed "example-operator.v0.1.0" 
[jesusr@transam operator-sdk{install-modes}]$ k get operatorgroup operator-sdk-og -o json  | jq '. | {namespace: .metadata.namespace,  targetNamespaces: .spec.targetNamespaces}'
{
  "namespace": "default",
  "targetNamespaces": null
}

jmrodri · 2020-09-22T18:27:49Z

PRECENDENCE

Existing operatorgroup suppolrting single namespace

[jesusr@transam operator-sdk{install-modes}]$ k create ns foo
namespace/foo created
[jesusr@transam operator-sdk{install-modes}]$ k apply -f single-og.yaml 
operatorgroup.operators.coreos.com/operator-sdk-og created
[jesusr@transam operator-sdk{install-modes}]$ k get operatorgroup operator-sdk-og -o json  | jq '. | {namespace: .metadata.namespace,  targetNamespaces: .spec.targetNamespaces}'
{
  "namespace": "default",
  "targetNamespaces": [
    "foo"
  ]
}
[jesusr@transam operator-sdk{install-modes}]$ build/operator-sdk run bundle quay.io/joelanford/example-operator-bundle:0.1.0 
INFO[0003] Successfully created registry pod: quay-io-joelanford-example-operator-bundle-0-1-0 
INFO[0003] Created CatalogSource: example-operator-catalog 
INFO[0003] Created Subscription: example-operator-v0-1-0-sub 
INFO[0011] Approved InstallPlan install-rkhrw for the Subscription: example-operator-v0-1-0-sub 
INFO[0011] Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to reach 'Succeeded' phase 
INFO[0011]   Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to appear 
INFO[0023]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: InstallReady 
INFO[0024]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Installing 
INFO[0029]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Succeeded 
INFO[0029] OLM has successfully installed "example-operator.v0.1.0"

Existing operatorgroup supporting own namespace

[jesusr@transam operator-sdk{install-modes}]$ k apply -f own-og.yaml 
operatorgroup.operators.coreos.com/operator-sdk-og created
[jesusr@transam operator-sdk{install-modes}]$ k get operatorgroup operator-sdk-og -o json  | jq '. | {namespace: .metadata.namespace,  targetNamespaces: .spec.targetNamespaces}'
{
  "namespace": "default",
  "targetNamespaces": [
    "default"
  ]
}
[jesusr@transam operator-sdk{install-modes}]$ build/operator-sdk run bundle quay.io/joelanford/example-operator-bundle:0.1.0 
INFO[0003] Successfully created registry pod: quay-io-joelanford-example-operator-bundle-0-1-0 
INFO[0003] Created CatalogSource: example-operator-catalog 
INFO[0003] Created Subscription: example-operator-v0-1-0-sub 
INFO[0011] Approved InstallPlan install-4fd4c for the Subscription: example-operator-v0-1-0-sub 
INFO[0011] Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to reach 'Succeeded' phase 
INFO[0011]   Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to appear 
INFO[0023]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Pending 
INFO[0024]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Installing 
INFO[0029]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Succeeded 
INFO[0029] OLM has successfully installed "example-operator.v0.1.0"

Existing operatorgroup supporting all namespaces

[jesusr@transam operator-sdk{install-modes}]$ k apply -f all-og.yaml 
operatorgroup.operators.coreos.com/operator-sdk-og created
[jesusr@transam operator-sdk{install-modes}]$ k get operatorgroup operator-sdk-og -o json  | jq '. | {namespace: .metadata.namespace,  targetNamespaces: .spec.targetNamespaces}'
{
  "namespace": "default",
  "targetNamespaces": null
}
[jesusr@transam operator-sdk{install-modes}]$ build/operator-sdk run bundle quay.io/joelanford/example-operator-bundle:0.1.0 
INFO[0003] Successfully created registry pod: quay-io-joelanford-example-operator-bundle-0-1-0 
INFO[0003] Created CatalogSource: example-operator-catalog 
INFO[0003] Created Subscription: example-operator-v0-1-0-sub 
INFO[0011] Approved InstallPlan install-zb5s2 for the Subscription: example-operator-v0-1-0-sub 
INFO[0011] Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to reach 'Succeeded' phase 
INFO[0011]   Waiting for ClusterServiceVersion "default/example-operator.v0.1.0" to appear 
INFO[0022]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Pending 
INFO[0023]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Installing 
INFO[0029]   Found ClusterServiceVersion "default/example-operator.v0.1.0" phase: Succeeded 
INFO[0029] OLM has successfully installed "example-operator.v0.1.0"

They all had successful operator runs

[jesusr@transam operator-sdk{install-modes}]$ k get all
NAME                                                                  READY   STATUS      RESTARTS   AGE
pod/6c68598feb4a9e5b62e608efafcc1b0f4c13209037d298c8f7041d888d9ms6q   0/1     Completed   0          50s
pod/example-operator-c86659899-56dhv                                  1/1     Running     0          38s
pod/quay-io-joelanford-example-operator-bundle-0-1-0                  1/1     Running     0          60s

NAME                               TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)             AGE
service/example-operator-metrics   ClusterIP   10.100.208.166   <none>        8383/TCP,8686/TCP   31s
service/kubernetes                 ClusterIP   10.96.0.1        <none>        443/TCP             2m49s

NAME                               READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/example-operator   1/1     1            1           38s

NAME                                         DESIRED   CURRENT   READY   AGE
replicaset.apps/example-operator-c86659899   1         1         1       38s

NAME                                                                        COMPLETIONS   DURATION   AGE
job.batch/6c68598feb4a9e5b62e608efafcc1b0f4c13209037d298c8f7041d888db8386   1/1           6s         50s

estroz

/lgtm

Dismissing Joe's review based on #3861 (review)

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 8, 2020

jmrodri requested a review from estroz September 8, 2020 13:53

openshift-ci-robot requested a review from theishshah September 8, 2020 13:53

jmrodri requested review from joelanford, rashmigottipati and theishshah and removed request for theishshah September 8, 2020 13:53

jmrodri mentioned this pull request Sep 8, 2020

Implement run bundle subcommand to improve OLM integration #3626

Closed

14 tasks

jmrodri force-pushed the install-modes branch from 4e58fd6 to fe08233 Compare September 9, 2020 03:46

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2020

jmrodri changed the title ~~WIP: run bundle: OperatorGroup Install modes~~ run bundle: OperatorGroup Install modes Sep 9, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 9, 2020

jmrodri force-pushed the install-modes branch from 0a0b4eb to 552cb8b Compare September 9, 2020 04:35

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2020

joelanford previously requested changes Sep 9, 2020

View reviewed changes

Comment thread internal/olm/operator/bundle/install.go Outdated

Comment thread internal/olm/operator/registry/operator_installer.go

estroz reviewed Sep 9, 2020

View reviewed changes

Comment thread internal/olm/operator/registry/operator_installer.go Outdated

estroz reviewed Sep 9, 2020

View reviewed changes

Comment thread internal/olm/operator/registry/operator_installer.go Outdated

estroz reviewed Sep 9, 2020

View reviewed changes

Comment thread internal/olm/operator/registry/operator_installer.go Outdated

estroz suggested changes Sep 9, 2020

View reviewed changes

Comment thread internal/olm/operator/registry/operator_installer.go Outdated

Comment thread internal/olm/operator/registry/operator_installer.go

Comment thread internal/olm/operator/registry/operator_installer_test.go Outdated

jmrodri force-pushed the install-modes branch from 552cb8b to 646208d Compare September 10, 2020 19:27

jmrodri mentioned this pull request Sep 15, 2020

run bundle: Enable run bundle #3877

Merged

2 tasks

openshift-ci-robot requested review from estroz and joelanford September 15, 2020 18:08

joelanford reviewed Sep 15, 2020

View reviewed changes

Comment thread internal/olm/operator/registry/operator_installer.go Outdated

joelanford reviewed Sep 15, 2020

View reviewed changes

camilamacedo86 reviewed Sep 17, 2020

View reviewed changes

Comment thread internal/olm/operator/registry/operator_installer_test.go Outdated

camilamacedo86 reviewed Sep 17, 2020

View reviewed changes

Comment thread internal/olm/operator/registry/operator_installer_test.go Outdated

camilamacedo86 reviewed Sep 17, 2020

View reviewed changes

Comment thread internal/olm/operator/bundle/install.go Outdated

jmrodri added 11 commits September 17, 2020 12:03

Remove outdated tests

6d2ebd6

Removed unused code

83e787c

Move getSupportedInstallModes to InstallMode and export it

4d702e4

Do not return OperatorGroup from ensureOpratorGroup

30f3451

Set SupportedInstallModes on packagemanifests as well

47f87ef

Replace validateOG with isOGCompatible

d0ce568

Validate supported install modes earlier

edf5b30

Rework the install mode verification

2ed2b91

add TODO for createSubscription & deleted unused test

45ba1d0

Move support install modes validation to CheckCompatiblity method

b5f0e32

Switch to constants for the label keys

59889fa

jmrodri force-pushed the install-modes branch from 6caac19 to 59889fa Compare September 17, 2020 16:55

camilamacedo86 approved these changes Sep 18, 2020

View reviewed changes

openshift-ci-robot assigned camilamacedo86 Sep 18, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2020

estroz suggested changes Sep 18, 2020

View reviewed changes

joelanford reviewed Sep 19, 2020

View reviewed changes

joelanford self-requested a review September 19, 2020 02:43

Default to existing operatorgroup if no installmode present

4efad9d

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2020

estroz approved these changes Sep 22, 2020

View reviewed changes

openshift-ci-robot assigned estroz Sep 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2020

jmrodri merged commit 7f43289 into operator-framework:master Sep 22, 2020

jmrodri deleted the install-modes branch January 30, 2022 18:36

Conversation

jmrodri commented Sep 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

estroz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmrodri commented Sep 15, 2020

Uh oh!

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

estroz left a comment

Choose a reason for hiding this comment

Uh oh!

estroz Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

estroz Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

joelanford Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

jmrodri Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

jmrodri Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

jmrodri commented Sep 22, 2020

OWN NAMESPACE

Uh oh!

jmrodri commented Sep 22, 2020

SINGLE NAMESPACE

Uh oh!

jmrodri commented Sep 22, 2020

ALL NAMESPACES

Uh oh!

jmrodri commented Sep 22, 2020

PRECENDENCE

Uh oh!

estroz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

estroz left a comment •

edited

Loading

joelanford Sep 19, 2020 •

edited

Loading