Skip to content

cli: Added parameter to print used images #37390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 26, 2025
Merged

cli: Added parameter to print used images #37390

merged 1 commit into from
Feb 26, 2025

Conversation

PhilipSchmid
Copy link
Contributor

@PhilipSchmid PhilipSchmid commented Jan 31, 2025 •
edited
Loading

Added --print-image-artifacts for the cilium connectivity test|perf subcommands so it's easier to know which image dependencies there are. This is especially helpful when running the CLI tests in an air-happed environment.

ToDos:

  • The go tests do not yet work. buf.String() always seems empty.
  • Does anyone see a nicer solution than iterating over the images individually? The default images are defined here as const string values (so not in an array/map or something).

Manual testing:

$ ./cilium connectivity test --print-image-artifacts
quay.io/cilium/alpine-curl:v1.10.0@sha256:913e8c9f3d960dde03882defa0edd3a919d529c2eb167caa7f54194528bde364
quay.io/cilium/json-mock:v1.3.8@sha256:5aad04835eda9025fe4561ad31be77fd55309af8158ca8663a72f6abb78c2603
docker.io/coredns/coredns:1.12.0@sha256:40384aa1f5ea6bfdc77997d243aec73da05f27aed0c5e9d65bfa98933c519d97
quay.io/cilium/test-connection-disruption:v0.0.14@sha256:c3fd56e326ae16f6cb63dbb2e26b4e47ec07a123040623e11399a7fe1196baa0
quay.io/frrouting/frr:10.2.1@sha256:c8543d3e0a1348cc0f2b19154fd8b0300e237773dbec65d9d6d6570c1d088deb
docker.io/alpine/socat:1.8.0.1@sha256:d95d6a210a87164533d444e8d7ebd586231b3387a27ee7c0732ade3d6c3b0f4d

$ ./cilium connectivity perf --print-image-artifacts
quay.io/cilium/network-perf:a816f935930cb2b40ba43230643da4d5751a5711@sha256:679d3a370c696f63884da4557a4466f3b5569b4719bb4f86e8aac02fbe390eea

$ ./cilium connectivity test --print-image-artifacts --curl-image "alpine/curl:latest"
alpine/curl:latest
quay.io/cilium/json-mock:v1.3.8@sha256:5aad04835eda9025fe4561ad31be77fd55309af8158ca8663a72f6abb78c2603
docker.io/coredns/coredns:1.12.0@sha256:40384aa1f5ea6bfdc77997d243aec73da05f27aed0c5e9d65bfa98933c519d97
quay.io/cilium/test-connection-disruption:v0.0.14@sha256:c3fd56e326ae16f6cb63dbb2e26b4e47ec07a123040623e11399a7fe1196baa0
quay.io/frrouting/frr:10.2.1@sha256:c8543d3e0a1348cc0f2b19154fd8b0300e237773dbec65d9d6d6570c1d088deb
docker.io/alpine/socat:1.8.0.1@sha256:d95d6a210a87164533d444e8d7ebd586231b3387a27ee7c0732ade3d6c3b0f4d

$ ./cilium connectivity perf --print-image-artifacts --performance-image "alpine:latest"
alpine:latest
cli: Added parameter to print used images

cc @tklauser

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 31, 2025
@github-actions github-actions bot added cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary labels Jan 31, 2025
@tklauser
Copy link
Member

Added --print-image-artifacts for the cilium connectivity test|perf subcommands so it's easier to know which image dependencies there are. This is especially helpful when running the CLI tests in an air-happed environment.

Nice work!

ToDos:

  • The go tests do not yet work. buf.String() always seems empty.

I think the problem is that the artifacts are printed to stdout using fmt.Println here: https://github.com/cilium/cilium/pull/37390/files#diff-5daff2cf2c5a2019340911b1cbdc3e39095398152d1f79aecd52e0c9fc3b3187R80-R87

This won't be captured by the buffer set on ct.SetOut. I think you'll need to use fmt.Fprintln(params.Writer, <image>) instead and then set params.Writer = &buf in TestPrintImageArtifacts to capture the output.

  • Does anyone see a nicer solution than iterating over the images individually? The default images are defined here as const string values (so not in an array/map or something).

I'd probably change the image definitions to be stored in a slice or map in defaults/defaults.go. That way we won't have to repeat the list of images in tests and elsewhere.

tklauser
tklauser reviewed Jan 31, 2025
View reviewed changes
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/cli_image_artifacts_flag branch from a50f4e3 to 37527c0 Compare February 13, 2025 08:57
@PhilipSchmid
Copy link
Contributor Author

Rebased to main

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/cli_image_artifacts_flag branch 3 times, most recently from bf10563 to 5aab7b2 Compare February 13, 2025 16:59
PhilipSchmid
PhilipSchmid commented Feb 13, 2025
View reviewed changes
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/cli_image_artifacts_flag branch from 5aab7b2 to aff456d Compare February 13, 2025 17:17
@PhilipSchmid PhilipSchmid marked this pull request as ready for review February 13, 2025 17:21
@PhilipSchmid PhilipSchmid requested review from a team as code owners February 13, 2025 17:21
@Artyop Artyop requested a review from tklauser February 14, 2025 09:13
Artyop
Artyop approved these changes Feb 14, 2025
View reviewed changes
Copy link
Contributor

@Artyop Artyop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

asauber
asauber approved these changes Feb 14, 2025
View reviewed changes
@tklauser tklauser added the release-note/misc This PR makes changes that have no direct user impact. label Feb 25, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 25, 2025
tklauser
tklauser approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Philip!

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/cli_image_artifacts_flag branch from aff456d to 9800799 Compare February 25, 2025 08:15
@PhilipSchmid
Copy link
Contributor Author

Rebased to main and resolved conflicts. Thanks for approving, everyone! 🙏

@PhilipSchmid
cli: Added parameter to print used images
cf802c4 
Added `--print-image-artifacts` for the cilium connectivity test|perf
subcommands so it's easier to know which image dependencies there are.
This is especially helpful when running the CLI tests in an air-happed
environment.

Signed-off-by: Philip Schmid <phisch@cisco.com>
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/cli_image_artifacts_flag branch from 9800799 to cf802c4 Compare February 25, 2025 10:06
@tklauser
Copy link
Member

/test

@tklauser tklauser enabled auto-merge February 25, 2025 10:07
@tklauser tklauser added this pull request to the merge queue Feb 26, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 26, 2025
Merged via the queue into cilium:main with commit cf6056a Feb 26, 2025
60 of 63 checks passed
@github-actions github-actions bot mentioned this pull request Mar 10, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@asauber asauber asauber approved these changes

@Artyop Artyop Artyop approved these changes

Assignees
No one assigned
Labels
cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PhilipSchmid @tklauser @asauber @Artyop