Provide smarter "plugin install/upgrade" for Central Repository Discovery #64

marckhouzam · 2023-02-10T14:58:45Z

What this PR does / why we need it

When the Central Repo feature is enabled, we use an OCI discovery with an SQLite PluginInventory.
Before this PR, installing/upgrading a plugin was done by getting all plugins, and then looping through the list in Go to find the desired one.

This PR uses the SQLite DB to do a targeted search, and only fetch the information for the desired plugin by filtering by name, target, version, os and arch.

There are actually no user-visible changes due to this change: it is just an improvement on the efficiency of the implementation.

Actually, there is one user-visible change: tz plugin install all is no longer supported when the Central Repo feature is enabled. It does not make sense to intall all plugins present in the Central Repo. An error message is shown to the user if the all argument is used (see testing section).

To introduce this filtering functionality in a generic way, two generic filtering concepts are introduced, one for the Discovery (a DiscoveryCriteria) and one for the PluginInventory (a PluginInventoryFilter). To clarify, this is what we use to install/upgrade plugin:

 _______________     _______________     _________________
|               |   |               |   |                 |
| PluginManager |-->| OCI Discovery |-->| PluginInventory |                
|_______________|   |_______________|   |_________________|

So, with this PR:

the PluginManager specifies a new DiscoveryCriteria when creating the OCI Discovery for the Central Repo. The criteria tells the Discovery what plugins should be discovered (by name, target, version, os, arch).
the OCI Discovery converts the DiscoveryCriteria into a similar PluginInventoryFilter, which it passes to the PluginInventory
the PluginInventory (which is an SQLite engine at the moment) uses the PluginInventoryFilter to write specific SQL queries that respect the PluginInventoryFilter.

Describe testing done for PR

# Clear config and build
$ rm ~/.config/tanzu/config*
$ make build
build darwin-amd64 CLI with version: v0.0.1-dev
mkdir -p bin
cp /Users/kmarc/git/tanzu-cli/artifacts/darwin/amd64/cli/core/v0.0.1-dev/tanzu-cli-darwin_amd64 ./bin/tanzu
$ tz plugin clean
✔  successfully cleaned up all plugins

# Enable Central Repo
$ tz config set features.global.central-repository true
$ export TANZU_CLI_PRE_RELEASE_REPO_IMAGE=gcr.io/eminent-nation-87317/test/v1/tanzu-cli/plugins/central:latest

$ tz plugin list
  NAME  DESCRIPTION  TARGET  VERSION  STATUS

$ tz plugin search
  NAME                DESCRIPTION                                                        TARGET           VERSION  STATUS         CONTEXT
  isolated-cluster    Prepopulating images/bundle for internet-restricted environments                    v0.28.0  not installed
  pinniped-auth       Pinniped authentication operations (usually not directly invoked)                   v0.28.0  not installed
  cluster             Kubernetes cluster operations                                      kubernetes       v0.28.0  not installed
  feature             Operate on features and featuregates                               kubernetes       v0.28.0  not installed
  kubernetes-release  Kubernetes release operations                                      kubernetes       v0.28.0  not installed
  management-cluster  Kubernetes management cluster operations                           kubernetes       v0.28.0  not installed
  package             Tanzu package management                                           kubernetes       v0.28.0  not installed
  secret              Tanzu secret management                                            kubernetes       v0.28.0  not installed
  telemetry           configure cluster-wide settings for vmware tanzu telemetry         kubernetes       v0.28.0  not installed
  account             description                                                        mission-control  v0.0.1   not installed
  apply               description                                                        mission-control  v0.0.1   not installed
  audit               description                                                        mission-control  v0.0.1   not installed
  cluster             description                                                        mission-control  v0.0.1   not installed
  clustergroup        description                                                        mission-control  v0.0.1   not installed
  data-protection     description                                                        mission-control  v0.0.1   not installed
  ekscluster          description                                                        mission-control  v0.0.1   not installed
  events              description                                                        mission-control  v0.0.1   not installed
  iam                 description                                                        mission-control  v0.0.1   not installed
  inspection          description                                                        mission-control  v0.0.1   not installed
  integration         description                                                        mission-control  v0.0.1   not installed
  management-cluster  description                                                        mission-control  v0.0.1   not installed
  policy              description                                                        mission-control  v0.0.1   not installed
  workspace           description                                                        mission-control  v0.0.1   not installed

