Integrate sidecar shim image #11272

dharmit · 2024-02-16T08:12:42Z

What this PR does

Before this PR: image field in the hooks.kubevirt.io/hookSidecars annotation in this example was mandatory.

After this PR: aforementioned field will be optional.

Fixes #11161

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

The following tradeoffs were made: N/A

The following alternatives were considered: N/A

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

Make 'image' field in hook sidecar annotation optional.

kubevirt-bot · 2024-02-16T08:12:52Z

Hi @dharmit. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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.

Wait for: kubevirt/kubevirt#11272 to get the --sidecar-shim-image parameter on the CSV generator on kubevirt oeprator Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

dharmit · 2024-03-04T14:26:46Z

@alicefr @victortoso @vasiliy-ul can you folks help with review? Pinging you folks since I have noticed you being active on sidecar hook side of the things. :)

vasiliy-ul

Overall, this looks okay. One thing to consider: we already have some tests for sidecars in tests/vmi_hook_sidecar_test.go. But they all explicitly set the image. Perhaps we shall consider changing that, so at least a couple of tests run with an empty image. WDYT?

vasiliy-ul · 2024-03-05T08:43:42Z

pkg/virt-controller/services/template.go

@@ -22,7 +22,9 @@ package services
 import (
 	"context"
 	"fmt"
+	operatorutil "kubevirt.io/kubevirt/pkg/virt-operator/util"


nit: can we move this to the group with local imports?

vasiliy-ul · 2024-03-05T08:47:51Z

/test pull-kubevirt-build

vasiliy-ul · 2024-03-05T08:48:31Z

/test pull-kubevirt-unit-test

vasiliy-ul · 2024-03-05T09:03:26Z

There are some formatting issues.

vasiliy-ul · 2024-03-05T10:29:24Z

Also, unit-test failure seems caused by this change.

dharmit · 2024-03-06T08:25:46Z

Perhaps we shall consider changing that, so at least a couple of tests run with an empty image. WDYT?

I agree. I have added one test. If you have a suggestion for more tests in the same file, let me know. I'm open to add more.

dharmit · 2024-03-06T08:27:02Z

There are some formatting issues.

Can you point? I found that running make test modified the bazel file. Is that the one you're talking about?

Also, unit-test failure seems caused by this change.

Thanks. Fixed.

dharmit · 2024-03-06T08:27:23Z

/test pull-kubevirt-unit-test

kubevirt-bot · 2024-03-06T08:27:39Z

@dharmit: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test pull-kubevirt-unit-test

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.

vasiliy-ul · 2024-03-06T08:39:33Z

Can you point? I found that running make test modified the bazel file. Is that the one you're talking about?

@dharmit, you can see the formatting issue in the details for the pull-kubevirt-build job: https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/11272/pull-kubevirt-build/1764935860379193344

diff -u ./pkg/hooks/hooks.go.orig ./pkg/hooks/hooks.go
--- ./pkg/hooks/hooks.go.orig	2024-03-05 08:50:01.812356865 +0000
+++ ./pkg/hooks/hooks.go	2024-03-05 08:50:01.812356865 +0000
@@ -21,6 +21,7 @@
 
 import (
 	"encoding/json"
+
 	k8sv1 "k8s.io/api/core/v1"
 
 	v1 "kubevirt.io/api/core/v1"
diff -u ./pkg/virt-controller/services/template.go.orig ./pkg/virt-controller/services/template.go
--- ./pkg/virt-controller/services/template.go.orig	2024-03-05 08:50:03.168358416 +0000
+++ ./pkg/virt-controller/services/template.go	2024-03-05 08:50:03.168358416 +0000
@@ -22,12 +22,13 @@
 import (
 	"context"
 	"fmt"
-	operatorutil "kubevirt.io/kubevirt/pkg/virt-operator/util"
 	"math/rand"
 	"os"
 	"strconv"
 	"strings"
 
+	operatorutil "kubevirt.io/kubevirt/pkg/virt-operator/util"
+
 	networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
 	k8sv1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"

Basically, you just need to build your changes locally and the autoformatter will adjust everything as needed. Then simply add and commit the changes.

One of the formatting issue is actually about #11272 (comment), which you probably fixed already.

tests/vmi_hook_sidecar_test.go

hack/generate.sh

tests/vmi_hook_sidecar_test.go

Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>

dharmit · 2024-03-14T11:36:55Z

@dharmit, there are compile failures.

/lgtm cancel

Oops. My apologies. I didn't run make test or make lint before pushing. I have done that this time.

Can you please help trigger a couple of selected tests to verify if we can move further with full tests?

vasiliy-ul · 2024-03-14T12:09:21Z

/test pull-kubevirt-unit-test
/test pull-kubevirt-code-lint
/test pull-kubevirt-unit-test-arm64
/test pull-kubevirt-goveralls

vasiliy-ul · 2024-03-14T12:33:11Z

/test pull-kubevirt-goveralls

dharmit · 2024-03-14T12:36:33Z

/test pull-kubevirt-goveralls

I wonder if it's a real failure. Let's see. 😕

vasiliy-ul · 2024-03-14T12:40:35Z

I think the failure is not related to this PR. Probably the job is not very stable. But lets just try one more time.

brianmcarey · 2024-03-14T12:42:24Z

/test pull-kubevirt-goveralls

I wonder if it's a real failure. Let's see. 😕

Theres a known issue with the goveralls lane on main at the moment - @fossedihelm is working on a fix. The lane is not required at the moment so it should be ok.

vasiliy-ul · 2024-03-14T12:43:53Z

Ok, then

/lgtm

kubevirt-commenter-bot · 2024-03-14T12:43:59Z

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-commenter-bot · 2024-03-15T06:44:18Z

/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.

dharmit · 2024-03-15T09:33:53Z

@vasiliy-ul @brianmcarey should the 'goveralls' lane be manually overwritten? If yes, someone will have to do it on this PR.

victortoso · 2024-03-15T09:40:20Z

should the 'goveralls' lane be manually overwritten

I think the job is not required so when tide finish the retesting, this should be merged, no?

brianmcarey · 2024-03-15T09:41:15Z

should the 'goveralls' lane be manually overwritten

I think the job is not required so when tide finish the retesting, this should be merged, no?

Yes tide should handle it - its in a pool waiting for retest. https://prow.ci.kubevirt.io/tide

kubevirt-bot · 2024-03-15T17:24:17Z

@dharmit: 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-goveralls	`8600813`	link	false	`/test pull-kubevirt-goveralls`

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.

with kubevirt/kubevirt#11272 image field becomes optional in the hook sidecar annotation since, if omitted, virt-controller will now use a default sidecar-shim image built with other kubevirt images. Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

tiraboschi · 2024-04-09T15:00:00Z

/cherry-pick release-1.2

kubevirt-bot · 2024-04-09T15:00:38Z

@tiraboschi: #11272 failed to apply on top of branch "release-1.2":

Applying: Sort arguments for readability
Applying: Integrate sidecar shim image
Using index info to reconstruct a base tree...
M	hack/generate.sh
M	pkg/virt-operator/kubevirt_test.go
M	pkg/virt-operator/resource/apply/apps_test.go
M	pkg/virt-operator/resource/generate/components/daemonsets.go
M	pkg/virt-operator/resource/generate/install/strategy.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-operator/resource/generate/install/strategy.go
Auto-merging pkg/virt-operator/resource/generate/components/daemonsets.go
Auto-merging pkg/virt-operator/resource/apply/apps_test.go
CONFLICT (content): Merge conflict in pkg/virt-operator/resource/apply/apps_test.go
Auto-merging pkg/virt-operator/kubevirt_test.go
CONFLICT (content): Merge conflict in pkg/virt-operator/kubevirt_test.go
Auto-merging hack/generate.sh
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Integrate sidecar shim image
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.2

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.

with kubevirt/kubevirt#11272 image field becomes optional in the hook sidecar annotation since, if omitted, virt-controller will now use a default sidecar-shim image built with other kubevirt images. Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

Wait for: kubevirt/kubevirt#11272 to get the --sidecar-shim-image parameter on the CSV generator on kubevirt oeprator Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

kubevirt-bot added the size/L label Feb 16, 2024

kubevirt-bot requested review from kbidarkar, lyarwood and rthallisey February 16, 2024 08:13

dharmit force-pushed the fix-cnv-36620 branch from 8b14f4b to e281eb6 Compare February 19, 2024 13:30

dharmit force-pushed the fix-cnv-36620 branch from e281eb6 to a5e8661 Compare March 1, 2024 11:03

dharmit marked this pull request as ready for review March 4, 2024 14:25

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2024

kubevirt-bot requested review from enp0s3 and mhenriks March 4, 2024 14:25

vasiliy-ul reviewed Mar 5, 2024

View reviewed changes

dharmit force-pushed the fix-cnv-36620 branch from a5e8661 to af475ff Compare March 6, 2024 07:58

dharmit force-pushed the fix-cnv-36620 branch from af475ff to abeaefe Compare March 6, 2024 08:50

vasiliy-ul reviewed Mar 6, 2024

View reviewed changes

tests/vmi_hook_sidecar_test.go Outdated Show resolved Hide resolved

hack/generate.sh Outdated Show resolved Hide resolved

dharmit commented Mar 7, 2024

View reviewed changes

tests/vmi_hook_sidecar_test.go Outdated Show resolved Hide resolved

dharmit added 2 commits March 14, 2024 17:02

Make image field optional for sidecar

63fc108

Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>

Fix incorrect help message

8600813

Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>

dharmit force-pushed the fix-cnv-36620 branch from d4bdd80 to 8600813 Compare March 14, 2024 11:32

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

kubevirt-bot merged commit c8f756d into kubevirt:main Mar 16, 2024
35 of 37 checks passed

alicefr mentioned this pull request Mar 22, 2024

Add how to retrieve the version of the hook sidecar shim version from kubevirt cr kubevirt/user-guide#775

Closed

tiraboschi mentioned this pull request Apr 9, 2024

image field in hook sidecar annotation is optional kubevirt/user-guide#786

Merged

8 tasks

tiraboschi mentioned this pull request Apr 9, 2024

[release-1.2] Integrate sidecar shim image #11675

Merged

8 tasks

dharmit deleted the fix-cnv-36620 branch April 10, 2024 05:31

tiraboschi mentioned this pull request Jul 1, 2024

Enhance sidecar-shim README #12259

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate sidecar shim image #11272

Integrate sidecar shim image #11272

Integrate sidecar shim image #11272

Integrate sidecar shim image #11272

Conversation

What this PR does

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

Special notes for your reviewer

Checklist

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment