Setup global initializer for missing central config file #723

marckhouzam · 2024-04-03T10:59:35Z

What this PR does / why we need it

This PR is provided in two commits to make the review easier.

With the introduction of the Central Configuration in #707, there is now a central_config.yaml file part of the plugin inventory OCI image; however, CLIs < 1.3.0 do not copy this file into the cache, so this PR forces a cache invalidation and a re-download of the OCI image as soon as a CLI >= 1.3.0 notices the missing file. This guarantees that the central_config.yaml file will be properly installed. See #722 for more details.

To achieve this, the first commit of the PR implements a small framework allowing a "global initialization" of the CLI based on pre-defined triggers. Different features can register their own trigger function and a corresponding initialization function which allow for any require global initialization. The CLI will verify the triggers on any command execution and if the trigger function returns true, the corresponding initialization will be executed.

The first commit implements the global initialization framework.
The second commit uses the framework to initialize the plugin inventory cache if the central_config.yaml file is missing.

Which issue(s) this PR fixes

Fixes #722

Describe testing done for PR

What the new feature looks like

# Build a new version of the CLI
$ make build BUILD_VERSION=v1.3.1
build darwin-arm64 CLI with version: v1.3.1

# Run this new version on an empty plugin cache
rm ~/.cache/tanzu/plugin_inventory

# Note that the `tz version` command does NOT trigger the initialization
# (same for `completion` and `__complete`)
$ tz version
version: v1.3.1
buildDate: 2024-04-03
sha: b711f5630
arch: arm64

# any other command will trigger the global initializers
$ tz plugin list
Some initialization of the CLI is required.
Let's set things up for you.  This will just take a few seconds.

[i] Refreshing plugin inventory cache for "projects.registry.vmware.com/tanzu_cli/plugins/plugin-inventory:latest", this will take a few seconds.
[i] Reading plugin inventory for "projects.registry.vmware.com/tanzu_cli/plugins/plugin-inventory:latest", this will take a few seconds.

Initialization done!
==
Standalone Plugins
  NAME       DESCRIPTION                                                 TARGET  VERSION  STATUS
  telemetry  configure cluster-wide settings for vmware tanzu telemetry  global  v1.1.0   installed

Run the command again to see there is no initialization anymore as it is only run once:

$ tz plugin list
Standalone Plugins
  NAME       DESCRIPTION                                                 TARGET  VERSION  STATUS
  telemetry  configure cluster-wide settings for vmware tanzu telemetry  global  v1.1.0   installed

Now remove the central_config.yaml file to see that the CLI notices:

$ rm ~/.cache/tanzu/plugin_inventory/default/central_config.yaml

# Run any command
$ tz context list
Some initialization of the CLI is required.
Let's set things up for you.  This will just take a few seconds.

[i] Refreshing plugin inventory cache for "projects.registry.vmware.com/tanzu_cli/plugins/plugin-inventory:latest", this will take a few seconds.
[i] Reading plugin inventory for "projects.registry.vmware.com/tanzu_cli/plugins/plugin-inventory:latest", this will take a few seconds.

Initialization done!
==
  NAME                                  ISACTIVE  TYPE             PROJECT           SPACE  CLUSTERGROUP
  TAP_pre-integration-staging           false     tanzu

[i] Use '--wide' flag to view additional columns.

Main scenario motivating this PR

Prepare the test :

# We have an old CLI
$ ~/bin/tanzu.v1.2.0 version
version: v1.2.0
buildDate: 2024-02-07
sha: f3abe62e
arch: arm64

# And the new one built with the PR
$ tz version
version: v1.3.0-dev
buildDate: 2024-04-03
sha: b711f5630
arch: arm64

# Use a central repo that has a `central_config.yaml` file
$ tz config set env.TANZU_CLI_PLUGIN_DISCOVERY_IMAGE_SIGNATURE_VERIFICATION_SKIP_LIST localhost:9876/tanzu-cli/plugins/central:small

$ make start-test-central-repo
[...]

# Start with no cache and no data store
$ rm ~/.cache/tanzu/plugin_inventory

Use an old CLI to first setup the cache. This is similar to what a user of CLI v1.2 would have done.
Notice the central_config.yaml file is not copied to the cache

$ ~/bin/tanzu.v1.2.0 plugin source update default -u localhost:9876/tanzu-cli/plugins/central:small
[i] Refreshing plugin inventory cache for "localhost:9876/tanzu-cli/plugins/central:small", this will take a few seconds.
[i] Reading plugin inventory for "localhost:9876/tanzu-cli/plugins/central:small", this will take a few seconds.
[!] Skipping the plugins discovery image signature verification for "localhost:9876/tanzu-cli/plugins/central:small"

[i] Refreshing plugin inventory cache for "localhost:9876/tanzu-cli/plugins/central:small", this will take a few seconds.
[ok] updated discovery source default

# Notice that the central_config.yaml file is not in the cache
# because CLIs < 1.3.0 don't copy it over
$ ls ~/.cache/tanzu/plugin_inventory/default
digest.ca212530af87bcf8c41b4b89c48cf453c86621e5e8f9fb501d219a582878f336 plugin_inventory.db
metadata.digest.none