# Install using the new logic
$ tz plugin install management-cluster
✖  unable to uniquely identify plugin 'management-cluster'. Please specify correct Target(kubernetes[k8s]/mission-control[tmc]) of the plugin with `--target` flag

$ tz plugin install management-cluster --target k8s
ℹ  Installing plugin 'management-cluster:v0.28.0' with target 'kubernetes'
✔  successfully installed 'management-cluster' plugin

$ tz plugin install management-cluster
✖  unable to uniquely identify plugin 'management-cluster'. Please specify correct Target(kubernetes[k8s]/mission-control[tmc]) of the plugin with `--target` flag

# This next one shows an aspect of the new filtering logic.  It does a targeted search for version v0.0.1,
# and since there is only the plugin from TMC with that version, it does not require the user to specify
# the target with the --target flag.
# I'm not sure if this is a good thing or a confusing thing, from a user's perspective
$ tz plugin install management-cluster --version v0.0.1
ℹ  Installing plugin 'management-cluster:v0.0.1' with target 'mission-control'
✔  successfully installed 'management-cluster' plugin
$ tz plugin list
  NAME                DESCRIPTION                               TARGET           VERSION  STATUS
  management-cluster  Kubernetes management cluster operations  kubernetes       v0.28.0  installed
  management-cluster  A TMC registered Management cluster       mission-control  v0.0.1   installed
$ tz plugin upgrade management-cluster
✖  unable to uniquely identify plugin 'management-cluster'. Please specify correct Target(kubernetes[k8s]/mission-control[tmc]) of the plugin with `--target` flag
$ tz plugin upgrade management-cluster --target tmc
ℹ  Installing plugin 'management-cluster:v0.0.1' with target 'mission-control'
✔  successfully upgraded plugin 'management-cluster' to version 'v0.0.1'

$ tz plugin install all
✖  the 'all' argument is not longer supported. You must install each plugin individually

Release note

The command `tanzu plugin install` no longer accepts the `all` argument.

Additional information

Unit tests have been added for the modified code. Actually 68% of the code changes are unit tests.
The new unit test in manager_test.go is disabled because it uses a local discovery which is not supported while we use the temporary TANZU_CLI_PRE_RELEASE_REPO_IMAGE variable to specify the discovery. If you disable the use of TANZU_CLI_PRE_RELEASE_REPO_IMAGE and use the list of discoveries in the config file, this new test does pass.
The PR also assumes that the Central Repo has the RecommendedVersion field empty for the moment.
The verifyPluginPreDownload(..) function now verifies the repo using the requested version of the plugin, instead of always using the recommendedVersion. This should cause not change in functionality IIUC. It is required because with a more targeted query of plugins, the recommendedVersion artifact is not always fetched. For example, if installing an older version of the cluster plugin, say v0.26.0, the "smart" install will only fetch information about that version; so, if we were to attempt to verifyPluginPreDownload() using the RecommendedVersion of v0.28.0, that information would not be available, and would need to be fetched separately from the SQLite DB; so, instead, we just use the requested version.

Special notes for your reviewer

As shown in the testing section, doing an efficient plugin discovery has some impacts.
We now differentiate between TMC and TKG plugins also using the version. So, if a user asks to install a specific version of a plugin without specifying the --target but specifying the --version flag, and that only one of the two targets provides that version, we no longer ask the user to specify the target. E.g.,

$ tz plugin install management-cluster --version v0.0.1
ℹ  Installing plugin 'management-cluster:v0.0.1' with target 'mission-control'
✔  successfully installed 'management-cluster' plugin

This is a rare case however, as we are aiming for users not to have to install plugins by versions themselves, but (eventually) use plugin groups.

marckhouzam · 2023-02-10T21:09:00Z

