Move host-disk to handler #6240

xpivarc · 2021-08-12T13:42:15Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

This change is noop because func ReplacePVCByHostDisk is executed from handler context but the file path is relevant to launcher context. Therefore the change is failing 100% time and we are ignoring it. Also this is side-effect of the function. Signed-off-by: L. Pivarc <lpivarc@redhat.com>

We create disk only on one node. NFS server needs to be scheduled on this node. Signed-off-by: L. Pivarc <lpivarc@redhat.com>

Creating a disk in launcher requires the PVC to be correctly configurated. This will allow us to provide better user experience even with non-root launcher. Signed-off-by: L. Pivarc <lpivarc@redhat.com>

xpivarc · 2021-08-12T17:23:05Z

/cc @davidvossel
This is the thing we talked about. Let me know what you think.

davidvossel

why is moving all of this host-disk logic to handler better than having virt-handler simply set permissions on disks before performing the sync with virt-launcher?

davidvossel · 2021-08-12T20:40:51Z

pkg/virt-launcher/virtwrap/manager.go

-	hostDiskCreator := hostdisk.NewHostDiskCreator(l.notifier, l.lessPVCSpaceToleration, l.minimumPVCReserveBytes)
-	err = hostDiskCreator.Create(vmi)
-	if err != nil {
-		return domain, fmt.Errorf("preparing host-disks failed: %v", err)
-	}


if lessPVCSpaceToleration and minimumPVCReserveBytes are only used here, then there's a whole chain of things starting with template.go that can be removed related to these vars.

xpivarc · 2021-08-16T09:01:06Z

why is moving all of this host-disk logic to handler better than having virt-handler simply set permissions on disks before performing the sync with virt-launcher?

We would need to set permissions for disk(if exists) and for empty pvc (directory) because disks are created inside sync and not before. For me, it feels like additional code. Moving host-disk to the handler is simpler and it also allows us to drop the dependency from the launcher. This is good as the launcher has fewer privileges now.

davidvossel · 2021-08-18T21:24:05Z

We would need to set permissions for disk(if exists) and for empty pvc (directory) because disks are created inside sync and not before

ah okay, so we potentially have to deal with the case where a disk doesn't exist as well. I think this makes sense to move to handler then

Signed-off-by: L. Pivarc <lpivarc@redhat.com>

davidvossel

/lgtm
/approve

kubevirt-bot · 2021-08-19T21:43:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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:

~~OWNERS~~ [davidvossel]

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

xpivarc · 2021-08-20T07:55:51Z

/retest

xpivarc · 2021-08-20T12:02:48Z

/retest

xpivarc · 2021-08-20T20:21:28Z

/retest

kubevirt-commenter-bot · 2021-08-21T04:46:50Z

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

kubevirt-commenter-bot · 2021-08-21T12:46:50Z

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

xpivarc · 2021-08-21T19:14:28Z

/retest

kubevirt-commenter-bot · 2021-08-22T04:46:52Z

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

kubevirt-commenter-bot · 2021-08-22T13:46:52Z

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

enp0s3 · 2021-08-22T20:43:19Z

/hold
lets wait for #6280 to get in. We have a an issue with a sig-network macvtap test

xpivarc · 2021-08-23T06:03:06Z

/hold cancel

xpivarc · 2021-08-23T06:03:16Z

/retest

kubevirt-commenter-bot · 2021-08-23T16:48:01Z

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

kubevirt-bot · 2021-08-23T17:33:59Z

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

Test name	Commit	Details	Rerun command
pull-kubevirt-check-tests-for-flakes	`a6de356`	link	`/test pull-kubevirt-check-tests-for-flakes`

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.

xpivarc · 2021-09-06T11:19:22Z

/cherrypick release-0.44

kubevirt-bot · 2021-09-06T11:19:59Z

@xpivarc: #6240 failed to apply on top of branch "release-0.44":

Applying: Remove noop file ownership change
Applying: Fix test with NFS disk not owned by qemu
Applying: Use host-disk creator in handler
Using index info to reconstruct a base tree...
M	pkg/host-disk/host-disk_test.go
M	pkg/virt-handler/vm.go
M	pkg/virt-launcher/virtwrap/BUILD.bazel
M	pkg/virt-launcher/virtwrap/manager.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-launcher/virtwrap/manager.go
Auto-merging pkg/virt-launcher/virtwrap/BUILD.bazel
Auto-merging pkg/virt-handler/vm.go
Auto-merging pkg/host-disk/host-disk_test.go
CONFLICT (content): Merge conflict in pkg/host-disk/host-disk_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Use host-disk creator in handler
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:

/cherrypick release-0.44

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.

…ndler Move host-disk to handler (cherry picked from commit c5ded0d)