Now test the actual scenario driving this change.
Normally, when running v1.3.0-alpha.2 with the test repo we should see a notification saying v1.3.3 is available for upgrade. However, notice this does not happen because the central_config.yaml file is missing from the cache.

$ /opt/homebrew/bin/tanzu version
version: v1.3.0-alpha.2
buildDate: 2024-03-29
sha: fdbe2498
arch: arm64

$ /opt/homebrew/bin/tanzu config set env.TEST 1
# Here there should have been a notification printed about upgrading to v1.3.3 but there is not.  That is the problem.

# Let's try the same thing with this PR
# which will first force a re-download of the OCI image
$ tz config set env.TEST 1
Some initialization of the CLI is required.
Let's set things up for you.  This will just take a few seconds.

[i] Refreshing plugin inventory cache for "localhost:9876/tanzu-cli/plugins/central:small", this will take a few seconds.
[i] Reading plugin inventory for "localhost:9876/tanzu-cli/plugins/central:small", this will take a few seconds.
[!] Skipping the plugins discovery image signature verification for "localhost:9876/tanzu-cli/plugins/central:small"


Initialization done!
==

==
Note: A new version of the Tanzu CLI is available. You are at version: v1.3.0-dev.
To benefit from the latest security and features, please update to a recommended version:
  - v1.5.0-beta.0
  - v1.3.3

Please refer to these instructions for upgrading: https://github.com/vmware-tanzu/tanzu-cli/blob/main/docs/quickstart/install.md.

This message will print at most once per 24 hours until you update the CLI.
Set TANZU_CLI_RECOMMEND_VERSION_DELAY_DAYS to adjust this period (0 to disable).

# And finally, just to see it with our own eyes, check the presence of `central_config.yaml`
$ ls ~/.cache/tanzu/plugin_inventory/default
central_config.yaml                                                     metadata.digest.none
digest.ca212530af87bcf8c41b4b89c48cf453c86621e5e8f9fb501d219a582878f336 plugin_inventory.db

Release note

Implement "global initializers" and use them to fix the plugin cache if the `central_config.yaml` file is missing.

Additional information

Special notes for your reviewer

anujc25

Looks great. Just a minor question but otherwise looks good to me.

pkg/centralconfig/central_config_cache_init.go

pkg/globalinit/global_initializers.go

vuil

Good stuff!
Some minor comments on the framework part

pkg/globalinit/global_initializers.go

vuil · 2024-04-17T04:17:56Z

pkg/globalinit/global_initializers.go