I got some good feedback from @vuil and will rework some aspects of the PR to be more generic and facilitate testing at the same time.
I converted the PR to a draft until that work is done.

marckhouzam · 2023-02-16T19:06:20Z

This PR is now ready for review 😌 . It kind of exploded in size with the unit tests, but the non-test code changes only account for 32% of the PR.

marckhouzam

Some points to consider in-lined in the code.

marckhouzam · 2023-02-16T19:09:00Z

pkg/command/plugin.go

 			}

 			err = pluginmanager.UpgradePlugin(pluginName, pluginVersion, getTarget())
 			if err != nil {
 				return err
 			}
-			log.Successf("successfully upgraded plugin '%s' to version '%s'", pluginName, pluginVersion)


The "version" info is removed from this printout because with the more efficient SQL DB, we don't know which version we are upgrading too here, we just as to install the recommendedVersion, whatever that version is.

This change in printout is not a big deal because it is preceded by another printout mentioning the version:

ℹ Installing plugin 'management-cluster:v0.0.1' with target 'mission-control' ✔ successfully upgraded plugin 'management-cluster'

marckhouzam · 2023-02-16T19:10:11Z

pkg/command/plugin_test.go

+			args:               []string{"plugin", "install", "--target", "invalid", "myplugin"},
+			expectedFailure:    true,
+			expectedErrorMsg:   "invalid target specified. Please specify correct value of `--target` or `-t` flag from 'kubernetes/k8s/mission-control/tmc'",
+		},


The above tests are only negative tests.
It is complicated to actually test installing a plugin from this part of the code, which I assume is why there was never a test for in this file.

marckhouzam · 2023-02-16T19:12:31Z

pkg/discovery/interface.go

 // CreateDiscoveryFromV1alpha1 creates discovery interface from v1alpha1 API
