Bug 1779990: fix(packageserver): cache packagemanifests #1150

njhale · 2019-11-23T04:40:27Z

Description of the change:

Generate and maintain a running cache of PackageManifests. Serve get/list requests directly from this cache.

Motivation for the change:

Packageserver queries catalog gRPC APIs and constructs PackageManifest resources, on the fly, per request. When dealing with large catalogs containing icon images, this can cause requests to be very slow. Poor response times from packageserver can impact the OperatorHub UI's load time and cause its tests to timeout.

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

njhale · 2019-11-23T05:04:54Z

Playing around with this on minikube w/ community-operators (87 packages total) reveals some pretty promising results running kubectl get packagemanifests --all-namespaces

Before:
GET /apis/packages.operators.coreos.com/v1/packagemanifests?limit=500: (3.84524649s) 200 [kubectl/v1.16.3 (darwin/amd64) kubernetes/b3cbbae 192.168.64.13:49110]

After:
GET /apis/packages.operators.coreos.com/v1/packagemanifests?limit=500: (3.679124ms) 200 [kubectl/v1.16.3 (darwin/amd64) kubernetes/b3cbbae 192.168.64.13:44272]

~1000x faster 😅

The OperatorHub UI loads almost instantly too.

njhale · 2019-11-26T03:16:28Z

/retest

njhale · 2019-11-27T17:09:36Z

/retest

exdx · 2019-11-27T18:34:56Z

/retry

pkg/package-server/provider/registry.go

ecordell · 2019-11-27T21:48:32Z

pkg/package-server/provider/registry.go

@@ -165,9 +200,107 @@ func (p *RegistryProvider) syncCatalogSource(obj interface{}) (syncError error)
 	p.setClient(newRegistryClient(source, conn), key)
 	logger.Info("new grpc connection added")

+	syncError = p.refreshCache(context.Background(), key)


This is my only real design comment - we don't need to rebuild the cache on every sync. We should only need to do it when the catalogsource changes, which should be indicated by the lastupdatetime, or when the grpc connection dies, etc.

njhale · 2019-12-05T07:06:39Z

cmd/package-server/main.go

-const (
-	defaultWakeupInterval = 5 * time.Minute
-)
-
-// config flags defined globally so that they appear on the test binary as well
-var (
-	ctx  = signals.Context()
-	options = server.NewPackageServerOptions(os.Stdout, os.Stderr)
-	cmd     = &cobra.Command{
-		Short: "Launch a package-server",
-		Long:  "Launch a package-server",
-		RunE: func(c *cobra.Command, args []string) error {
-			if err := options.Run(ctx); err != nil {
-				return err
-			}
-			return nil
-		},
-	}
-)
-
-func init() {
-	flags := cmd.Flags()
-
-	flags.DurationVar(&options.WakeupInterval, "interval", options.WakeupInterval, "Interval at which to re-sync CatalogSources")
-	flags.StringVar(&options.GlobalNamespace, "global-namespace", options.GlobalNamespace, "Name of the namespace where the global CatalogSources are located")
-	flags.StringSliceVar(&options.WatchedNamespaces, "watched-namespaces", options.WatchedNamespaces, "List of namespaces the package-server will watch watch for CatalogSources")
-	flags.StringVar(&options.Kubeconfig, "kubeconfig", options.Kubeconfig, "The path to the kubeconfig used to connect to the Kubernetes API server and the Kubelets (defaults to in-cluster config)")
-	flags.BoolVar(&options.Debug, "debug", options.Debug, "use debug log level")
-
-	options.SecureServing.AddFlags(flags)
-	options.Authentication.AddFlags(flags)
-	options.Authorization.AddFlags(flags)
-	options.Features.AddFlags(flags)
-
-	flags.AddGoFlagSet(flag.CommandLine)
-	flags.Parse(flag.Args())
-}
-


Decided to clean all this up while I'm here.

njhale · 2019-12-05T07:11:29Z

go.mod

