Add support for pulling and pushing all images #1061

zubron · 2019-12-20T15:24:39Z

What this PR does / why we need it:
This changes adds support for pulling the Sonobuoy worker image, the
systemd-logs image and the Kubernetes conformance image in addition to
the images required for the e2e plugin.

The individual images to pull cannot be configured. Only the default
version for the cluster will be pulled. The images can only be pushed to
one registry which is specified with the new flags --custom-registry.

This currently only works for the default built in plugins. A follow up
change will be made to apply the same approach to any image in a plugin
definition.

Signed-off-by: Bridget McErlean bmcerlean@vmware.com

Which issue(s) this PR fixes

Special notes for your reviewer:
I've tested the new commands for this manually. I ran the following locally:

sonobuoy images pull -p systemd-logs
sonobuoy images push -p systemd-logs --custom-registry zubron

And you can see that the images were pushed to my public registry: https://hub.docker.com/u/zubron

Release note:

Sonobuoy can now pull all images for Sonobuoy and the built-in plugins and push them to a custom registry. The E2E test images are still configured as before, however all other images will be pushed to a custom registry specified using the flag `--custom-registry`. The image operations can also be previewed by using the `--dry-run` flag.

zubron · 2019-12-20T17:26:44Z

cmd/sonobuoy/app/images.go