+	kerrors "k8s.io/apimachinery/pkg/util/errors"
+)
+
+type initializer struct {


a suggestion: how about support providing a name for each initializer registered? It can be useful for a few things:

Used in aggregated error list to provide some consistency in the output

In the future, should it happen that a particular initializer consistently fails for a CLI user (intentionally caused by the customer or otherwise), it is much easier to provide some means to turn off certain initializers if we can refer to them by name

vuil · 2024-04-17T04:22:08Z

pkg/globalinit/global_initializers.go

+// RegisterInitializer registers a new initializer with the global list of initializers.
+// The trigger function is used to determine if the initialization function should be run.
+// The initialization function is the function that will be run if the trigger function returns true.
+// The set of initializer triggers is checked whenever the CLI is run.


weak suggestion:
while no initializer should be written in a way that depends on other initializers, consider mentioning that the initializers will be run in the order that they are registered.

Good point. In fact, while working on a second initializer I realized that the order may matter.
For example, the current initializer will make sure the central_config.yaml file is in the cache; another initializer may want to read the the central config, so having the cache refresh run first would be useful.

Controlling the order needs a bit of thought. Currently I use an init() function, but those are hard to order. We could use an extra order or rank argument to the RegisterInitializer() function of value 1 to 100 and we could then register a position in the order.

Let's discuss.

We will keep this point for later

vuil · 2024-04-17T04:31:18Z

pkg/globalinit/global_initializers_test.go

+			unexpectedOut: []string{"1", "3"},
+		},
+		{
+			test: "init throws error",


nit: I think one interesting case to include in multiple initFuncs, returning a mix of success and error (e.g. erro just on the second of 3). Demonstrates that error does not interrupt other initializers.

vuil

A couple more nits and suggestions.

pkg/centralconfig/central_config_cache_init.go

test/e2e/cli_lifecycle/initialize_central_config_test.go

vuil · 2024-04-17T05:07:18Z

test/e2e/cli_lifecycle/initialize_central_config_test.go

+			Expect(errStream).ToNot(ContainSubstring(initializationStr))
+			Expect(errStream).ToNot(ContainSubstring(initErrorStr))
+		})
+		It("initialization when config file is missing a second time", func() {


nit: test title and comments aside, this test looks identical to the previous one!
To prove the point that an initialized CLI seeing a deleted central config afterwards can still re-initialize correctly (which I gather is the purpose of this test), you might be better off just running the previous test in a loop of 2 iterations and combining the comments.

Good eye 👍 , will do

pkg/command/root.go

I fatfingered the approval :-P

The concept of global initializers is added to allow the CLI to prepare its data on certain conditions (triggers). The commit also uses one such initializer to fix a plugin cache that is missing the central_config.yaml file. Signed-off-by: Marc Khouzam <marc.khouzam@broadcom.com>

Signed-off-by: Marc Khouzam <marc.khouzam@broadcom.com>

vuil

Thanks for making prompt updates. The remaining issues (e.g. regarding ordering of the initializers) are not blockers at the moment, and can be addressed in a followup after some discussion

…u#723) The concept of global initializers is added to allow the CLI to prepare its data on certain conditions (triggers). The commit also uses one such initializer to fix a plugin cache that is missing the central_config.yaml file.

The concept of global initializers is added to allow the CLI to prepare its data on certain conditions (triggers). The commit also uses one such initializer to fix a plugin cache that is missing the central_config.yaml file.

marckhouzam requested a review from a team as a code owner April 3, 2024 10:59

vmwclabot added the cla-not-required label Apr 3, 2024

marckhouzam force-pushed the feat/refreshOnUpgrade branch from b711f56 to 76c083c Compare April 3, 2024 14:09

marckhouzam added this to the v1.3.0 milestone Apr 4, 2024

marckhouzam added kind/feature Categorizes issue or PR as related to a new feature docs-impact issues with documentation impact labels Apr 4, 2024

vuil self-requested a review April 5, 2024 15:40

anujc25 mentioned this pull request Apr 9, 2024

Unify the plugin installation for standalone and context-scoped plugins and use context as recommendation #698

Merged

marckhouzam force-pushed the feat/refreshOnUpgrade branch from 76c083c to d324648 Compare April 12, 2024 12:33

marckhouzam marked this pull request as draft April 12, 2024 16:49

marckhouzam force-pushed the feat/refreshOnUpgrade branch 5 times, most recently from 451364b to d9e21bc Compare April 15, 2024 12:47

marckhouzam changed the title ~~Refresh the DB cache on a new version installation~~ Setup global initializer for missing central config file Apr 15, 2024

marckhouzam marked this pull request as ready for review April 15, 2024 13:11

anujc25 reviewed Apr 16, 2024

View reviewed changes

pkg/centralconfig/central_config_cache_init.go Show resolved Hide resolved

pkg/globalinit/global_initializers.go Show resolved Hide resolved

vuil reviewed Apr 17, 2024

View reviewed changes

vuil previously approved these changes Apr 17, 2024

View reviewed changes

vuil self-requested a review April 17, 2024 05:23

marckhouzam added 4 commits April 18, 2024 08:35

Perform global init of central cfg file

2c8e2ab

Signed-off-by: Marc Khouzam <marc.khouzam@broadcom.com>

Address comments

c6bed1b

Signed-off-by: Marc Khouzam <marc.khouzam@broadcom.com>

Add a name to global initializers

3e358c7

Signed-off-by: Marc Khouzam <marc.khouzam@broadcom.com>

marckhouzam force-pushed the feat/refreshOnUpgrade branch from d9e21bc to 3e358c7 Compare April 18, 2024 16:50

vuil approved these changes Apr 18, 2024

View reviewed changes

marckhouzam merged commit d6ec358 into vmware-tanzu:main Apr 19, 2024
7 checks passed

marckhouzam deleted the feat/refreshOnUpgrade branch April 19, 2024 00:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup global initializer for missing central config file #723

Setup global initializer for missing central config file #723

marckhouzam commented Apr 3, 2024 •

edited

anujc25 left a comment

vuil left a comment

vuil Apr 17, 2024 •

edited

vuil Apr 17, 2024

marckhouzam Apr 18, 2024

marckhouzam Apr 19, 2024

vuil Apr 17, 2024

vuil left a comment

vuil Apr 17, 2024

marckhouzam Apr 18, 2024

vuil left a comment

Setup global initializer for missing central config file #723

Setup global initializer for missing central config file #723

Conversation

marckhouzam commented Apr 3, 2024 • edited

What this PR does / why we need it

Which issue(s) this PR fixes

Describe testing done for PR

What the new feature looks like

Main scenario motivating this PR

Release note

Additional information

Special notes for your reviewer

anujc25 left a comment

Choose a reason for hiding this comment

vuil left a comment

Choose a reason for hiding this comment

vuil Apr 17, 2024 • edited

Choose a reason for hiding this comment

vuil Apr 17, 2024

Choose a reason for hiding this comment

marckhouzam Apr 18, 2024

Choose a reason for hiding this comment

marckhouzam Apr 19, 2024

Choose a reason for hiding this comment

vuil Apr 17, 2024

Choose a reason for hiding this comment

vuil left a comment

Choose a reason for hiding this comment

vuil Apr 17, 2024

Choose a reason for hiding this comment

marckhouzam Apr 18, 2024

Choose a reason for hiding this comment

vuil left a comment

Choose a reason for hiding this comment

marckhouzam commented Apr 3, 2024 •

edited

vuil Apr 17, 2024 •

edited