tests: Use a function to build path for utility images #11540

vasiliy-ul · 2024-03-19T08:33:54Z

What this PR does

The PR introduces a common function to build full registry paths using the provided image name and respecting the utility repo prefix and tag.

Before this PR: image paths were hardcoded in some places without taking into account the flags (i.e. repo prefix and tag).

After this PR: common function is used. No more hardcoding.

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

NONE

victortoso · 2024-03-19T09:00:49Z

The only other place my grep found KubeVirtUtility that might be related to this PR is in tests/storage/datavolume.go:536, to split name/port.

IMHO, PR is good as is so
/lgtm

fossedihelm

Thank you!

EdDev · 2024-04-08T09:32:25Z

tests/util/images.go

+	"kubevirt.io/kubevirt/tests/flags"
+)
+
+func GetUtilityImageFromRegistry(imageName string) string {


I would prefer not to see additions under any generic util package.

Can this be placed in a more focused package?
E.g. libimageregistry or libregistry.

@EdDev, what do you think about adding the function to libinfra instead? Does it look like a suitable place? I do not have a strong opinion on that, tbh, so I am also fine with creating a new package.

If I only knew what this libinfra represents.
Based on git history, it got helpers from a prev test file named infra_test.go.

Do you think the image registry is related somehow? I just do not know.

@dhiller , can you please suggest what this libinfra represents exactly?

My guess was that it stand for infrastructure stuff. Currently, it includes smth related to certificates, http, node-labeler, etc. Just assumed that utility container images might fall into the infra category as well.

Looks like I am not alone with that pain

kubevirt/tests/migration/eviction_strategy.go

Lines 257 to 261 in 9db1b13

// TODO: move this function in `libnode` package. First resolve cycle in dependency graph

// .-> //tests/testsuite:go_default_library

// | //tests/libnode:go_default_library

// | //tests/clientcmd:go_default_library

// `-- //tests/testsuite:go_default_library

Looks like I am not alone with that pain

kubevirt/tests/migration/eviction_strategy.go

Lines 257 to 261 in 9db1b13

// TODO: move this function in `libnode` package. First resolve cycle in dependency graph

// .-> //tests/testsuite:go_default_library

// | //tests/libnode:go_default_library

// | //tests/clientcmd:go_default_library

// `-- //tests/testsuite:go_default_library

Here I am 🙂 🙋‍♂️

@vasiliy-ul , can you try and place it in the previously suggested new package?

libimageregistry or libregistry ?

Well, I tried to fix the cycle. But eventually faced a few more. There is a need to decouple a lot of stuff to fully address the problem, and the refactoring turns out pretty messy at the end. So yeah, probably for now if that is okay, I will go with a new libregistry package.

I will take one more look and probably open a follow-up PR with some attempts to move things around in the hope to make things a bit better.

@dhiller , can you please suggest what this libinfra represents exactly?

As you guessed right, the libinfra was created to collect things that originated from the slicing of tests/infrastructure.go into several files.

xpivarc

/approve
/hold
@EdDev please unhold if you agree.

kubevirt-bot · 2024-04-09T14:16:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~tests/OWNERS~~ [xpivarc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-commenter-bot · 2024-04-09T14:16:08Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

EdDev

Thank you!

kubevirt-commenter-bot · 2024-04-10T07:35:28Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

EdDev · 2024-04-10T07:35:42Z

/approve /hold @EdDev please unhold if you agree.

/unhold

kubevirt-commenter-bot · 2024-04-10T08:04:16Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

kubevirt-bot · 2024-04-10T10:24:35Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This is a common function to build full registry paths using the provided image name and respecting the utility repo prefix and tag. Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>

vasiliy-ul · 2024-04-10T11:32:16Z

Rebased the PR. No new changes.

kubevirt-commenter-bot · 2024-04-10T12:05:38Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

kubevirt-bot · 2024-04-10T22:24:59Z

@vasiliy-ul: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-conformance-arm64	`f4cce54`	link	false	`/test pull-kubevirt-conformance-arm64`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

kubevirt-commenter-bot · 2024-04-11T05:01:48Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 19, 2024

kubevirt-bot requested a review from akalenyu March 19, 2024 08:33

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Mar 19, 2024

kubevirt-bot requested review from EdDev and kbidarkar March 19, 2024 08:33

kubevirt-bot added sig/buildsystem Denotes an issue or PR that relates to changes in the build system. sig/network sig/storage size/M labels Mar 19, 2024

vasiliy-ul force-pushed the tests-utility-images branch from ca9f522 to 0eb48d6 Compare March 19, 2024 08:39

fossedihelm approved these changes Mar 20, 2024

View reviewed changes

kubevirt-bot assigned fossedihelm Mar 20, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2024

EdDev reviewed Apr 8, 2024

View reviewed changes

xpivarc reviewed Apr 9, 2024

View reviewed changes

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2024

vasiliy-ul force-pushed the tests-utility-images branch from 0eb48d6 to 06f260f Compare April 10, 2024 07:32

kubevirt-bot added size/L and removed lgtm Indicates that a PR is ready to be merged. size/M labels Apr 10, 2024

EdDev approved these changes Apr 10, 2024

View reviewed changes

kubevirt-bot assigned EdDev Apr 10, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2024

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2024

vasiliy-ul force-pushed the tests-utility-images branch from 06f260f to 2d54a74 Compare April 10, 2024 07:35

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2024

EdDev approved these changes Apr 10, 2024

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2024

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2024

vasiliy-ul mentioned this pull request Apr 10, 2024

tests: refactor: Try to avoid cycles in dependency graph #11683

Closed

8 tasks

vasiliy-ul added 3 commits April 10, 2024 13:28

tests: Add GetUtilityImageFromRegistry() function

0a2fe30

This is a common function to build full registry paths using the provided image name and respecting the utility repo prefix and tag. Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>

tests: Avoid hardcoding network binding images

a57e07d

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>

tests: Use GetUtilityImageFromRegistry() function

f4cce54

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>

vasiliy-ul force-pushed the tests-utility-images branch from 2d54a74 to f4cce54 Compare April 10, 2024 11:31

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2024

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2024

EdDev approved these changes Apr 10, 2024

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2024

kubevirt-bot merged commit bd8c81a into kubevirt:main Apr 11, 2024
36 of 37 checks passed

vasiliy-ul deleted the tests-utility-images branch April 11, 2024 06:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Use a function to build path for utility images #11540

tests: Use a function to build path for utility images #11540

vasiliy-ul commented Mar 19, 2024

victortoso commented Mar 19, 2024 •

edited

fossedihelm left a comment

EdDev Apr 8, 2024

vasiliy-ul Apr 8, 2024

EdDev Apr 8, 2024 •

edited

EdDev Apr 8, 2024

vasiliy-ul Apr 8, 2024

vasiliy-ul Apr 9, 2024

fossedihelm Apr 9, 2024

EdDev Apr 9, 2024

vasiliy-ul Apr 10, 2024

dhiller Apr 10, 2024

xpivarc left a comment

kubevirt-bot commented Apr 9, 2024

kubevirt-commenter-bot commented Apr 9, 2024

EdDev left a comment

kubevirt-commenter-bot commented Apr 10, 2024

EdDev commented Apr 10, 2024

kubevirt-commenter-bot commented Apr 10, 2024

kubevirt-bot commented Apr 10, 2024

vasiliy-ul commented Apr 10, 2024

kubevirt-commenter-bot commented Apr 10, 2024

kubevirt-bot commented Apr 10, 2024 •

edited

kubevirt-commenter-bot commented Apr 11, 2024

	// TODO: move this function in `libnode` package. First resolve cycle in dependency graph
	// .-> //tests/testsuite:go_default_library
	// \| //tests/libnode:go_default_library
	// \| //tests/clientcmd:go_default_library
	// `-- //tests/testsuite:go_default_library

tests: Use a function to build path for utility images #11540

tests: Use a function to build path for utility images #11540

Conversation

vasiliy-ul commented Mar 19, 2024

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

victortoso commented Mar 19, 2024 • edited

fossedihelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev Apr 8, 2024 • edited

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

xpivarc left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Apr 9, 2024

kubevirt-commenter-bot commented Apr 9, 2024

EdDev left a comment

Choose a reason for hiding this comment

kubevirt-commenter-bot commented Apr 10, 2024

EdDev commented Apr 10, 2024

kubevirt-commenter-bot commented Apr 10, 2024

kubevirt-bot commented Apr 10, 2024

vasiliy-ul commented Apr 10, 2024

kubevirt-commenter-bot commented Apr 10, 2024

kubevirt-bot commented Apr 10, 2024 • edited

kubevirt-commenter-bot commented Apr 11, 2024

victortoso commented Mar 19, 2024 •

edited

EdDev Apr 8, 2024 •

edited

kubevirt-bot commented Apr 10, 2024 •

edited