-func CreateDiscoveryFromV1alpha1(pd configtypes.PluginDiscovery) (Discovery, error) {
+func CreateDiscoveryFromV1alpha1(pd configtypes.PluginDiscovery, criteria *PluginDiscoveryCriteria) (Discovery, error) {


I don't love adding the criteria here and not respecting it for any discovery other than OCI.
However, the go code using the discoveries does filter the plugin returned, so this is safe.

marckhouzam · 2023-02-16T19:14:05Z

pkg/discovery/oci_dbbacked.go

+	// extracted from the OCI image
+	pluginDataDir string
+	// inventory is the pluginInventory to be used by this discovery.
+	inventory plugininventory.PluginInventory


The use of this field allows the unit tests to change the inventory to use a stub.

marckhouzam · 2023-02-16T19:15:59Z

pkg/discovery/oci_dbbacked_test.go

+	return pluginEntries, nil
+}
+
+// nolint: gocyclo


This stub filtering function is too complex for the linter 😄
But I had spent enough time getting those tests to run, so I just disabled the linter for it.

This is why it is better to use SQL than our own Go logic for this smart filtering 😉

marckhouzam · 2023-02-16T19:25:10Z

pkg/pluginmanager/manager.go

-func verifyPluginPreDownload(p *discovery.Discovered) error {
-	artifactInfo, err := p.Distribution.DescribeArtifact(p.RecommendedVersion, runtime.GOOS, runtime.GOARCH)
+func verifyPluginPreDownload(p *discovery.Discovered, version string) error {
+	artifactInfo, err := p.Distribution.DescribeArtifact(version, runtime.GOOS, runtime.GOARCH)


@anujc25 Please pay attention to this change as it is not limited to the new Central Repo feature.
I believe it is safe, but I would appreciate your confirmation.

The reason is:
The verifyPluginPreDownload(..) function now verifies the repo using the requested version of the plugin, instead of always using the recommendedVersion. This should cause not change in functionality IIUC. It is required because with a more targeted query of plugins, the recommendedVersion artifact is not always fetched. For example, if installing an older version of the cluster plugin, say v0.26.0, the "smart" install will only fetch information about that version; so, if we were to attempt to verifyPluginPreDownload() using the RecommendedVersion of v0.28.0, that information would not be available, and would need to be fetched separately from the SQLite DB; so, instead, we just use the requested version.

marckhouzam · 2023-02-16T19:26:41Z

pkg/pluginmanager/manager.go

+			return nil, err
+		}
+		return []configtypes.PluginDiscovery{pd}, nil
+	}


It is the above block that uses the TANZU_CLI_PRE_RELEASE_REPO_IMAGE variable.
If we comment this, we can successfully run the new Test_InstallPlugin_InstalledPlugins_Central_Repo unit test.

marckhouzam · 2023-02-17T13:25:56Z

pkg/discovery/oci.go

+		// then the image prefix should be project.registry.vmware.com/tanzu-cli/plugins/
+		imagePrefix := path.Dir(image)
+		// The data for the inventory is stored in the cache
+		pluginDataDir := filepath.Join(common.DefaultCacheDir, inventoryDirName, name)


Notice this change where the inventory SQLite DB is moved from:
~/.cache/_tanzu/plugin_inventory/plugin_inventory.db to
~/.cache/_tanzu/plugin_inventory/<discoveryName>/plugin_inventory.db

This is to allow having multiple discovery sources using SQLite at the same time.

@anujc25 Does that fits with your publication approach?

Yes. This seems reasonable.

anujc25 · 2023-02-21T17:01:36Z

pkg/plugininventory/sqlite_inventory.go

+		if filter.Version != "" {
+			if filter.Version == cli.VersionLatest {
+				// We want the recommended version of the plugin
+				whereClause = fmt.Sprintf("%s Version=RecommendedVersion AND", whereClause)


Can you explain this a bit? How will this condition be met?

Good question.

I coded this before I remembered that the Central Repo would not fill the RecommendedVersion column at the beginning. Instead of removing it, I decided to prepare for when the RecommendedVersion would be included in the Central Repo and leave it in.

However, if you look at the implementation of GetPlugins(..) in that same file, you will notice that if filter.Version == cli.VersionLatest , we compute the RecommendedVersion and set it in filter.Version.

So, currently, the code you are pointing out is actually dead code. Should we remove it?

We can keep it as it but please add a commit specifying the details and mention that this is a dead code that might be useful in the future.

pkg/plugininventory/sqlite_inventory.go

The SQLite-backed OCI discovery now has "criteria" field which allows to only discover plugins that match that criteria. At the same time the PluginInventory now has a "PluginInventoryFilter" field which allows to only List() plugins respecting the filter. When using the PluginInventory, the OCI discovery converts its criteria into a PluginInventoryFilter to only request the proper plugins. Using this filtering system, it is now possible to limit the amount of fetched plugin data to a more targeted subset. Signed-off-by: Marc Khouzam <kmarc@vmware.com>

Signed-off-by: Marc Khouzam <kmarc@vmware.com>

This new version of "plugin install" only obtains the required amount of information it needs to install a plugin. This avoids getting the entire list of plugins, which, with a Central Repository, can be large. A similar change was made for "plugin upgrade". Signed-off-by: Marc Khouzam <kmarc@vmware.com>

Signed-off-by: Marc Khouzam <kmarc@vmware.com>

anujc25

LGTM. Thanks for the detailed PR description and testing section.

Signed-off-by: Marc Khouzam <kmarc@vmware.com>

…very (#64) * Add filtering to the SQLite-backed OCI discovery The SQLite-backed OCI discovery now has "criteria" field which allows to only discover plugins that match that criteria. At the same time the PluginInventory now has a "PluginInventoryFilter" field which allows to only List() plugins respecting the filter. When using the PluginInventory, the OCI discovery converts its criteria into a PluginInventoryFilter to only request the proper plugins. Using this filtering system, it is now possible to limit the amount of fetched plugin data to a more targeted subset. The new "plugin install" now only obtains the required amount of information it needs to install a plugin. This avoids getting the entire list of plugins, which, with a Central Repository, can be large. A similar change was made for "plugin upgrade". Signed-off-by: Marc Khouzam <kmarc@vmware.com>

marckhouzam requested a review from a team as a code owner February 10, 2023 14:58

vmwclabot added the cla-not-required label Feb 10, 2023

marckhouzam force-pushed the feat/centralInventoryFilter branch from ef07d40 to 3632732 Compare February 10, 2023 20:36

marckhouzam marked this pull request as draft February 10, 2023 21:07

marckhouzam mentioned this pull request Feb 14, 2023

Replace "DiscoveryBackend" with "PluginInventory" #59

Merged

marckhouzam force-pushed the feat/centralInventoryFilter branch 7 times, most recently from 2b4efbf to 22a8e16 Compare February 16, 2023 18:53

marckhouzam marked this pull request as ready for review February 16, 2023 19:04

marckhouzam force-pushed the feat/centralInventoryFilter branch from 22a8e16 to 03945c6 Compare February 16, 2023 19:22

marckhouzam commented Feb 16, 2023

View reviewed changes

marckhouzam commented Feb 17, 2023

View reviewed changes

marckhouzam force-pushed the feat/centralInventoryFilter branch from 03945c6 to 7ba64a4 Compare February 19, 2023 15:23

anujc25 reviewed Feb 21, 2023

View reviewed changes

marckhouzam force-pushed the feat/centralInventoryFilter branch from 7ba64a4 to ea876be Compare February 22, 2023 21:22

marckhouzam added 7 commits February 22, 2023 16:34

Add unit tests for Plugin Inventory

8ad4326

Signed-off-by: Marc Khouzam <kmarc@vmware.com>

Use Artifacts map to find available versions

8408ae7

Signed-off-by: Marc Khouzam <kmarc@vmware.com>

Prepare unit testing for the new plugin install logic

879e175

Signed-off-by: Marc Khouzam <kmarc@vmware.com>

Add unit tests for plugin discovery changes

eca5789

Signed-off-by: Marc Khouzam <kmarc@vmware.com>

Add unit test for oci_dbbacked

6a98d97

Signed-off-by: Marc Khouzam <kmarc@vmware.com>

marckhouzam force-pushed the feat/centralInventoryFilter branch from ea876be to 6a98d97 Compare February 22, 2023 21:34

anujc25 approved these changes Feb 23, 2023

View reviewed changes

marckhouzam mentioned this pull request Feb 23, 2023

Add support for Plugin Groups #78

Merged

Address review comments

9b4f889

Signed-off-by: Marc Khouzam <kmarc@vmware.com>

marckhouzam merged commit 701c583 into vmware-tanzu:main Feb 23, 2023

marckhouzam deleted the feat/centralInventoryFilter branch February 23, 2023 16:45

vuil added the kind/feature Categorizes issue or PR as related to a new feature label Apr 7, 2023

marckhouzam added this to the v0.90.0 milestone Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide smarter "plugin install/upgrade" for Central Repository Discovery #64

Provide smarter "plugin install/upgrade" for Central Repository Discovery #64

marckhouzam commented Feb 10, 2023 •

edited

Loading

marckhouzam commented Feb 10, 2023

marckhouzam commented Feb 16, 2023

marckhouzam left a comment

marckhouzam Feb 16, 2023

marckhouzam Feb 16, 2023

marckhouzam Feb 16, 2023

marckhouzam Feb 16, 2023

marckhouzam Feb 16, 2023

marckhouzam Feb 16, 2023

marckhouzam Feb 16, 2023

marckhouzam Feb 17, 2023

anujc25 Feb 22, 2023

anujc25 Feb 21, 2023

marckhouzam Feb 22, 2023

anujc25 Feb 23, 2023

anujc25 left a comment

Provide smarter "plugin install/upgrade" for Central Repository Discovery #64

Provide smarter "plugin install/upgrade" for Central Repository Discovery #64

Conversation

marckhouzam commented Feb 10, 2023 • edited Loading

What this PR does / why we need it

Describe testing done for PR

Release note

Additional information

Special notes for your reviewer

marckhouzam commented Feb 10, 2023

marckhouzam commented Feb 16, 2023

marckhouzam 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

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

anujc25 left a comment

Choose a reason for hiding this comment

marckhouzam commented Feb 10, 2023 •

edited

Loading