Skip to content

[run bundle] Implement IndexImageCatalogCreator - create grpc catalog source #3467

Merged
jmrodri merged 1 commit intooperator-framework:masterfrom
rashmigottipati:create-catalog-source
Aug 18, 2020
Merged

[run bundle] Implement IndexImageCatalogCreator - create grpc catalog source #3467
jmrodri merged 1 commit intooperator-framework:masterfrom
rashmigottipati:create-catalog-source

Conversation

@rashmigottipati
Copy link
Copy Markdown
Member

@rashmigottipati rashmigottipati commented Jul 20, 2020

Description of the change:

  • Create GRPC CatalogSource, pointing to the registry pod address (pointing to .:50051).
  • Update catalog source with source type, address and annotations
  • Verify that catalog source connection is ready

Motivation for the change:

This is one of the building blocks for implementing run bundle and run bundle-upgrade commands.
Links to enhancement proposals:
https://github.com/operator-framework/enhancements/blob/master/enhancements/run-bundle.md

@rashmigottipati rashmigottipati added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2020
@rashmigottipati rashmigottipati self-assigned this Jul 20, 2020
@openshift-ci-robot 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 Jul 20, 2020
@rashmigottipati rashmigottipati force-pushed the create-catalog-source branch from 02905f4 to 9b0aad8 Compare July 20, 2020 19:47
Comment thread internal/olm/operator/catalog_source.go Outdated
Comment thread internal/olm/operator/catalog_source.go Outdated
Comment thread internal/olm/operator/catalog_source.go Outdated
Comment thread internal/olm/operator/catalog_source.go Outdated
Comment thread internal/olm/operator/catalog_source.go Outdated
Comment thread internal/olm/operator/catalog_source_test.go Outdated
Comment thread internal/olm/operator/catalog_source.go Outdated
Comment thread internal/olm/operator/catalog_source.go Outdated
Comment thread internal/olm/operator/catalog_source.go Outdated
Comment thread internal/olm/operator/catalog_source.go Outdated
Comment thread internal/olm/operator/catalog_source_test.go Outdated
Comment thread internal/olm/operator/catalog_source_test.go Outdated
Comment thread internal/olm/operator/catalog_source_test.go Outdated
Comment thread internal/olm/operator/internal/registry.go Outdated
Comment thread internal/olm/operator/internal/registry_pod.go Outdated
@rashmigottipati rashmigottipati force-pushed the create-catalog-source branch 4 times, most recently from f8703cd to c668113 Compare July 21, 2020 19:14
Comment thread internal/olm/operator/catalog_source.go Outdated
@rashmigottipati rashmigottipati force-pushed the create-catalog-source branch 2 times, most recently from ce045ae to 77429a5 Compare July 28, 2020 19:09
@rashmigottipati rashmigottipati force-pushed the create-catalog-source branch from 77429a5 to 6194174 Compare July 28, 2020 21:39
estroz
estroz previously requested changes Jul 28, 2020
Copy link
Copy Markdown
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setup of this CatalogSource seems really weird to me. We're calling Init(internal.RegistryPod), but RegistryPod should be initialized by its own init method. We're also now exporting Pod, which shouldn't be since it is an internal concept.

How about doing something like what exists for packageManifestsManager, which will construct a CatalogSource in its run() method:

type BundleCmd struct {
	// Flag fields
}

type bundleManager struct {
	*operatorManager

	version     	  string
	resourceNamespace string // Where to create OLM resources
	operatorNamespace string
	bundle      	  *apimanifests.Bundle
	registryPod 	  *internalregistry.RegistryPod
	objects			  []unstructured.Unstructured // Resources to create
	client 			  olmclient.Client
	// Other required fields
}

func (c *BundleCmd) newManager() (m *bundleManager, err error) {
	// Check and parse flag values, other init stuff
	...
	m.registryPod, err = internalregistry.NewRegistryPod(...)
	if err != nil {
		return nil, err
	}
	...
}

func (m *bundleManager) run(ctx context.Context) (err error) {
	// Create registry pod and other resources
	...
	registryGRPCAddr := internalregistry.GetRegistryServiceAddr(
		registryPod.GetPodName(),
		m.resourceNamespace)
	catsrc := newCatalogSource(registryPod.GetPodName(),
		m.operatorNamespace,
		withGRPC(registryGRPCAddr))
	m.objects = append(m.objects, catsrc)

	...
	return m.client.DoCreate(m.objects...)
}

