cmd/operator-sdk/alpha: 'up/down olm' commands to run an operator #1912

estroz · 2019-09-12T22:44:01Z

Description of the change:

cmd/operator-sdk/alpha: 'up/down olm' commands to run an operator on a cluster that has OLM installed
internal/olm/operator: logic to load an operator bundle into a registry Deployment from a ConfigMap and serve that bundle from an operator-registry server to OLM
internal/util/operator-registry: use registry bundle utilities to parse a manifests dir; some file renaming and added validation

Motivation for the change: OLM integration POC.

Test using:

$ operator-sdk alpha olm install
$ operator-sdk alpha up olm ${bundle_path} --operator-version ${semantic_version}
$ operator-sdk alpha down olm ${bundle_path} --operator-version ${semantic_version}

or integration tests:

$ make test-integration

Notes:

Lots of TODO's/FEAT's kicking around the source. Let me know if you have opinions about any of them.
Needs integration tests for more features. Simple one implemented.
Command line naming is open to change; it doesn't implement exactly what the design doc specifies.
The operator-registry dependency is using a branch from my fork until pkg/registry: use v1beta1.CustomResourceDefinition, as the apiextensions type is internal operator-registry#86 is merged/addressed.
I have broken out a few pieces of this PR into smaller PR's and will do the same with bundle utils.

/cc @dmesser @ecordell

on a cluster that has OLM installed internal/olm/operator: logic to load an operator bundle into a registry Deployment from a ConfigMap and serve that bundle from an operator-registry server to OLM

joelanford

Just a quick review with some questions and nits. Definitely want to give this a test run.

cmd/operator-sdk/alpha/cmd.go

joelanford · 2019-09-16T20:06:59Z

cmd/operator-sdk/alpha/cmd.go