@@ -43,6 +43,7 @@ require (
 	k8s.io/client-go v8.0.0+incompatible
 	k8s.io/code-generator v0.0.0
 	k8s.io/component-base v0.0.0
+	k8s.io/klog v0.4.0


OLM codepaths using glog were brought into the packageserver dependency tree transitively. This was causing flag registration conflicts with klog, so I swapped all references from glog to klog.

njhale · 2019-12-05T07:20:05Z

pkg/package-server/provider/registry.go

 	globalNamespace string
-	clients         map[sourceKey]registryClient
+	sources         *registrygrpc.SourceStore


I replaced the local gRPC connection handling with @ecordell's SourceStore. It has a cleaner interface, and now with its use here, consolidates gRPC connection management logic across OLM.

njhale · 2019-12-05T07:26:37Z

pkg/package-server/provider/registry.go

 	}
-
-	for _, namespace := range watchedNamespaces {


I got rid of multi-namespace configuration, since other OLM controllers already assume cluster-wide privileges. I also did this to avoid needing to register/access different CatalogSource listers for each namespace.

njhale · 2019-12-05T07:30:07Z

pkg/package-server/provider/registry.go

 	return
 }

+func (p *RegistryProvider) refreshCache(ctx context.Context, client *registryClient) error {


Decided to pass this a *registryClient instead of a key to make testing easier, since we have pre-existing tests that use a fake of this type to directly inject registry responses.

njhale · 2019-12-05T07:32:47Z

pkg/package-server/provider/registry_test.go

@@ -55,7 +56,7 @@ func server() {
 		logrus.Fatal(err)
 	}

-	loader := sqlite.NewSQLLoaderForDirectory(load, "manifests")
+	loader := sqlite.NewSQLLoaderForDirectory(load, filepath.Join("testdata", "manifests"))


We've been trying to use the testdata convention throughout operator-framework, so I updated it while I was here. I can remove it from the PR, but I don't think it's too risky to leave it.

njhale · 2019-12-05T07:38:07Z

pkg/package-server/provider/registry.go

 }

-func toPackageManifest(logger *logrus.Entry, pkg *api.Package, client registryClient) (*operators.PackageManifest, error) {
+func newPackageManifest(ctx context.Context, logger *logrus.Entry, pkg *api.Package, client *registryClient) (*operators.PackageManifest, error) {


Plumbed down the context so that cache building can enforce a timeout on packagemanifest generation.

njhale · 2019-12-05T08:18:20Z

test/e2e/setup_test.go

@@ -39,9 +39,6 @@ var (
 )

 func TestMain(m *testing.M) {
-	if err := flag.Set("logtostderr", "true"); err != nil {


In klog this is true by default.

openshift-ci-robot · 2019-12-05T08:19:50Z

@njhale: This pull request references Bugzilla bug 1779990, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1779990: fix(packageserver): cache packagemanifests

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.

njhale · 2019-12-05T08:22:03Z

/cherry-pick release-4.3

openshift-cherrypick-robot · 2019-12-05T08:22:04Z

@njhale: once the present PR merges, I will cherry-pick it on top of release-4.3 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.3

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.

njhale · 2019-12-05T13:02:59Z

/retest

Flakes look related to OAuth.

njhale · 2019-12-05T17:31:23Z

/retest

jpeeler · 2019-12-05T19:29:49Z

/retest

One of the flakes this time was this marketplace one https://bugzilla.redhat.com/show_bug.cgi?id=1780143.

jpeeler

This is some quality code. There are a lot of nice improvements here.

jpeeler · 2019-12-05T21:37:36Z

pkg/package-server/provider/registry.go

 		}
+	case connectivity.TransientFailure, connectivity.Shutdown:


I wonder if having transient failure here actually adds additional work. Did you find that gc-ing a source and then re-adding was quicker than letting a source transition back to connecting and (hopefully) ready?

We discussed offline that having something in the cache in this state is probably not the best, so we'll just leave it as is.

jpeeler · 2019-12-05T21:42:07Z

pkg/package-server/provider/registry.go

+			break
+		}
+
+		g.Go(func() (pkgErr error) {


njhale · 2019-12-05T22:39:35Z

/retest

exdx · 2019-12-05T22:52:39Z

pkg/package-server/provider/registry.go

+			pkg, err := client.GetPackage(ctx, &api.GetPackageRequest{Name: pkgName.GetName()})
+			if err != nil {
+				logger.WithField("err", err.Error()).Warnf("eliding package: error getting package")
+				return


In the anonymous function you defined a named-error return type pkgErr but then don't set pkgErr equal to the returned error before returning - does this function return the intended error or nil? Might be simpler to just return the error?

Ah, I found I didn't want to return an error anywhere since this should be best effort, and didn't want to write return nil 4 times. l like the way errorgroups manage goroutines, but it may not be the best abstraction to use here. Any suggestions, using a waitgroup leaves something to be desired 🙁

Oh I see what you mean...You're saying based off the *Group.Go() signature you need the anonymous function to to return an error (https://godoc.org/golang.org/x/sync/errgroup#Group.Go) but you don't actually want *Group.Wait() to return a non-nil error if one does come up. Since the derived Context is canceled the first time a function passed to Go returns a non-nil error and this is best effort we don't want that.

Maybe just have _ := g.Wait() instead and comment that the error is being dropped intentionally as this is best effort to make it more clear for a reader? I see what you mean about the abstraction though

@exdx So I took a look back at error group and found that some assumptions I made around how it handles context cancellation were wrong. I'll switch this to using WaitGroup instead and then we won't be returning an error from the anonymous function anyway. Should have this change up soon.

njhale · 2019-12-06T01:02:56Z

/retest

jpeeler · 2019-12-06T16:16:59Z

/retest
/lgtm

I did see one comment that might be worth addressing in a follow up. But let's try and get this in today.

jpeeler · 2019-12-06T19:19:22Z

/retest

njhale · 2019-12-06T21:46:38Z

/retest

njhale · 2019-12-07T07:46:25Z

/retest

Bowenislandsong · 2019-12-09T14:30:02Z

/lgtm

openshift-ci-robot · 2019-12-09T14:30:49Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

ecordell · 2019-12-09T14:24:35Z

pkg/package-server/provider/registry.go

+	}
+	if sourceMeta := p.sources.GetMeta(key); sourceMeta != nil && sourceMeta.Address == address {
+		// If the address hasn't changed, don't bother creating a new source
+		logger.Debug("catalog address unchanged, skipping source creation")


Can’t the pod be updated with new content without the address changing?

I'm assuming that, at minimum, the gRPC connection changes state when catalog content is updated. That's probably a bad assumption in general, but I believe it's the behavior of all current registry implementations.

ecordell · 2019-12-09T14:33:05Z

pkg/package-server/provider/registry.go

+
+	var err error
+	switch state.State {
+	case connectivity.Ready:


It’s unlikely, but there are cases where you might not receive an event for this (for example, if the connection is already in the ready state before it’s events are watched, or if the states change too rapidly). In practice this doesn’t seem to be an issue for grpc connections, but I wanted to mention it.

(This is due to relying on the grpc wait for state change, which requires you specify the state you’re waiting to change away from)

It’s not an issue in catalog-operator because the state change is used as an optimization and requeues a catalogsource for processing - if an event is missed like that, a periodic resync will fix it.

Fair -- We can make caching more aggressive in the sync function to compensate.

njhale · 2019-12-09T15:51:34Z

/retest

openshift-ci-robot · 2019-12-09T17:21:12Z

@njhale: All pull requests linked via external trackers have merged. Bugzilla bug 1779990 has been moved to the MODIFIED state.

In response to this:

Bug 1779990: fix(packageserver): cache packagemanifests

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.

openshift-cherrypick-robot · 2019-12-09T17:21:19Z

@njhale: new pull request created: #1181

In response to this:

/cherry-pick release-4.3

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.

njhale · 2020-01-20T16:53:03Z

/cherry-pick release-4.2

openshift-cherrypick-robot · 2020-01-20T16:53:12Z

@njhale: #1150 failed to apply on top of branch "release-4.2":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
M	pkg/package-server/provider/registry_test.go
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
Auto-merging pkg/package-server/provider/registry_test.go
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Patch failed at 0001 fix(packageserver): cache packagemanifests

In response to this:

/cherry-pick release-4.2

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.

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 23, 2019

openshift-ci-robot requested review from ecordell and jpeeler November 23, 2019 04:40

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 23, 2019

njhale force-pushed the spur-pkgsvr branch 3 times, most recently from f4c1362 to fbc3413 Compare November 25, 2019 14:31

njhale changed the title ~~WIP: fix(packageserver): cache packagemanifests~~ fix(packageserver): cache packagemanifests Nov 25, 2019

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 25, 2019

exdx mentioned this pull request Nov 25, 2019

Bug 1760608: remove resource limits from packageserver #1149

Closed

5 tasks

ecordell reviewed Nov 27, 2019

View reviewed changes

njhale force-pushed the spur-pkgsvr branch from fbc3413 to cefc2c8 Compare December 5, 2019 07:02

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 5, 2019

njhale commented Dec 5, 2019

View reviewed changes

njhale force-pushed the spur-pkgsvr branch from cefc2c8 to ca9fa38 Compare December 5, 2019 08:15

njhale commented Dec 5, 2019

View reviewed changes

njhale changed the title ~~fix(packageserver): cache packagemanifests~~ Bug 1779990: fix(packageserver): cache packagemanifests Dec 5, 2019

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Dec 5, 2019

jpeeler reviewed Dec 5, 2019

View reviewed changes

exdx reviewed Dec 5, 2019

View reviewed changes

openshift-ci-robot assigned jpeeler Dec 6, 2019

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

njhale added 3 commits December 6, 2019 14:33

fix(packageserver): cache packagemanifests

5723332

chore(modules): vendor deps

495ca9b

chore(git): remove and ignore packageserver test key pair

f618585

njhale force-pushed the spur-pkgsvr branch from ca9fa38 to f618585 Compare December 6, 2019 19:34

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

openshift-ci-robot assigned Bowenislandsong Dec 9, 2019

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

ecordell reviewed Dec 9, 2019

View reviewed changes

openshift-merge-robot merged commit 0e18a81 into operator-framework:master Dec 9, 2019

openshift-cherrypick-robot mentioned this pull request Dec 9, 2019

[release-4.3] Bug 1779995: fix(packageserver): cache packagemanifests #1181

Merged

njhale deleted the spur-pkgsvr branch March 31, 2020 21:31

Bug 1779990: fix(packageserver): cache packagemanifests #1150

Bug 1779990: fix(packageserver): cache packagemanifests #1150

Conversation

njhale commented Nov 23, 2019 • edited Loading

njhale commented Nov 23, 2019 • edited Loading

njhale commented Nov 26, 2019

njhale commented Nov 27, 2019

exdx commented Nov 27, 2019

ecordell Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale Dec 5, 2019 • 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

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 5, 2019

njhale commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

njhale commented Dec 5, 2019

njhale commented Dec 5, 2019

jpeeler commented Dec 5, 2019

jpeeler 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

njhale commented Dec 5, 2019

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 commented Dec 6, 2019

jpeeler commented Dec 6, 2019

jpeeler commented Dec 6, 2019

njhale commented Dec 6, 2019

njhale commented Dec 7, 2019

Bowenislandsong commented Dec 9, 2019

openshift-ci-robot commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

njhale commented Dec 9, 2019

openshift-ci-robot commented Dec 9, 2019

openshift-cherrypick-robot commented Dec 9, 2019

njhale commented Jan 20, 2020

openshift-cherrypick-robot commented Jan 20, 2020

njhale commented Nov 23, 2019 •

edited

Loading

njhale commented Nov 23, 2019 •

edited

Loading

ecordell Nov 27, 2019 •

edited

Loading

njhale Dec 5, 2019 •

edited

Loading

njhale Dec 9, 2019 •

edited

Loading