+func downloadImages(plugins []string, kubeconfig Kubeconfig, e2eRegistryConfig string) error {
+ for _, plugin := range plugins {
+ switch plugin {
+ case e2ePlugin:


I haven't updated this function as I don't know if we should maintain it: #1058

codecov-io · 2019-12-20T17:30:11Z

Codecov Report

Merging #1061 into master will increase coverage by 0.79%.
The diff coverage is 33.59%.

@@            Coverage Diff            @@
##           master   #1061      +/-   ##
=========================================
+ Coverage   49.91%   50.7%   +0.79%     
=========================================
  Files          78      79       +1     
  Lines        5634    5745     +111     
=========================================
+ Hits         2812    2913     +101     
- Misses       2654    2659       +5     
- Partials      168     173       +5

Impacted Files	Coverage Δ
pkg/image/dryrun_client.go	`0% <0%> (ø)`
cmd/sonobuoy/app/args.go	`91.05% <100%> (+0.49%)`	⬆️
pkg/image/manifest.go	`70.43% <54.28%> (+1.54%)`	⬆️
cmd/sonobuoy/app/gen.go	`75.88% <83.33%> (+1.41%)`	⬆️
cmd/sonobuoy/app/images.go	`26.12% <9.93%> (-5.91%)`	⬇️
pkg/image/docker_client.go	`92.1% <92.1%> (ø)`
pkg/plugin/aggregation/aggregator.go	`75.77% <0%> (+3.1%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb0c5a3...e928173. Read the comment docs.

zubron · 2019-12-20T17:37:33Z

cmd/sonobuoy/app/images.go

- return imageClient.DeleteImages(images, numDockerRetries)
+func substituteRegistry(image string, customRegistry string) string {
+ trimmedRegistry := strings.TrimRight(customRegistry, "/")
+ components := strings.SplitAfter(image, "/")


I thought I left a comment on this but it's gotten lost 😕

This is a best attempt at replacing the registry in a full image name. It's possible for the "name" component of an image name to also contain a / and we see that in some of the E2E test images. In cases like these, this function would substitute everything up to the last / which could include the registry and part of the image name. I don't think there's a way to identify what is the registry URL component of a FQIN. The images will still tag and push correctly, but it might be confusing for users when replacing images in their plugin definitions.

Its a short function but I think this is a good function to have in a function so that we can visually ensure it handles certain cases.

zubron · 2019-12-20T17:54:52Z

cmd/sonobuoy/app/args.go

@@ -348,6 +351,11 @@ func AddPluginEnvFlag(p *PluginEnvVars, flags *pflag.FlagSet) {
 flags.Var(p, "plugin-env", "Set env vars on plugins. Values can be given multiple times and are in the form plugin.env=value")
 }

+// AddPluginListFlag adds the flag to keep track of which built-in plugins to use.
+func AddPluginListFlag(p *[]string, flags *pflag.FlagSet) {
+ flags.StringSliceVarP(p, "plugin", "p", []string{"e2e"}, "Describe which plugin's images to interact (Valid plugins are 'e2e', 'systemd-logs').")


Should this default to both plugins now? Seems like that would be simpler UX here.

Yes it looks like this is just used for the images command. If we are going to, by default, try to run both of those plugins and the images commands can support both of them, I agree it should.

johnSchnake · 2019-12-20T22:19:49Z

cmd/sonobuoy/app/images.go

- if err != nil {
- return errors.Wrap(err, "failed to get cluster version")
- }
+func listImages(plugins []string, kubeconfig Kubeconfig) error {


This is another good candidate for a test function; we can add it when we expand to other plugins though. I know the systemd-logs test wouldnt be very significant and the e2e one is a bit awkward to test.

johnSchnake

I'll have to try this out a bit more before I want to merge it; no rush. Enjoy the time off.

johnSchnake · 2020-01-06T19:18:40Z

My only other idea for this is if it would be useful to have a dry-run option when pushing so that you can confirm the repos/tags are as expected.

When trying to manually confirm this all works, I can't actually check much manually without actually pushing some images.

Otherwise, my only comments were (already commented but adding for clarity):

ideally add another test to ensure we are splitting registries/tags appropriately
default to doing both systemd-logs and e2e

zubron · 2020-01-07T13:36:29Z

Thanks for the feedback! I've addressed your two comments. I also added a dry-run option which just performs the logging operations that the existing docker image client does. I've left it as a separate commit for now. Let me know what you think!

johnSchnake

I ❤️ the dry run client you added; it surpasses what I was hoping for. Good to squash/merge

This changes adds support for pulling the Sonobuoy worker image, the systemd-logs image and the Kubernetes conformance image in addition to the images required for the e2e plugin. The individual images to pull cannot be configured. Only the default version for the cluster will be pulled. The images can only be pushed to one registry which is specified with the new flags `--custom-registry`. This currently only works for the default built in plugins. A follow up change will be made to apply the same approach to any image in a plugin definition. Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

zubron force-pushed the include-all-images-1028 branch 2 times, most recently from 5301e9a to 02fbd27 Compare December 20, 2019 17:24

zubron commented Dec 20, 2019

View reviewed changes

zubron force-pushed the include-all-images-1028 branch from 02fbd27 to 27ee39d Compare December 20, 2019 17:39

zubron requested a review from johnSchnake December 20, 2019 17:41

zubron commented Dec 20, 2019

View reviewed changes

zubron force-pushed the include-all-images-1028 branch from 27ee39d to fad6f45 Compare December 20, 2019 18:00

zubron marked this pull request as ready for review December 20, 2019 18:03

johnSchnake reviewed Dec 20, 2019

View reviewed changes

zubron force-pushed the include-all-images-1028 branch from fad6f45 to ea10f43 Compare December 20, 2019 23:12

zubron force-pushed the include-all-images-1028 branch from ea10f43 to 7a0296e Compare January 7, 2020 13:34

johnSchnake approved these changes Jan 7, 2020

View reviewed changes

zubron force-pushed the include-all-images-1028 branch from 7a0296e to e928173 Compare January 7, 2020 16:11

zubron merged commit 23d1d5c into vmware-tanzu:master Jan 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for pulling and pushing all images #1061

Add support for pulling and pushing all images #1061

zubron commented Dec 20, 2019 •

edited

Loading

zubron Dec 20, 2019

codecov-io commented Dec 20, 2019 •

edited

Loading

zubron Dec 20, 2019

johnSchnake Dec 20, 2019

zubron Dec 20, 2019

johnSchnake Jan 6, 2020

johnSchnake Dec 20, 2019

johnSchnake left a comment

johnSchnake commented Jan 6, 2020

zubron commented Jan 7, 2020

johnSchnake left a comment

Add support for pulling and pushing all images #1061

Add support for pulling and pushing all images #1061

Conversation

zubron commented Dec 20, 2019 • edited Loading

zubron Dec 20, 2019

Choose a reason for hiding this comment

codecov-io commented Dec 20, 2019 • edited Loading

Codecov Report

zubron Dec 20, 2019

Choose a reason for hiding this comment

johnSchnake Dec 20, 2019

Choose a reason for hiding this comment

zubron Dec 20, 2019

Choose a reason for hiding this comment

johnSchnake Jan 6, 2020

Choose a reason for hiding this comment

johnSchnake Dec 20, 2019

Choose a reason for hiding this comment

johnSchnake left a comment

Choose a reason for hiding this comment

johnSchnake commented Jan 6, 2020

zubron commented Jan 7, 2020

johnSchnake left a comment

Choose a reason for hiding this comment

zubron commented Dec 20, 2019 •

edited

Loading

codecov-io commented Dec 20, 2019 •

edited

Loading