+	cmd.AddCommand(
+		olm.NewCmd(),
+		up.NewCmd(),
+		down.NewCmd(),


Do we need a down subcommand or can we make up olm/olm up work like up local, where we could create the resources, wait for the operator to run, tail the logs, and wait for the user to terminate the process. Then we could catch the signal, and tear everything down cleanly.

One problem with that approach (and there are probably others, e.g. scorecard use cases?) would be the user using SIGKILL, which cannot be caught and handled.

Thoughts?

I imagine using operator-sdk alpha olm up to deploy an operator for both production and test purposes, so IMO having a process that doesn't return gives a test-only feel. down is for convenience, and likely won't/shouldn't be used in production.

I think this needs more discussion regardless, since there are a few use cases to consider.

internal/olm/operator/internal/configmap.go

internal/olm/operator/internal/deployment.go

joelanford · 2019-09-16T20:42:33Z

internal/olm/operator/manager.go

+	if m.force {
+		log.Printf("Forcefully recreating registry")
+	} else {
+		log.Printf("Registry data stale. Recreating registry")


Right now, it seems like the only difference between --force=true and --force=false is that in the former case, we'll delete and re-create the registry even when the registry data is correct.

Is there a use case for that? Maybe to get a newer registry image version?

Also, without --force, we still delete and re-create when the data is stale. Is there a use case that dictates that we should bail out with stale data and require the --force flag?

In this particular case --force=true isn't really useful, because as you say its not doing anything that a cache miss wouldn't.

I can't think of any case where we wouldn't want to rebuild the registry if its data is stale.

One slightly related thing I just thought of is that we don't necessarily have to re-create a Subscription and CatalogSource when rebuilding the registry, since they'll be pointing to the same registry server address. WDYT?

internal/olm/operator/olm.go

estroz · 2019-09-18T04:04:18Z

internal/olm/operator/manager.go

+	// not the other is an error.
+	hasSub, hasCatSrc := m.hasSubscription(), m.hasCatalogSource()
+	if hasSub || hasCatSrc && !(hasSub && hasCatSrc) {
+		return nil, errors.New("both a CatalogSource and Subscription must be supplied if one is supplied")


We can likely relax this requirement in the future, as we can accept an external CatalogSource and write a Subscription internally referencing that CatalogSource.

estroz · 2019-11-26T22:19:48Z

@camilamacedo86 alpha olm install/uninstall is already in master. Only alpha olm up/down are being added in this PR.

estroz · 2019-12-04T16:41:12Z

internal/olm/operator/tenancy.go

+// with UnsupportedOperatorGroup.
+//
+// https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/operatorgroups.md
+func (m *operatorManager) operatorGroupUp(ctx context.Context) error {


@njhale @kevinrizza does this OperatorGroup logic make sense? I'm doing a thorough writeup of this logic and push that soon, which will further clarify my thinking.

Proposed logic: #2324

estroz · 2019-12-12T03:08:55Z

/hold

waiting on #2313 and #2324

openshift-ci-robot · 2019-12-12T03:09:02Z

@estroz: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

estroz · 2020-01-15T19:29:56Z

Closing in favor of #2402

cmd/operator-sdk/alpha: 'up/down olm' commands to run an operator

b890ef5

on a cluster that has OLM installed internal/olm/operator: logic to load an operator bundle into a registry Deployment from a ConfigMap and serve that bundle from an operator-registry server to OLM

estroz added kind/feature Categorizes issue or PR as related to a new feature. olm-integration Issue relates to the OLM integration labels Sep 12, 2019

estroz requested review from jmrodri, joelanford, jmccormick2001, hasbro17, theishshah and camilamacedo86 September 12, 2019 22:44

openshift-ci-robot requested review from dmesser and ecordell September 12, 2019 22:44

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 12, 2019

estroz mentioned this pull request Sep 12, 2019

doc/proposals/sdk-integration-with-olm.md: SDK/OLM integration… #1913

Merged

fix unit test

2da1009

estroz force-pushed the olm-poc branch from 29f09b2 to 2da1009 Compare September 13, 2019 00:08

estroz added 3 commits September 12, 2019 18:22

remove excessive Deployment mount paths

ea20611

reorganize imports, move DNS 1123 label formatter to registry utils

7324bfd

internal/olm/operator/internal: implement stale registry data check

17471e5

estroz force-pushed the olm-poc branch from 3bad844 to 17471e5 Compare September 13, 2019 03:12

estroz added 2 commits September 13, 2019 09:45

bump test timeout

fefa150

generalize manifests field passed to OLMCmd

6c89c93

joelanford reviewed Sep 16, 2019

View reviewed changes

estroz added 4 commits September 17, 2019 18:00

handle OperatorGroup's and namespacing correctly

86f170f

use integration test suite instead of subcommand tests

11e63dd

fix up tests

e62ca67

fix test image

bc43140

estroz commented Sep 18, 2019

View reviewed changes

estroz added 2 commits September 17, 2019 22:26

prow CI integration tests and Makefile target

4e11d1d

redo CLI

5ef2709

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

estroz added 5 commits November 21, 2019 15:48

go.sum: revendor

3842b55

Merge branch 'master' into olm-poc

ec2fab6

update comment and CRD scheme installation

50ee689

go.mod: bump OLM to 0.13.0 which uses kk8s 1.16

a97f01e

update test/test-framework go.sum

c382576

estroz added 8 commits November 27, 2019 14:04

Merge branch 'master' into olm-poc

e61fd47

Makefile: ignore all files in test/ in test-unit recipe

a87c6e9

minor changes to integration test script

ed368ae

check errs

ea728e3

fix spelling

c489e80

Merge branch 'master' into olm-poc

4a303c3

fix linter errors

94382d2

Merge branch 'master' into olm-poc

d95369e

estroz mentioned this pull request Dec 4, 2019

internal/olm: make CLI output text more consistent; check resource errors #1902

Merged

estroz commented Dec 4, 2019

View reviewed changes

estroz mentioned this pull request Dec 10, 2019

internal/olm/operator/internal: operator-registry wrappers #2313

Merged

estroz added 2 commits December 10, 2019 11:39

Merge branch 'master' into olm-poc

b695492

update status method name after merge

5e150cc

estroz mentioned this pull request Dec 11, 2019

internal/olm/operator: OLM operator manager #2320

Merged

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 12, 2019

estroz mentioned this pull request Dec 12, 2019

doc/proposals: update OLM integration proposal with OperatorGroup logic #2324

Closed

estroz mentioned this pull request Jan 13, 2020

cmd/operator-sdk: OLM integration alpha run/cleanup CLI #2402

Merged

estroz closed this Jan 15, 2020

estroz deleted the olm-poc branch April 1, 2020 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/operator-sdk/alpha: 'up/down olm' commands to run an operator #1912

cmd/operator-sdk/alpha: 'up/down olm' commands to run an operator #1912

estroz commented Sep 12, 2019 •

edited

Loading

joelanford left a comment

joelanford Sep 16, 2019

estroz Sep 18, 2019

estroz Sep 18, 2019

joelanford Sep 16, 2019

estroz Sep 18, 2019

estroz Sep 18, 2019

estroz commented Nov 26, 2019

estroz Dec 4, 2019

estroz Dec 12, 2019

estroz commented Dec 12, 2019 •

edited

Loading

openshift-ci-robot commented Dec 12, 2019

estroz commented Jan 15, 2020

cmd/operator-sdk/alpha: 'up/down olm' commands to run an operator #1912

cmd/operator-sdk/alpha: 'up/down olm' commands to run an operator #1912

Conversation

estroz commented Sep 12, 2019 • edited Loading

joelanford left a 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

estroz commented Nov 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz commented Dec 12, 2019 • edited Loading

openshift-ci-robot commented Dec 12, 2019

estroz commented Jan 15, 2020

estroz commented Sep 12, 2019 •

edited

Loading

estroz commented Dec 12, 2019 •

edited

Loading