From 021ba4bf01d2b70e106a8664d865786423f0298e Mon Sep 17 00:00:00 2001 From: Ben Luddy Date: Tue, 31 Aug 2021 16:38:12 -0400 Subject: [PATCH] Mark GetBundleForChannel as deprecated and trim its response. The RPC api.Registry/GetBundleForChannel is consumed only by package-server and by declarative index conversion. Neither reads any field other than CsvName and CsvJson. The package-server generates one call to this RPC per channel per package per catalog and is responsible for needlessly large CPU utilization and resident set sizes on registry pods. Removing the additional per-request processing and database queries that populate unused fields makes the registry server better able to absorb the bursty load from package-server. Signed-off-by: Ben Luddy --- pkg/api/registry.proto | 4 ++- pkg/registry/interface.go | 5 ++- pkg/registry/populator_test.go | 6 +++- pkg/server/server_test.go | 8 ++++- pkg/sqlite/configmap_test.go | 6 +++- pkg/sqlite/directory_test.go | 6 +++- pkg/sqlite/query.go | 64 +++++++++------------------------- 7 files changed, 45 insertions(+), 54 deletions(-) diff --git a/pkg/api/registry.proto b/pkg/api/registry.proto index ac7e3b9e1..5f1daa9a7 100644 --- a/pkg/api/registry.proto +++ b/pkg/api/registry.proto @@ -8,7 +8,9 @@ service Registry { rpc ListPackages(ListPackageRequest) returns (stream PackageName) {} rpc GetPackage(GetPackageRequest) returns (Package) {} rpc GetBundle(GetBundleRequest) returns (Bundle) {} - rpc GetBundleForChannel(GetBundleInChannelRequest) returns (Bundle) {} + rpc GetBundleForChannel(GetBundleInChannelRequest) returns (Bundle) { + option deprecated = true; + } rpc GetChannelEntriesThatReplace(GetAllReplacementsRequest) returns (stream ChannelEntry) {} rpc GetBundleThatReplaces(GetReplacementRequest) returns (Bundle) {} rpc GetChannelEntriesThatProvide(GetAllProvidersRequest) returns (stream ChannelEntry) {} diff --git a/pkg/registry/interface.go b/pkg/registry/interface.go index 454cec386..f6a5f843b 100644 --- a/pkg/registry/interface.go +++ b/pkg/registry/interface.go @@ -37,7 +37,10 @@ type GRPCQuery interface { // Get a bundle by its package name, channel name and csv name from the index GetBundle(ctx context.Context, pkgName, channelName, csvName string) (*api.Bundle, error) - // Get the bundle in the specified package at the head of the specified channel + // Get the bundle in the specified package at the head of the + // specified channel. DEPRECATED. Returned bundles may have + // only the "name" and "csvJson" fields populated in order to + // support legacy usage. GetBundleForChannel(ctx context.Context, pkgName string, channelName string) (*api.Bundle, error) // Get all channel entries that say they replace this one diff --git a/pkg/registry/populator_test.go b/pkg/registry/populator_test.go index 442806199..280907964 100644 --- a/pkg/registry/populator_test.go +++ b/pkg/registry/populator_test.go @@ -171,7 +171,11 @@ func TestQuerierForImage(t *testing.T) { {Group: "testapi.coreos.com", Version: "v1", Kind: "testapi"}, }, } - EqualBundles(t, *expectedBundle, *etcdBundleByChannel) + bareGetBundleForChannelResult := &api.Bundle{ + CsvName: expectedBundle.CsvName, + CsvJson: expectedBundle.CsvJson + "\n", + } + EqualBundles(t, *bareGetBundleForChannelResult, *etcdBundleByChannel) etcdBundle, err := store.GetBundle(context.TODO(), "etcd", "alpha", "etcdoperator.v0.9.2") require.NoError(t, err) diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index c257cc1f4..f79725165 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -216,7 +216,13 @@ func testGetBundle(addr string, expected *api.Bundle) func(*testing.T) { } func TestGetBundleForChannel(t *testing.T) { - t.Run("Sqlite", testGetBundleForChannel(dbAddress, etcdoperator_v0_9_2("alpha", false, false))) + { + b := etcdoperator_v0_9_2("alpha", false, false) + t.Run("Sqlite", testGetBundleForChannel(dbAddress, &api.Bundle{ + CsvName: b.CsvName, + CsvJson: b.CsvJson + "\n", + })) + } t.Run("DeclarativeConfig", testGetBundleForChannel(cfgAddress, etcdoperator_v0_9_2("alpha", false, true))) } diff --git a/pkg/sqlite/configmap_test.go b/pkg/sqlite/configmap_test.go index 1d7598336..a40c9fef9 100644 --- a/pkg/sqlite/configmap_test.go +++ b/pkg/sqlite/configmap_test.go @@ -175,7 +175,11 @@ func TestQuerierForConfigmap(t *testing.T) { "{\"apiVersion\":\"apiextensions.k8s.io/v1beta1\",\"kind\":\"CustomResourceDefinition\",\"metadata\":{\"creationTimestamp\":null,\"name\":\"etcdbackups.etcd.database.coreos.com\"},\"spec\":{\"group\":\"etcd.database.coreos.com\",\"names\":{\"kind\":\"EtcdBackup\",\"listKind\":\"EtcdBackupList\",\"plural\":\"etcdbackups\",\"singular\":\"etcdbackup\"},\"scope\":\"Namespaced\",\"version\":\"v1beta2\",\"versions\":[{\"name\":\"v1beta2\",\"served\":true,\"storage\":true}]},\"status\":{\"acceptedNames\":{\"kind\":\"\",\"plural\":\"\"},\"conditions\":null,\"storedVersions\":null}}", "{\"apiVersion\":\"apiextensions.k8s.io/v1beta1\",\"kind\":\"CustomResourceDefinition\",\"metadata\":{\"creationTimestamp\":null,\"name\":\"etcdrestores.etcd.database.coreos.com\"},\"spec\":{\"group\":\"etcd.database.coreos.com\",\"names\":{\"kind\":\"EtcdRestore\",\"listKind\":\"EtcdRestoreList\",\"plural\":\"etcdrestores\",\"singular\":\"etcdrestore\"},\"scope\":\"Namespaced\",\"version\":\"v1beta2\",\"versions\":[{\"name\":\"v1beta2\",\"served\":true,\"storage\":true}]},\"status\":{\"acceptedNames\":{\"kind\":\"\",\"plural\":\"\"},\"conditions\":null,\"storedVersions\":null}}"}, } - EqualBundles(t, *expectedBundle, *etcdBundleByChannel) + bareGetBundleForChannelResult := &api.Bundle{ + CsvName: expectedBundle.CsvName, + CsvJson: expectedBundle.CsvJson + "\n", + } + EqualBundles(t, *bareGetBundleForChannelResult, *etcdBundleByChannel) etcdBundle, err := store.GetBundle(context.TODO(), "etcd", "alpha", "etcdoperator.v0.9.2") require.NoError(t, err) diff --git a/pkg/sqlite/directory_test.go b/pkg/sqlite/directory_test.go index a2a94f9e5..2df0c33a1 100644 --- a/pkg/sqlite/directory_test.go +++ b/pkg/sqlite/directory_test.go @@ -180,7 +180,11 @@ func TestQuerierForDirectory(t *testing.T) { {Group: "etcd.database.coreos.com", Version: "v1beta2", Kind: "EtcdCluster", Plural: "etcdclusters"}, }, } - EqualBundles(t, *expectedBundle, *etcdBundleByChannel) + bareGetBundleForChannelResult := &api.Bundle{ + CsvName: expectedBundle.CsvName, + CsvJson: expectedBundle.CsvJson + "\n", + } + EqualBundles(t, *bareGetBundleForChannelResult, *etcdBundleByChannel) etcdBundle, err := store.GetBundle(context.TODO(), "etcd", "alpha", "etcdoperator.v0.9.2") require.NoError(t, err) diff --git a/pkg/sqlite/query.go b/pkg/sqlite/query.go index a53f0c5c9..432b99811 100644 --- a/pkg/sqlite/query.go +++ b/pkg/sqlite/query.go @@ -287,64 +287,32 @@ func (s *SQLQuerier) GetBundle(ctx context.Context, pkgName, channelName, csvNam return out, nil } -func (s *SQLQuerier) GetBundleForChannel(ctx context.Context, pkgName string, channelName string) (*api.Bundle, error) { - query := `SELECT DISTINCT channel_entry.entry_id, operatorbundle.name, operatorbundle.bundle, operatorbundle.bundlepath, operatorbundle.version, operatorbundle.skiprange FROM channel - INNER JOIN operatorbundle ON channel.head_operatorbundle_name=operatorbundle.name - INNER JOIN channel_entry ON (channel_entry.channel_name = channel.name and channel_entry.package_name=channel.package_name and channel_entry.operatorbundle_name=operatorbundle.name) - WHERE channel.package_name=? AND channel.name=? LIMIT 1` - rows, err := s.db.QueryContext(ctx, query, pkgName, channelName) +func (s *SQLQuerier) GetBundleForChannel(ctx context.Context, pkg string, channel string) (*api.Bundle, error) { + query := ` +SELECT operatorbundle.name, operatorbundle.csv FROM operatorbundle INNER JOIN channel +ON channel.head_operatorbundle_name = operatorbundle.name +WHERE channel.name = :channel AND channel.package_name = :package` + rows, err := s.db.QueryContext(ctx, query, sql.Named("channel", channel), sql.Named("package", pkg)) if err != nil { return nil, err } defer rows.Close() if !rows.Next() { - return nil, fmt.Errorf("no entry found for %s %s", pkgName, channelName) - } - var entryId sql.NullInt64 - var name sql.NullString - var bundle sql.NullString - var bundlePath sql.NullString - var version sql.NullString - var skipRange sql.NullString - if err := rows.Scan(&entryId, &name, &bundle, &bundlePath, &version, &skipRange); err != nil { - return nil, err - } - - out := &api.Bundle{} - if bundle.Valid && bundle.String != "" { - out, err = registry.BundleStringToAPIBundle(bundle.String) - if err != nil { - return nil, err - } - } - out.CsvName = name.String - out.PackageName = pkgName - out.ChannelName = channelName - out.BundlePath = bundlePath.String - out.Version = version.String - out.SkipRange = skipRange.String - - provided, required, err := s.GetApisForEntry(ctx, entryId.Int64) - if err != nil { - return nil, err - } - out.ProvidedApis = provided - out.RequiredApis = required - - dependencies, err := s.GetDependenciesForBundle(ctx, name.String, version.String, bundlePath.String) - if err != nil { - return nil, err + return nil, fmt.Errorf("no entry found for %s %s", pkg, channel) } - out.Dependencies = dependencies - - properties, err := s.GetPropertiesForBundle(ctx, name.String, version.String, bundlePath.String) - if err != nil { + var ( + name sql.NullString + csv sql.NullString + ) + if err := rows.Scan(&name, &csv); err != nil { return nil, err } - out.Properties = properties - return out, nil + return &api.Bundle{ + CsvName: name.String, + CsvJson: csv.String, + }, nil } func (s *SQLQuerier) GetChannelEntriesThatReplace(ctx context.Context, name string) (entries []*registry.ChannelEntry, err error) {