@joelanford
Copy link
Copy Markdown
Member

@estroz @rashmigottipati I was hoping we could refactor and reuse even more of what already exists for run packagemanifests.

Will there be any difference whatsoever between run packagemanifests and run bundle for anything after the creation of the pod that runs the ephemeral index?

I'm wondering if the CLIs for run bundle and run packagemanifest can even reuse the same struct with an interface field for creating the index pod. Then run packagemanifest can plug in its implementation and run bundle can plug in its. All other setup would be the same right?

@estroz
Copy link
Copy Markdown
Member

estroz commented Jul 29, 2020

@joelanford other than pod creation and some catalogsource/subscription data, they’re effectively the same. The main difference is having a slice of bundles and a package manifest vs one bundle and metadata, so just the underlying implementations of the interface you suggest.

@joelanford
Copy link
Copy Markdown
Member

@estroz What would be different in the catalog source and subscription between the two?

In both cases, aren't we pointing the catalog source to a GRPC port on a pod and creating a subscription with the package name, and catalog source name/namespace?

@rashmigottipati
Copy link
Copy Markdown
Member Author

@joelanford

@estroz What would be different in the catalog source and subscription between the two?

In both cases, aren't we pointing the catalog source to a GRPC port on a pod and creating a subscription with the package name, and catalog source name/namespace?

I think that's right. Creation of catalog source is same for both run bundle and run packagemanifest.
For creating a subscription, run packagemanifest sets the catalog name and namespace, target package, channel, and starting CSV. It uses the CSV name to set the subscription name.

@rashmigottipati
Copy link
Copy Markdown
Member Author

rashmigottipati commented Jul 29, 2020

@joelanford @estroz

Thanks for the inputs. Totally agree with refactoring and reusing more of the existing logic in packagemanifests. Since jira tasks for run bundle have create grpc catalog source and create subscription logic, I was treating them as individual building blocks first, so started writing separate logic and adding unit tests for these smaller portions with the intention to refactor later to suit the packagemanifests style. However, as I'm working on subscription logic, it occurred to me that all of these custom resources (i.e. catalog source and subscription) could be created at once (similar to run method in packagemanifests) after initializing the registry pod.

Thinking about what would be the best approach to take:
If we decide to reuse packagemanifests code by using the same struct with an interface field for creating the index pod, it's probably better to do it by breaking down into smaller PRs.
For the purpose of this PR, would it be reasonable to do something like what exists for packageManifestsManager, which will construct a CatalogSource in its run() method as @estroz suggested, and in the upcoming PRs, refactor and reuse the existing packagemanifests logic by adding an interface field to the same struct.

Any suggestions?

@rashmigottipati rashmigottipati force-pushed the create-catalog-source branch 4 times, most recently from 1cf4504 to 3f76aa3 Compare August 17, 2020 14:19
@rashmigottipati rashmigottipati requested review from camilamacedo86, estroz and joelanford and removed request for shawn-hurley and theishshah August 17, 2020 14:20
@rashmigottipati
Copy link
Copy Markdown
Member Author

Added functionality to create the catalog source based on new design, @joelanford @estroz @jmrodri please take a look

@rashmigottipati rashmigottipati force-pushed the create-catalog-source branch 3 times, most recently from 2b1a428 to 68e4e22 Compare August 17, 2020 20:37
@rashmigottipati rashmigottipati changed the title [run bundle] Create GRPC Catalog Source [run bundle] Implement IndexImageCatalogCreator - create grpc catalog source Aug 18, 2020
Copy link
Copy Markdown
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2020
@jmrodri jmrodri dismissed estroz’s stale review August 18, 2020 16:39

The changes in this PR supercede the comments from Eric's review. Dismissing this review to keep this moving.

@jmrodri jmrodri merged commit a87bfd1 into operator-framework:master Aug 18, 2020
@jmrodri
Copy link
Copy Markdown
Member

jmrodri commented Aug 18, 2020

Also, we're postponing adding tests for next sprint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants