Unpack bundles #1215

njhale · 2020-01-06T16:00:51Z

Description of the change:

A refactor and continuation of #1081

Motivation for the change:

Simplification of InstallPlan updates and unpacking job management.

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

njhale · 2020-01-07T01:13:45Z

/retest

njhale · 2020-01-07T12:42:40Z

/retest

njhale · 2020-01-07T14:31:05Z

/retest

ecordell · 2020-01-07T20:15:48Z

pkg/controller/bundle/bundle_unpacker.go

+}
+
+func (c *configmapUnpacker) ensureRoleBinding(roleNamespace string) (roleBinding *rbacv1.RoleBinding, err error) {
+	fresh := &rbacv1.RoleBinding{


shouldn't these ensured resources have some reasonable ownerrefs?

Okay. I'll add ownerrefs in the direction of Role/RoleBindings/Jobs -> ConfigMap.

The ConfigMap itself is a bit more tricky since there can exist InstallPlans that effectively reference it across several namespaces. The sensible thing to me is to add an ownerref in the direction of ConfigMap -> CatalogSource, and then propagate an error condition on the BundleLookup if it's not present when ensuring the ConfigMap.

there can exist InstallPlans that effectively reference it across several namespaces

Can you describe the case where this happens? Won't everything be scoped to one namespace?

Also - ownerrefs may not be needed at all if OLM will perform the cleanup (which might be nice for the rbac; it's not really needed after the job runs)

ecordell · 2020-01-07T20:18:27Z

pkg/controller/bundle/bundle_unpacker.go

+		Rules: []rbacv1.PolicyRule{rule},
+	}
+	fresh.SetNamespace(configmapNamespace)
+	fresh.SetName(c.rbacName)


since this uses the rbacName set for the bundle unpacker, won't this cause an issue for multiple bundle unpacks in the same namespace at the same time? Shouldn't the role name be based on the configmapName in some way (or generated with labels/ownerrefs?)

Instead of generating a role per bundle, we can aggregate rules on a single role that gets updated. This should shake out to be eventually consistent in the event of contention between concurrent bundle. It's simple enough to switch it over -- I'll do that.

Just to clarify:

ResourceNames: []string{ configmapName, },

with

fresh.SetName(c.rbacName)

and above, c.rbacName is set:

rbacName: "olm-bundle-unpacker",

won't this mean that the ResourceNames will be stomped over for this one Role if multiple bundles are unpacked in the same namespace?

just clarifying because I don't think the aggregation is really needed, I think we just need a role per configmap/job

It should end up being a separate PolicyRule in the same Role.

ecordell · 2020-01-07T22:11:58Z

pkg/controller/bundle/bundle_unpacker.go

+
+	cond := result.GetCondition(operatorsv1alpha1.BundleLookupPending)
+	now := c.now()
+	if job == nil || len(job.Status.Conditions) == 0 || len(job.Status.Conditions) > 0 && job.Status.Conditions[0].Type != batchv1.JobComplete {


non-blocking: there may be some nice ways to clean this up a bit, e.g. job.Status.ConditionMatches(operatorsv1alpha1.BundleLookupPending, func(Condition) bool { } )

ecordell · 2020-01-07T22:22:46Z

pkg/controller/bundle/bundle_unpacker.go

+}
+
+func (c *configmapUnpacker) ensureRoleBinding(roleNamespace string) (roleBinding *rbacv1.RoleBinding, err error) {
+	fresh := &rbacv1.RoleBinding{


there can exist InstallPlans that effectively reference it across several namespaces

Can you describe the case where this happens? Won't everything be scoped to one namespace?

Also - ownerrefs may not be needed at all if OLM will perform the cleanup (which might be nice for the rbac; it's not really needed after the job runs)

ecordell · 2020-01-07T22:58:32Z

pkg/controller/registry/resolver/generation.go

@@ -67,6 +83,16 @@ func NewGenerationFromCluster(csvs []*v1alpha1.ClusterServiceVersion, subs []*v1
 	return g, nil
 }

+func (g *NamespaceGeneration) AddPendingOperator(l LaunchBundleImageInfo) {
+	g.pendingOperators[l] = struct{}{}


Unless I'm missing it - this isn't adjusting the provided/required apis for the current generation?

ecordell · 2020-01-07T22:59:05Z

pkg/controller/registry/resolver/generation_test.go

-				requiredAPIs:  EmptyAPIMultiOwnerSet(),
-				uncheckedAPIs: EmptyAPISet(),
-				missingAPIs:   EmptyAPIMultiOwnerSet(),
+				pendingOperators: BundleImageSet{},


this file is missing tests for Add/Remove pending operators

ecordell · 2020-01-07T23:00:03Z

pkg/controller/registry/resolver/resolver.go

@@ -93,26 +100,20 @@ func (r *OperatorsV1alpha1Resolver) ResolveSteps(namespace string, sourceQuerier
 		}

 		// add steps for any new bundle
-		if op.Bundle() != nil {
-			bundleSteps, err := NewStepResourceFromBundle(op.Bundle(), namespace, op.Replaces(), op.SourceInfo().Catalog.Name, op.SourceInfo().Catalog.Namespace)
+		if op.Bundle().BundlePath == "" {


double check - can op.Bundle() be nil?

ecordell · 2020-01-07T23:00:22Z

pkg/controller/registry/resolver/resolver.go

@@ -122,14 +123,25 @@ func (r *OperatorsV1alpha1Resolver) ResolveSteps(namespace string, sourceQuerier
 			}
 		}

+		if op.Bundle().BundlePath != "" {


same - double check that op.Bundle() can't return nil

ecordell · 2020-01-07T23:04:36Z

pkg/lib/queueinformer/resourcequeue.go

@@ -68,6 +69,29 @@ func (r *ResourceQueueSet) Requeue(namespace, name string) error {
 	return fmt.Errorf("couldn't find queue for resource")
 }

+// TODO: this may not actually be required if the requeue is done on the namespace rather than the installplan
+// RequeueAfter requeues the resource in the set with the given name and namespace (just like Requeue), but only does so after duration has passed
+func (r *ResourceQueueSet) RequeueAfter(namespace, name string, duration time.Duration) error {


I don't see this used anywhere?

This is being used in the installplan sync.

ecordell · 2020-01-07T23:06:12Z

pkg/controller/registry/resolver/resolver_test.go

 			querier := NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{catalog: tt.bundlesInCatalog})
-			steps, subs, err := resolver.ResolveSteps(namespace, querier)
+			steps, _, subs, err := resolver.ResolveSteps(namespace, querier)


should have some tests that return bundle lookups, right?

yup! good catch.

njhale · 2020-01-08T06:49:17Z

/retest

njhale · 2020-01-08T13:07:49Z

/retest

njhale · 2020-01-09T16:52:14Z

/retest

ecordell · 2020-01-09T14:42:23Z

pkg/controller/operators/catalog/operator.go

@@ -85,17 +86,14 @@ type Operator struct {
 	catalogSubscriberIndexer map[string]cache.Indexer
 	clientAttenuator         *scoped.ClientAttenuator
 	serviceAccountQuerier    *scoped.UserDefinedServiceAccountQuerier
+	bundleUnpacker           bundle.Unpacker
+	configmapRegistryImage   string


shouldn't this be "unpackerImage"?

ecordell · 2020-01-09T16:54:39Z

pkg/controller/registry/resolver/operators.go

@@ -263,7 +264,11 @@ func NewOperatorFromBundle(bundle *api.Bundle, startingCSV string, sourceKey Cat
 	if len(required) == 0 && len(provided) == 0 {
 		// fallback to csv parsing
 		if bundle.CsvJson == "" {
-			return nil, fmt.Errorf("couldn't parse bundle")
+			if bundle.GetBundlePath() != "" {


this doesn't seem like the right place - if we're falling back to CSV parsing, we don't care if there's a bundlepath at all.

I'll admit it seems odd, but I think we still want to throw a specific error when a bundlepath isn't accompanied by the expected metadata. WDYT?

njhale · 2020-01-09T19:42:18Z

/retest

njhale · 2020-01-09T20:27:39Z

/retest

njhale · 2020-01-09T21:05:12Z

/retest

njhale · 2020-01-10T00:05:58Z

/retest

njhale · 2020-01-10T02:55:15Z

/retest

njhale · 2020-01-10T16:53:47Z

/retest

Specifically, pulls in v1.5.5.

This extracts bundle data from an image and uses that data to populate the installplan. (The code here depends on later commits.)

WIP: this is currently using a local custom image.

Instead use image that is pullable from quay. Test data has been updated to include image building files.

The test data for generating this custom image uses a go script to generate and populate an operator registry database that references a bundle image.

ecordell · 2020-01-10T18:37:51Z

/lgtm

openshift-ci-robot · 2020-01-10T18:38:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, njhale

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ecordell,njhale]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 6, 2020

openshift-ci-robot requested review from ecordell and jpeeler January 6, 2020 16:01

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 6, 2020

njhale force-pushed the unpack-bundles branch 3 times, most recently from 2fb8fc7 to c5b3346 Compare January 6, 2020 23:34

njhale force-pushed the unpack-bundles branch 2 times, most recently from 84688ca to fbeb16e Compare January 7, 2020 19:54

njhale changed the title ~~WIP: Unpack bundles~~ Unpack bundles Jan 7, 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 Jan 7, 2020

njhale force-pushed the unpack-bundles branch 2 times, most recently from 1d56c78 to 68fe005 Compare January 7, 2020 20:11

ecordell reviewed Jan 7, 2020

View reviewed changes

njhale force-pushed the unpack-bundles branch from 68fe005 to 7ff5a7f Compare January 7, 2020 22:38

ecordell reviewed Jan 7, 2020

View reviewed changes

njhale force-pushed the unpack-bundles branch from 7ff5a7f to 9ef16e0 Compare January 8, 2020 05:04

njhale force-pushed the unpack-bundles branch 3 times, most recently from 75ef40c to b4ec643 Compare January 9, 2020 14:28

ecordell reviewed Jan 9, 2020

View reviewed changes

njhale force-pushed the unpack-bundles branch from b4ec643 to 2773fd3 Compare January 9, 2020 18:50

openshift-ci-robot assigned ecordell Jan 9, 2020

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

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

jpeeler and others added 9 commits January 10, 2020 12:56

chore(vendor): get latest operator-registry

79bd139

Specifically, pulls in v1.5.5.

feat(catalog): make install possible from bundle image

05e1615

This extracts bundle data from an image and uses that data to populate the installplan. (The code here depends on later commits.)

feat(bundleimage): resolve bundle images

ed1db7b

chore(codegen): make codegen

b6f484f

chore(fakes): make mockgen

640cff3

test(e2e): add test for bundle image

a2af51c

WIP: this is currently using a local custom image.

fix(e2e): remove local dev test image

914ae21

Instead use image that is pullable from quay. Test data has been updated to include image building files.

fix(e2e): remove local dev test registry image

bf5e886

The test data for generating this custom image uses a go script to generate and populate an operator registry database that references a bundle image.

fix(catalog): add ownerreferences for bundle image resources

aee24f4

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

njhale added 3 commits January 10, 2020 13:16

refactor(resolve): simplify bundle lookups

e8b9580

docs(design): add bundle image unpack design

3772cae

test(e2e): fix subscription update flake by retrying

4cef16c

njhale force-pushed the unpack-bundles branch from 2d03335 to 4cef16c Compare January 10, 2020 18:18

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

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

openshift-merge-robot merged commit 2b93a4b into operator-framework:master Jan 10, 2020

jpeeler mentioned this pull request Jan 10, 2020

Resolve and unpack operators from bundle images #1081

Closed

njhale deleted the unpack-bundles branch July 23, 2021 02:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unpack bundles #1215

Unpack bundles #1215

njhale commented Jan 6, 2020

njhale commented Jan 7, 2020

njhale commented Jan 7, 2020

njhale commented Jan 7, 2020

ecordell Jan 7, 2020

njhale Jan 7, 2020

ecordell Jan 7, 2020

ecordell Jan 7, 2020

njhale Jan 7, 2020

ecordell Jan 7, 2020 •

edited

njhale Jan 7, 2020

ecordell Jan 7, 2020

ecordell Jan 7, 2020

ecordell Jan 7, 2020

ecordell Jan 7, 2020

ecordell Jan 7, 2020

ecordell Jan 7, 2020

ecordell Jan 7, 2020

njhale Jan 8, 2020

ecordell Jan 7, 2020

njhale Jan 8, 2020

njhale commented Jan 8, 2020

njhale commented Jan 8, 2020

njhale commented Jan 9, 2020

ecordell Jan 9, 2020

ecordell Jan 9, 2020

njhale Jan 9, 2020

njhale commented Jan 9, 2020

njhale commented Jan 9, 2020

njhale commented Jan 9, 2020

njhale commented Jan 10, 2020

njhale commented Jan 10, 2020

njhale commented Jan 10, 2020

ecordell commented Jan 10, 2020

openshift-ci-robot commented Jan 10, 2020

Unpack bundles #1215

Unpack bundles #1215

Conversation

njhale commented Jan 6, 2020

njhale commented Jan 7, 2020

njhale commented Jan 7, 2020

njhale commented Jan 7, 2020

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

ecordell Jan 7, 2020 • edited

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

njhale commented Jan 8, 2020

njhale commented Jan 8, 2020

njhale commented Jan 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale commented Jan 9, 2020

njhale commented Jan 9, 2020

njhale commented Jan 9, 2020

njhale commented Jan 10, 2020

njhale commented Jan 10, 2020

njhale commented Jan 10, 2020

ecordell commented Jan 10, 2020

openshift-ci-robot commented Jan 10, 2020

ecordell Jan 7, 2020 •

edited