…ndler Move host-disk to handler (cherry picked from commit c5ded0d) Signed-off-by: L. Pivarc <lpivarc@redhat.com>

xpivarc added 3 commits August 12, 2021 15:10

Fix test with NFS disk not owned by qemu

684feb1

We create disk only on one node. NFS server needs to be scheduled on this node. Signed-off-by: L. Pivarc <lpivarc@redhat.com>

Use host-disk creator in handler

47cce0c

Creating a disk in launcher requires the PVC to be correctly configurated. This will allow us to provide better user experience even with non-root launcher. Signed-off-by: L. Pivarc <lpivarc@redhat.com>

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Aug 12, 2021

kubevirt-bot requested review from AlonaKaplan and ILpinto August 12, 2021 13:42

xpivarc mentioned this pull request Aug 12, 2021

Bug - Remove noop file ownership change #6217

Closed

kubevirt-bot requested a review from davidvossel August 12, 2021 17:23

davidvossel reviewed Aug 12, 2021

View reviewed changes

Remove unused parameters from launcher

a6de356

Signed-off-by: L. Pivarc <lpivarc@redhat.com>

xpivarc force-pushed the nr-move-host-disk-to-handler branch from ede47f0 to a6de356 Compare August 19, 2021 16:14

davidvossel approved these changes Aug 19, 2021

View reviewed changes

kubevirt-bot assigned davidvossel Aug 19, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2021

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2021

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 22, 2021

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2021

kubevirt-bot merged commit c5ded0d into kubevirt:main Aug 23, 2021

xpivarc mentioned this pull request Sep 8, 2021

[Release-0.44] Move host-disk to handler #6357

Merged

xpivarc pushed a commit to xpivarc/kubevirt that referenced this pull request Sep 8, 2021

Merge pull request kubevirt#6240 from xpivarc/nr-move-host-disk-to-ha…

c6d7239

…ndler Move host-disk to handler (cherry picked from commit c5ded0d)

xpivarc pushed a commit to xpivarc/kubevirt that referenced this pull request Sep 8, 2021

Merge pull request kubevirt#6240 from xpivarc/nr-move-host-disk-to-ha…

4217e70

…ndler Move host-disk to handler (cherry picked from commit c5ded0d) Signed-off-by: L. Pivarc <lpivarc@redhat.com>

xpivarc mentioned this pull request Oct 25, 2023

Remove usage of the deprecated NonRoot feature gate #10615

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move host-disk to handler #6240

Move host-disk to handler #6240

xpivarc commented Aug 12, 2021

xpivarc commented Aug 12, 2021

davidvossel left a comment

davidvossel Aug 12, 2021

xpivarc commented Aug 16, 2021

davidvossel commented Aug 18, 2021

davidvossel left a comment

kubevirt-bot commented Aug 19, 2021

xpivarc commented Aug 20, 2021

xpivarc commented Aug 20, 2021

xpivarc commented Aug 20, 2021

kubevirt-commenter-bot commented Aug 21, 2021

kubevirt-commenter-bot commented Aug 21, 2021

xpivarc commented Aug 21, 2021

kubevirt-commenter-bot commented Aug 22, 2021

kubevirt-commenter-bot commented Aug 22, 2021

enp0s3 commented Aug 22, 2021

xpivarc commented Aug 23, 2021

xpivarc commented Aug 23, 2021

kubevirt-commenter-bot commented Aug 23, 2021

kubevirt-bot commented Aug 23, 2021 •

edited

xpivarc commented Sep 6, 2021

kubevirt-bot commented Sep 6, 2021

Move host-disk to handler #6240

Move host-disk to handler #6240

Conversation

xpivarc commented Aug 12, 2021

xpivarc commented Aug 12, 2021

davidvossel left a comment

Choose a reason for hiding this comment

davidvossel Aug 12, 2021

Choose a reason for hiding this comment

xpivarc commented Aug 16, 2021

davidvossel commented Aug 18, 2021

davidvossel left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Aug 19, 2021

xpivarc commented Aug 20, 2021

xpivarc commented Aug 20, 2021

xpivarc commented Aug 20, 2021

kubevirt-commenter-bot commented Aug 21, 2021

kubevirt-commenter-bot commented Aug 21, 2021

xpivarc commented Aug 21, 2021

kubevirt-commenter-bot commented Aug 22, 2021

kubevirt-commenter-bot commented Aug 22, 2021

enp0s3 commented Aug 22, 2021

xpivarc commented Aug 23, 2021

xpivarc commented Aug 23, 2021

kubevirt-commenter-bot commented Aug 23, 2021

kubevirt-bot commented Aug 23, 2021 • edited

xpivarc commented Sep 6, 2021

kubevirt-bot commented Sep 6, 2021

kubevirt-bot commented Aug 23, 2021 •

edited