feat(catalog): multiple CatalogSource resolution #386

njhale · 2018-07-18T11:47:12Z

Description

Adds multi-catalog resolution of CSVs and CRDs.

ecordell

Some small things, but looks great overall!

ecordell · 2018-07-18T11:49:19Z

pkg/controller/operators/catalog/operator.go

-		sourcesSnapshot[key] = source
-	}
-	o.sourcesLock.RUnlock()
+	sourcesSnapshot := o.getSourcesSnapshot()


ecordell · 2018-07-18T11:54:14Z

pkg/controller/operators/catalog/operator.go

-		steps, notFoundErr = o.dependencyResolver.ResolveInstallPlan(sourcesSnapshot, srcKey, CatalogLabel, plan)
-		if notFoundErr != nil {
-			continue
+		// Get the first key (arbitrary)


Do we have a followup issue/story to pass the desired catalog through to here?

Not that I can see. Could we have a discussion on what the criteria of a desired first catalog is?

ecordell · 2018-07-18T11:59:30Z

pkg/controller/registry/resolver/resolver_test.go

+	}
+	var table = []struct {
+		description string
+		planCSVName string


doesn't seem like you need the planCSVName field for anything (can be constant or generated per test) so may as well remove it here

planCSVName specifies the main CSV the InstallPlan must resolve for each test in the test table. If we remove and hard-code we lose some flexibility with the naming of that CSV. (I'm wondering if it's worth taking out)

ecordell · 2018-07-18T12:05:48Z

pkg/controller/registry/resolver/resolver_test.go

+		csvs        map[string]csvName
+		crds        map[string]crdName
+		srcKeys     []registry.SourceKey
+		expectedErr error


Instead of building the expected steps from the input csvs/crds, let's just have a list of expected steps (can include secrets then, too)?

alecmerdler

Looks good, just would be nice to add namespace to status.catalogSources list as well.

alecmerdler · 2018-07-19T16:37:00Z

pkg/api/apis/installplan/v1alpha1/types.go

-	Name          string `json:"name"`
-	Manifest      string `json:"manifest,omitempty"`
+	CatalogSource          string `json:"sourceName"`
+	CatalogSourceNamespace string `json:"sourceNamespace"`


Can we change status.catalogSources to be a list with both the name and namespace of the CatalogSources as well?

InstallPlanStatus.CatalogSources as a []registry.SourceKey?

alecmerdler

Limit number of CatalogSources that we resolve from.

alecmerdler · 2018-07-20T19:59:42Z

pkg/controller/registry/resolver/resolver.go


 		if err == nil {
 			srm.Combine(csvSRM)
+			usedSourceKeys = append(used, usedSourceKeys...)
 			continue
 		}

 		// Attempt to resolve from any other CatalogSource
 		for srcKey := range sources {


Only resolve from CatalogSources in the same namespace of the InstallPlan or the "global" namespace (o.namespace).

I reduced the catalog sources snapshot that gets passed in to only contain sources from plan.Namespace and o.namespace. That should address this issue.

ecordell · 2018-07-23T23:57:21Z

pkg/controller/operators/catalog/operator.go

-	// Copy the sources for resolution
-	sourcesSnapshot := o.getSourcesSnapshot()
+	// Copy the sources for resolution from the included namespaces
+	includedNamespaces := map[string]bool{


typically do map[string]struct{} to avoid allocating the bool

ecordell · 2018-07-24T12:27:49Z

pkg/controller/operators/catalog/operator.go

-
-	var preferredSourceKey registry.SourceKey
-
+func (o *Operator) getPreferredSourceKey(plan *v1alpha1.InstallPlan, includedNamespaces map[string]bool) registry.SourceKey {


I think this method makes more sense if it takes sourcesSnapshot as an arg instead of included namespaces, since you've already filtered the sources by includedNamespaces above

ecordell · 2018-07-24T12:30:56Z

pkg/controller/operators/catalog/operator.go

+	var preferredSourceKey registry.SourceKey
+
+	// Attempt to get the plan's source
+	if _, ok := includedNamespaces[plan.Spec.CatalogSource]; ok {


this part wouldn't be needed if the source list were passed in, but as it is now I think it's supposed to be plan.Spec.CatalogSourceNamespace

ecordell · 2018-07-24T12:35:40Z

pkg/controller/registry/resolver/resolver.go

@@ -215,24 +220,24 @@ func (resolver *MultiSourceResolver) resolveCSV(sources map[registry.SourceKey]r
 		}

 		// registry.SourceKey for the source containing the CSV
-		csvSrcKey := firstSrcKey
+		csvSourceKey := preferredSourceKey


var csvSourceKey SourceKey

(easier to catch bugs where this isn't being set properly if it's not initialized to a valid value)

ecordell · 2018-07-24T12:53:42Z

pkg/controller/registry/resolver/resolver.go

-	// Attempt to get the first source
-	source, ok := sources[firstSrcKey]
+	// Attempt to get the preferredSource
+	source, ok := sources[preferredSourceKey]


There's a ton of code scattered throughout for managing the "preferred source". Ideally, these methods should not have to care about which source is preferred. This isn't something we have to fix now, it can be a followup or it can wait until we have better plans for a total-ordering of sources.

We could always change the ResolveInstallPlan sources parameter to be a pre-ordered, pre-filtered slice of Sources. Another possible alternative is that we establish some criteria for general Source priority and pass in a priority queue.

…terface

…ourceResolver

…atalogSources

…tallPlan

…n owning subscription

…namespaces

…custom resource

…n newer kubeadm versions)

… a source snapshot

…eter

…urce

ecordell

LGTM pending CI

Verbal agreement

…solution feat(catalog): multiple CatalogSource resolution

njhale requested review from ecordell and alecmerdler July 18, 2018 11:47

ecordell requested changes Jul 18, 2018

View reviewed changes

alecmerdler requested changes Jul 19, 2018

View reviewed changes

njhale force-pushed the multi-source-resolution branch from f899556 to 7a0765a Compare July 19, 2018 21:02

njhale changed the title ~~Multi source resolution~~ feat(catalog): multiple CatalogSource resolution Jul 20, 2018

alecmerdler previously requested changes Jul 20, 2018

View reviewed changes

ecordell requested changes Jul 24, 2018

View reviewed changes

njhale added 21 commits July 24, 2018 13:20

feat(resolver): add CatalogSourceNamespace field to StepResource struct

d2f46ed

refactor(resolver): remove extra functions from DependencyResolver in…

faa81b6

…terface

refactor(catalog): add helper method for getting a sources snapshot

d4c81d8

test(resolver): add tests for MultiSourceResolver

c75e110

feat(catalog): use MultiSourceResolver in catalog operator

38ad7a4

fix(resolver): set resolved csv namespace to plan namespace in MultiS…

e8d8194

…ourceResolver

chore(.gitignore): remove vscode file exceptions

eac0b9b

refactor(InstallPlan): use registry.SourceKey to keep track of used C…

ddedab7

…atalogSources

chore(registry): dedupe to in comment

efa5fd5

refactor(resolver): return used catalog sources slice from ResolveIns…

3bc0a3c

…tallPlan

test(resolve): decouple expected results from test input

1441985

feat(registry): add subscription key type

4225400

feat(catalog): add helper method to get preferred source key from pla…

438bf78

…n owning subscription

feat(catalog): add catalog and catalog namespace to install plan spec

94e3a98

refactor(catalog): limit sources snapshot to global and install plan …

538c472

…namespaces

refactor(InstallPlan): add source name and namespace to install plan …

a003d17

…custom resource

fix(e2e): apply charts from templates directory

7c5a5c4

fix(e2e-local): remove extra RBAC config on minikube start (default i…

7617033

…n newer kubeadm versions)

refactor(catalog): change parameters of getPrefferedSourceKey to take…

d3c380d

… a source snapshot

refactor(resolver): accept prioritized list of source refs as a param…

ec59313

…eter

fix(subscriptions_test): replace single source resolver with multi so…

edb8cff

…urce

njhale force-pushed the multi-source-resolution branch from 577741b to edb8cff Compare July 24, 2018 17:28

ecordell approved these changes Jul 24, 2018

View reviewed changes

njhale merged commit a7295d4 into operator-framework:master Jul 24, 2018

njhale deleted the multi-source-resolution branch July 27, 2018 15:16

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

Merge pull request operator-framework#386 from njhale/multi-source-re…

906defb

…solution feat(catalog): multiple CatalogSource resolution

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

Merge pull request operator-framework#386 from njhale/multi-source-re…

8b0f68e

…solution feat(catalog): multiple CatalogSource resolution

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(catalog): multiple CatalogSource resolution #386

feat(catalog): multiple CatalogSource resolution #386

njhale commented Jul 18, 2018

ecordell left a comment

ecordell Jul 18, 2018

ecordell Jul 18, 2018

njhale Jul 18, 2018

ecordell Jul 18, 2018

njhale Jul 18, 2018

ecordell Jul 18, 2018

njhale Jul 18, 2018 •

edited

Loading

alecmerdler left a comment

alecmerdler Jul 19, 2018

njhale Jul 19, 2018

njhale Jul 23, 2018

alecmerdler left a comment

alecmerdler Jul 20, 2018

njhale Jul 23, 2018

ecordell Jul 23, 2018

ecordell Jul 24, 2018

ecordell Jul 24, 2018

ecordell Jul 24, 2018

ecordell Jul 24, 2018

njhale Jul 24, 2018

ecordell left a comment


		var preferredSourceKey registry.SourceKey

		func (o Operator) getPreferredSourceKey(plan v1alpha1.InstallPlan, includedNamespaces map[string]bool) registry.SourceKey {

feat(catalog): multiple CatalogSource resolution #386

feat(catalog): multiple CatalogSource resolution #386

Conversation

njhale commented Jul 18, 2018

Description

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

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 Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

alecmerdler 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

alecmerdler 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

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 Jul 18, 2018 •

edited

Loading