Create registry pods for ConfigMap CatalogSources #556

ecordell · 2018-11-08T00:12:58Z

This is step 2 in a long line of changes to switch to using operator-registry in OLM.

Add pod reconciler that creates registry pods from configmaps
Add ConfigMap sourcetype as an alias for internal
Write tests

Note that this doesn't yet use the pods for resolving dependencies, so the in memory registry is staying for now.

alecmerdler

Just a few thoughts about code design, but I'm sure it works.

alecmerdler · 2018-11-12T17:33:03Z

pkg/controller/operators/catalog/operator.go

@@ -7,12 +7,17 @@ import (
 	"sync"
 	"time"

+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister"


Fix import order.

alecmerdler · 2018-11-12T17:34:37Z

pkg/controller/registry/reconciler.go

+var timeNow = func() metav1.Time { return metav1.NewTime(time.Now().UTC()) }
+
+// catalogSourceDecorator wraps CatalogSource to add additional methods
+type catalogSourceDecorator struct {


Why not just use pure functions instead of embedding a struct?

It's mostly a style issue; I personally think writing compositions of objects in go ends up more readable than writing pure functions.

That said, all of the methods I defined on the decorator require state from the CatalogSource. Wrapping it in a (private) struct is equivalent to defining a set of functions that all take the catalog source as a first argument.

The way I'm using the struct here is way more like you'd use a State Monad to thread common state throughout pure function calls.

alecmerdler · 2018-11-12T17:35:51Z

pkg/controller/registry/reconciler.go

+	return rb
+}
+
+type RegistryReconciler interface {


Can we potentially not introduce another class that ends in -er?

In general I think naming doesn't matter nearly as much as interface shapes, and names should just be whatever will help a reader the most. So if there's a name that would help you read it better, I would be happy to change it here 😁

njhale · 2018-11-12T18:24:44Z

pkg/controller/operators/catalog/operator.go

 	return op, nil
 }

+func (o *Operator) syncObject(obj interface{}) (syncError error) {


pkg/controller/registry/reconciler.go

njhale · 2018-11-12T19:11:57Z

pkg/lib/operatorlister/service.go

@@ -111,6 +111,6 @@ func (n *NullServiceNamespaceLister) Get(name string) (*v1.Service, error) {
 }

 // GetPodServices returns nil and an error explaining that this is a NullServiceNamespaceLister.
-func (n *NullServiceAccountNamespaceLister) GetPodServices(pod *v1.Pod) ([]*v1.Service, error) {
+func (n *NullServiceNamespaceLister) GetPodServices(pod *v1.Pod) ([]*v1.Service, error) {


njhale · 2018-12-11T15:24:31Z

pkg/controller/registry/reconciler.go

+	if err := c.ensureRoleBinding(source, overwrite); err != nil {
+		return err
+	}
+	if err := c.ensurePod(source, overwritePod); err != nil {


We could make use of a Deployment instead of a Pod. Then we would get rolling updates and a safeguard against losing a working registry pod (if the CatalogSource's ConfigMap is no longer valid).

ecordell · 2018-12-12T20:22:29Z

/test e2e-aws-olm

njhale · 2018-12-12T20:22:35Z

cmd/catalog/main.go

@@ -19,6 +19,7 @@ import (
 const (
 	defaultWakeupInterval   = 15 * time.Minute
 	defaultCatalogNamespace = "tectonic-system"


tectonic-system?

ecordell · 2018-12-15T22:05:38Z

/retest

ecordell · 2018-12-16T14:37:39Z

/retest

ecordell · 2018-12-16T17:16:20Z

/retest

ecordell · 2018-12-16T17:27:22Z

/retest

ecordell · 2018-12-16T23:56:37Z

/retest

ecordell · 2018-12-17T13:13:26Z

/retest

ecordell · 2018-12-17T15:53:01Z

/retest

ecordell · 2018-12-17T18:49:15Z

/retest

njhale

/lgtm

ecordell · 2018-12-18T04:43:32Z

/retest

ecordell · 2018-12-18T14:34:48Z

/test e2e-aws-olm

njhale

/lgtm

njhale · 2018-12-18T16:29:12Z

/approve

openshift-ci-robot · 2018-12-18T16:29:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [njhale]

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

Create registry pods for ConfigMap CatalogSources

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2018

openshift-ci-robot requested review from jpeeler and njhale November 8, 2018 00:13

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 8, 2018

ecordell force-pushed the registry-pods branch from eba191a to 08457e6 Compare November 8, 2018 12:04

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 8, 2018

ecordell force-pushed the registry-pods branch 2 times, most recently from e2aa9e5 to 6bebbf1 Compare November 8, 2018 18:03

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 8, 2018

ecordell force-pushed the registry-pods branch from 6bebbf1 to 927888b Compare November 8, 2018 18:47

ecordell changed the title ~~[WIP] Create registry pods for configmaps~~ Create registry pods for configmaps Nov 8, 2018

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 Nov 8, 2018

ecordell changed the title ~~Create registry pods for configmaps~~ Create registry pods for ConfigMap CatalogSources Nov 8, 2018

ecordell force-pushed the registry-pods branch 3 times, most recently from 9940b92 to 68132c8 Compare November 12, 2018 13:45

alecmerdler reviewed Nov 12, 2018

View reviewed changes

njhale reviewed Nov 12, 2018

View reviewed changes

pkg/controller/operators/catalog/operator.go

return op, nil

}

func (o *Operator) syncObject(obj interface{}) (syncError error) {

Copy link

Member

njhale Nov 12, 2018

Choose a reason for hiding this comment

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

👍

njhale reviewed Nov 12, 2018

View reviewed changes

pkg/controller/registry/reconciler.go Show resolved Hide resolved

njhale reviewed Nov 12, 2018

View reviewed changes

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

ecordell force-pushed the registry-pods branch from 68132c8 to 6014135 Compare December 7, 2018 13:46

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

njhale reviewed Dec 11, 2018

View reviewed changes

ecordell force-pushed the registry-pods branch 2 times, most recently from 056e33e to de4f82f Compare December 12, 2018 19:49

njhale reviewed Dec 12, 2018

View reviewed changes

ecordell force-pushed the registry-pods branch from 738d607 to 800120d Compare December 15, 2018 18:08

ecordell force-pushed the registry-pods branch from 800120d to ee50f5e Compare December 17, 2018 17:07

ecordell force-pushed the registry-pods branch 3 times, most recently from 7132821 to 69cbdfe Compare December 18, 2018 01:03

openshift-ci-robot assigned njhale Dec 18, 2018

njhale approved these changes Dec 18, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2018

chore(deps): vendor

1d71139

ecordell force-pushed the registry-pods branch from 69cbdfe to 1d71139 Compare December 18, 2018 02:54

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2018

njhale approved these changes Dec 18, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2018

openshift-merge-robot merged commit 0c9e050 into operator-framework:master Dec 18, 2018

ecordell deleted the registry-pods branch December 18, 2018 18:29

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

Merge pull request operator-framework#556 from ecordell/registry-pods

9bc496a

Create registry pods for ConfigMap CatalogSources

njhale added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create registry pods for ConfigMap CatalogSources #556

Create registry pods for ConfigMap CatalogSources #556

ecordell commented Nov 8, 2018 •

edited by njhale

Loading

alecmerdler left a comment

alecmerdler Nov 12, 2018

alecmerdler Nov 12, 2018

ecordell Nov 12, 2018

alecmerdler Nov 12, 2018

ecordell Nov 12, 2018

njhale Nov 12, 2018

njhale Nov 12, 2018

njhale Dec 11, 2018

ecordell commented Dec 12, 2018

njhale Dec 12, 2018

ecordell commented Dec 15, 2018

ecordell commented Dec 16, 2018

ecordell commented Dec 16, 2018

ecordell commented Dec 16, 2018

ecordell commented Dec 16, 2018

ecordell commented Dec 17, 2018

ecordell commented Dec 17, 2018

ecordell commented Dec 17, 2018

njhale left a comment

ecordell commented Dec 18, 2018

ecordell commented Dec 18, 2018

njhale left a comment

njhale commented Dec 18, 2018

openshift-ci-robot commented Dec 18, 2018

Create registry pods for ConfigMap CatalogSources #556

Create registry pods for ConfigMap CatalogSources #556

Conversation

ecordell commented Nov 8, 2018 • edited by njhale Loading

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 commented Dec 12, 2018

Choose a reason for hiding this comment

ecordell commented Dec 15, 2018

ecordell commented Dec 16, 2018

ecordell commented Dec 16, 2018

ecordell commented Dec 16, 2018

ecordell commented Dec 16, 2018

ecordell commented Dec 17, 2018

ecordell commented Dec 17, 2018

ecordell commented Dec 17, 2018

njhale left a comment

Choose a reason for hiding this comment

ecordell commented Dec 18, 2018

ecordell commented Dec 18, 2018

njhale left a comment

Choose a reason for hiding this comment

njhale commented Dec 18, 2018

openshift-ci-robot commented Dec 18, 2018

ecordell commented Nov 8, 2018 •

edited by njhale

Loading