[WIP] feat(resolver): add namespace and channel awareness #402

njhale · 2018-07-31T22:04:46Z

Description

Makes dependency resolution aware of default package channels and preexisting CSVs in an install plan's namespace.

Roughly addresses ALM-641 and ALM-469

ecordell · 2018-08-02T13:02:01Z

pkg/controller/operators/catalog/operator.go

@@ -41,10 +41,10 @@ type Operator struct {
 	*queueinformer.Operator
 	client             versioned.Interface
 	namespace          string
-	sources            map[registry.SourceKey]registry.Source


This is a bigger wrench than I intended to throw when I started looking at this PR...

But it strikes me that this type of tracking (for both "sources" and "subscriptions") is exactly what the cache and indexers are supposed to do for you in client-go.

We already have SharedIndexInformers for all of this available to the operator, so we can probably just use the index provided there.

Take a look at the cache tooling and see if you agree: https://github.com/kubernetes/client-go/tree/master/tools/cache

I would guess that it's as simple as keeping track of the SharedIndexInformers by namespace and then querying those.

I think this makes a lot of sense when dealing with subscriptions since we don't need to do any additional work to pull it from the indexer's cache/store. Sources however, need an InMem instance constructed from the catalog source object and its config map before they are useful.

Good point!

A future optimization to consider would be to disallow "internal" catalogsources (replacing them with pods that expose an API over the network), which would let us do the same caching for sources and remove the need to parse and keep all the contents in memory.

Thanks!

Also, it's questionable at this point whether knowing about subscriptions at resolution time matters as much as knowing what's in the target namespace.

Tracked in ALM-679, ignore this for this PR

ecordell · 2018-08-07T11:30:09Z

pkg/controller/operators/catalog/operator.go

+// getExistingCSVNames returns a set of existing CSV names for the given namespace
+func (o *Operator) getExistingCSVNames(namespace string) (map[string]struct{}, error) {
+	// Get a list of CSV CRs in the namespace
+	csvList, err := o.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).List(metav1.ListOptions{Watch: false})


pretty sure Watch is default false

ecordell · 2018-08-07T11:43:24Z

pkg/controller/operators/catalog/operator.go

+	}
+
+	// Create a set of CSV names from the list
+	existing := make(map[string]struct{})


There's an edge case with this (and the consumer side in resolveCRDDescription). There can be more than one CSV in a namespace that owns a CRD for a couple of reasons:

Could be in the middle of upgrading

Could have attempted to create one and failed (so status is failed but owner is still there)

Anyone with csv create permission can write arbitrary owned CRDs (they won't be accepted, but they can write them).

The simplest way to fix this would be to just fail the installplan if we see two csvs that own the CRD already in the namespace (we can't tell which to resolve). Ideally we would just retry the resolution after a bit, and only mark the installplan as truly "Failed" if we've tried a few times and never got a single owner for a CRD (which indicates a bad namespace state - something on the CSV side got stuck).

We could try to detect the upgrading states of the CSVs and watch them directly, but I'd rather keep things more decoupled between catalog/olm.

Anyway, this is returning a list of existing CSVs in the namespace, but I think what you actually want is a map from a CRDName to the CSVName in the namespace that owns it (then you don't have to loop both here and in resolveCRDDescription). Depending on where it makes sense to have the failing logic, it may need to map crdname to a list of owners.

ecordell · 2018-08-07T11:47:05Z

pkg/controller/registry/resolver/resolver_test.go

+		},
+	}
+
+	for _, tt := range table {


want some test cases for when there are multiple CSVs that own the CRD in the namespace

ecordell · 2018-08-07T20:19:23Z

pkg/controller/registry/resolver/resolver_test.go

@@ -399,10 +496,10 @@ func TestMultiSourceResolveInstallPlan(t *testing.T) {
 	resolver := &MultiSourceResolver{}

 	// Test single catalog source resolution
-	resolveInstallPlan(t, resolver)
+	// resolveInstallPlan(t, resolver)


what's going on here?

This change enables dependency resolution to choose a (namespace) prexisting CSV to add as a step resource or if one does not exist, choose the CSV belonging to the package's default channel. This change also introduces wait for delete logic in installplan, csv, and crd cleanup functions. refactor(registry): export InMem.addPackageManifest method refactor(resolver): add awareness of prexisting CRD owners in target namespace Sends a mapping of CRD to existing owners in namespace as parameter to resolver. This allows the resolver to return an error if more than one preexisting CSV is discovered for a given dependent CRD. chore(vendor): stop tracking vendor directory

ecordell · 2018-08-08T12:05:38Z

Gopkg.lock

@@ -2,380 +2,487 @@


 [[projects]]
+  digest = "1:d8ebbd207f3d3266d4423ce4860c9f3794956306ded6c7ba312ecc69cdfbf04c"


can you re-vendor with dep stable?

ecordell

looks really good! one small thing

ecordell · 2018-08-08T12:14:08Z

pkg/controller/registry/resolver/resolver.go

 	crdKey := registry.CRDKey{
 		Kind:    crdDesc.Kind,
 		Name:    crdDesc.Name,
 		Version: crdDesc.Version,
 	}
-	logger.Debug("resolving")


thanks for cleaning up logs!

ecordell · 2018-08-08T12:20:45Z

pkg/controller/registry/resolver/resolver.go

-	logger.WithField("owner", csvs[0].CSV.Name).Info("found owner")
-	return v1alpha1.StepResource{}, csvs[0].CSV.Name, nil
+	var ownerName string
+	if owners, ok := existingCRDOwners[crdKey.Name]; ok && len(owners) > 0 {


could we make this control flow clearer?

doesn't have to be this, but something that removes the nesting:

owners, ok := existingCRDOwners[crdKey.Name] if !ok { ownerCount = 0 } else { ownerCount = len(owners) } switch len(owners): case 0: // get default channel case 1: // found an owner case >1: // error }

ecordell

LGTM

…-resolution feat(resolver): add namespace and channel awareness

ecordell reviewed Aug 2, 2018

View reviewed changes

njhale force-pushed the namespace-aware-resolution branch from a5bb9ce to 788a510 Compare August 3, 2018 11:49

ecordell requested changes Aug 7, 2018

View reviewed changes

ecordell reviewed Aug 7, 2018

View reviewed changes

njhale force-pushed the namespace-aware-resolution branch from 6402ae4 to 1cc9fe2 Compare August 7, 2018 22:17

ecordell reviewed Aug 8, 2018

View reviewed changes

chore(dep): re-vendor with dep stable

5a16990

ecordell requested changes Aug 8, 2018

View reviewed changes

njhale added 2 commits August 8, 2018 08:38

refactor(catalog): remove map-based subscription caching

201181e

style(resolver): remove nesting from owner CSV selection logic

4c80d1b

ecordell approved these changes Aug 8, 2018

View reviewed changes

njhale merged commit bed2541 into operator-framework:master Aug 8, 2018

njhale deleted the namespace-aware-resolution branch August 15, 2018 22:55

njhale added a commit to njhale/operator-lifecycle-manager that referenced this pull request Sep 10, 2018

Merge pull request operator-framework#402 from njhale/namespace-aware…

bc76c3f

…-resolution feat(resolver): add namespace and channel awareness

ecordell pushed a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019

Merge pull request operator-framework#402 from njhale/namespace-aware…

a580b44

…-resolution feat(resolver): add namespace and channel awareness

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat(resolver): add namespace and channel awareness #402

[WIP] feat(resolver): add namespace and channel awareness #402

njhale commented Jul 31, 2018 •

edited

Loading

ecordell Aug 2, 2018 •

edited

Loading

njhale Aug 2, 2018

ecordell Aug 2, 2018

njhale Aug 2, 2018

ecordell Aug 7, 2018

ecordell Aug 7, 2018

ecordell Aug 7, 2018 •

edited

Loading

ecordell Aug 7, 2018

ecordell Aug 7, 2018

ecordell Aug 8, 2018

ecordell left a comment

ecordell Aug 8, 2018

ecordell Aug 8, 2018

ecordell left a comment

		@@ -2,380 +2,487 @@


		[[projects]]
		digest = "1:d8ebbd207f3d3266d4423ce4860c9f3794956306ded6c7ba312ecc69cdfbf04c"

[WIP] feat(resolver): add namespace and channel awareness #402

[WIP] feat(resolver): add namespace and channel awareness #402

Conversation

njhale commented Jul 31, 2018 • edited Loading

Description

ecordell Aug 2, 2018 • 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

ecordell Aug 7, 2018 • 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

ecordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell left a comment

Choose a reason for hiding this comment

njhale commented Jul 31, 2018 •

edited

Loading

ecordell Aug 2, 2018 •

edited

Loading

ecordell Aug 7, 2018 •

edited

Loading