Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consume Operator Registry from PackageManifest Server #650

Merged
merged 2 commits into from
Jan 11, 2019
Merged

Consume Operator Registry from PackageManifest Server #650

merged 2 commits into from
Jan 11, 2019

Conversation

njhale
Copy link
Member

@njhale njhale commented Jan 8, 2019

Description

Continuation of 639 by @alecmerdler

Replace the in-memory, ConfigMap implementation with the gRPC interface provided by the operator-registry server.

Addresses https://jira.coreos.com/browse/ALM-831

@openshift-ci-robot 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 8, 2019
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 8, 2019
njhale
njhale commented Jan 8, 2019
View reviewed changes
pkg/package-server/provider/manifests/etcd/0.6.1/etcdcluster.crd.yaml
@@ -0,0 +1,16 @@
apiVersion: apiextensions.k8s.io/v1beta1
Copy link
Member Author

Choose a reason for hiding this comment

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

Added these for use with registry-server in unit tests.

@njhale njhale force-pushed the packageserver-use-registry branch 2 times, most recently from 1410c3c to 2ac7a95 Compare January 8, 2019 13:04
@njhale njhale mentioned this pull request Jan 8, 2019
Closed
ecordell
ecordell reviewed Jan 8, 2019
View reviewed changes
pkg/package-server/provider/registry.go Outdated Show resolved Hide resolved
pkg/package-server/provider/registry.go Outdated Show resolved Hide resolved
pkg/lib/forwarder/fanout.go Outdated Show resolved Hide resolved
pkg/package-server/provider/registry.go Outdated Show resolved Hide resolved
pkg/package-server/provider/registry.go Outdated
if client.source.GetNamespace() == namespace || client.source.GetNamespace() == p.globalNamespace || namespace == "" {
logger.Debugf("found CatalogSource %s", client.source.GetName())
// Start up forwarders.
p.forward(ctx, logger, add, pkgTransform(addPkg))
Copy link
Member

Choose a reason for hiding this comment

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

super nit: pkgTransform to me suggests that this Transform will mutate the events. I'd prefer a naming that suggested that it's just a filter, e.g. addEventFilter := pkgTransform(addPkg)

pkg/package-server/server/server.go Outdated Show resolved Hide resolved
@njhale njhale force-pushed the packageserver-use-registry branch 3 times, most recently from 76bbdd7 to 4e1df36 Compare January 9, 2019 06:54
njhale
njhale commented Jan 9, 2019
View reviewed changes
pkg/package-server/provider/registry.go Outdated
defer cancel()

// Signal that the subscriber's channel is being written to and is not safe to close.
info.wg.Add(1)
Copy link
Member Author

Choose a reason for hiding this comment

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

Some additional protection against subscriptions ending.

@njhale njhale force-pushed the packageserver-use-registry branch 2 times, most recently from 65bc7c0 to 383751d Compare January 9, 2019 12:47
ecordell
ecordell reviewed Jan 9, 2019
View reviewed changes
pkg/package-server/provider/registry.go Outdated Show resolved Hide resolved
pkg/package-server/provider/registry.go Outdated
smu sync.RWMutex
cmu sync.RWMutex
globalNamespace string
clients []registryClient
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be a map from a sourcekey (name, namespace) to a registryclient?

pkg/package-server/provider/registry.go Outdated Show resolved Hide resolved
@njhale njhale force-pushed the packageserver-use-registry branch from 383751d to a26f157 Compare January 9, 2019 19:29
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2019
@njhale njhale force-pushed the packageserver-use-registry branch 3 times, most recently from 6bf1e0e to 3922fc6 Compare January 10, 2019 03:48
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2019
@njhale njhale force-pushed the packageserver-use-registry branch 6 times, most recently from 07f45d6 to 7c004cf Compare January 10, 2019 16:01
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2019
alecmerdler
alecmerdler approved these changes Jan 10, 2019
View reviewed changes
Copy link
Member

@alecmerdler alecmerdler left a comment

Choose a reason for hiding this comment

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

/lgtm

@njhale
Copy link
Member Author

njhale commented Jan 10, 2019

/retest

1 similar comment
@njhale
Copy link
Member Author

njhale commented Jan 10, 2019

/retest

@njhale njhale force-pushed the packageserver-use-registry branch from b26b3f8 to 7e84cc6 Compare January 10, 2019 22:04
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2019
ecordell
ecordell approved these changes Jan 10, 2019
View reviewed changes
Copy link
Member

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

/lgtm

@ecordell
Copy link
Member

/retest

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2019
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@njhale
Copy link
Member Author

njhale commented Jan 10, 2019

/retest

4 similar comments
@ecordell
Copy link
Member

/retest

@ecordell
Copy link
Member

/retest

@ecordell
Copy link
Member

/retest

@ecordell
Copy link
Member

/retest

alecmerdler and others added 2 commits January 11, 2019 09:50
@alecmerdler @njhale
feat(packageserver): consume registry pods
4e6d85c 
- Consumes from registry pods
- Removes in-mem Provider implementation
- Removes watch capabilities
- Fixes packageserver namespace watching
@njhale
test(unit): fix TestAPIMultiOwnerSet_PopAPIKey tests
196cc1c
@njhale njhale force-pushed the packageserver-use-registry branch from 7e84cc6 to 196cc1c Compare January 11, 2019 15:12
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2019
@openshift-ci-robot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@njhale
Copy link
Member Author

njhale commented Jan 11, 2019

/retest

3 similar comments
@njhale
Copy link
Member Author

njhale commented Jan 11, 2019

/retest

@ecordell
Copy link
Member

/retest

@njhale
Copy link
Member Author

njhale commented Jan 11, 2019

/retest

@ecordell ecordell merged commit 4170dc9 into operator-framework:master Jan 11, 2019
@openshift-ci-robot
Copy link
Collaborator

@njhale: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-olm 196cc1c link /test e2e-aws-olm
ci/prow/e2e-aws 196cc1c link /test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ecordell added a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019
@ecordell
Merge pull request operator-framework#650 from njhale/packageserver-u…
c2e0511 
…se-registry

Consume Operator Registry from PackageManifest Server
@njhale njhale added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 19, 2019
@njhale njhale deleted the packageserver-use-registry branch September 30, 2019 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecmerdler alecmerdler alecmerdler approved these changes

@ecordell ecordell ecordell approved these changes

@jpeeler jpeeler Awaiting requested review from jpeeler

Assignees

@ecordell ecordell

@alecmerdler alecmerdler

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@njhale @ecordell @openshift-ci-robot @alecmerdler