Fix hostpath hotplug not working when volume on separate device #6308

awels · 2021-08-26T19:08:20Z

Signed-off-by: Alexander Wels awels@redhat.com

What this PR does / why we need it:
findmnt returns device[path relative in device] when looking up the
source of a mount by container pid. So on separate device that often
returned / which is not where it is mounted in the boot device.

This PR modifies the check to look up the device, and then the path
where the device is mounted, and combines the two paths to get the
path relative to the boot device so we can properly bind mount the
hostpath volume into another container.

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 #6303

Special notes for your reviewer:
I have made some modifications to disks-images-provider.yaml.in and the entrypoint.sh. In particular I am mounting the host root directory under /host, and then chrooting the losetup commands. This is needed because it will not allow one to create more than 1 loop back device in the container otherwise.

Release note:

BugFix: hotplug was broken when using it with a hostpath volume that was on a separate device.

awels · 2021-08-26T19:14:06Z

/cherrypick release-0.44

kubevirt-bot · 2021-08-26T19:14:15Z

@awels: once the present PR merges, I will cherry-pick it on top of release-0.44 in a new PR and assign it to you.

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.

awels · 2021-08-27T11:24:04Z

/retest

awels · 2021-08-27T15:57:27Z

/retest

brybacki · 2021-08-30T16:01:05Z

pkg/virt-handler/hotplug-disk/mount.go

+					}
+					// Check if the path was relative to the device.
+					if _, err := os.Stat(filepath.Join(util.HostRootMount, source)); err != nil {
+						return filepath.Join(deviceFindMnt[0].Target, source), nil


Why it only checks if hostRoot/source exists and does not check deviceFindMnt[0].Target/source.

Also this code is run when file not found or storage isBlock? Is the logic the same for both?

if _, err := os.Stat(filepath.Join(util.HostRootMount, source)); os.IsNotExist(err) || isBlock

Checks if the returned path doesn't exists, or if the returned path does exist, but is a block device (isBlock)
If either is true, then we call deviceFindMnt with that path. Basically figuring out where this path is mounted (if it is a block device for instance). If found return the target (mount path). If not found we try again but this time with the SourceDevice part of the findmnt information. The arguments passed to findmtn return 'source' in the following format:

/dev/device[/path/relative/to/device]

findmnt.GetSourceDevice() returns the /dev/device part
findmnt.GetSourcePath() returns /path/relative/to/device part

Once I get to

if _, err := os.Stat(filepath.Join(util.HostRootMount, source)); err != nil {

I know that deviceFindMnt[0].Target is the path the device is mounted on relative to the boot partition. And I know that source is the path relative to the device. So the two combined will be the path I am looking for. I can check if that path exists, but if it doesn't I have no recourse to do anything but return an error, and the caller of this function is going to try and do stuff with the path immeidately as well, and error if it doesn't exist then.

Thanks, this explains it. I just wonder if this can be simplified.

davidvossel · 2021-08-30T19:06:03Z

pkg/virt-handler/hotplug-disk/findmnt_test.go

+		test := FindmntInfo{
+			Source: "/dev/test",
+		}
+		Expect(test.GetSourceDevice()).To(Equal(""))


why wouldn't this test case return /dev/test ?

Because the regex is looking for .*[.+] so no [ ] is no match. This is because I am calling findmnt with arguments that should never return /dev/test for a filesystem mount.

jean-edouard

/approve
This looks ok to me, just a few comments in-line.
@davidvossel @brybacki have your concerns been addressed?

jean-edouard · 2021-08-31T18:43:06Z

images/disks-images-provider/entrypoint.sh

@@ -34,7 +34,8 @@ num=$(losetup -l | wc -l)
 [ ${num} -gt 100 ] && echo "attached loopdevices have reach limit number(100)" && exit 1

 # Put LOOP_DEVICE in /etc/bashrc in order to detach this loop device when the pod stopped.
-LOOP_DEVICE=$(losetup --find --show /local-storage/cirros.img.raw)
+LOOP_DEVICE=$(chroot /host losetup --find --show /mnt/local-storage/cirros.img.raw)
+echo $LOOP_DEVICE


Leftover debug print?

Yes and no, I can remove it, but I think it would be useful for people to see what loopback device was created.

jean-edouard · 2021-08-31T18:48:20Z

images/disks-images-provider/entrypoint.sh

+ls -al /local-storage/hp_file.img
+# Put LOOP_DEVICE_HP in /etc/bashrc in order to detach this loop device when the pod stopped.
+LOOP_DEVICE_HP=$(chroot /host losetup --verbose --find --show /mnt/local-storage/hp_file.img)
+echo $LOOP_DEVICE_HP


Leftover debug print?

Yes and no, I can remove it, but I think it would be useful for people to see what loopback device was created.

If you leave it I'd suggest making it a bit more verbose so people know what it's about!

I am not that passionate about it, I am removing it.

if people really want to know, you can look in /etc/bashrc to find out.

jean-edouard · 2021-08-31T18:56:31Z

pkg/virt-handler/hotplug-disk/findmnt.go

 	match := sourceRgx.FindStringSubmatch(f.Source)
 	if len(match) != 2 {
 		return strings.TrimPrefix(f.Source, rhcosPrefix)
 	}
 	return strings.TrimPrefix(match[1], rhcosPrefix)
 }

+func (f *FindmntInfo) GetSourceDevice() string {
+	match := deviceRgx.FindStringSubmatch(f.Source)
+	if len(match) == 0 {


Nit: we really want len(match) to be exactly 2, like in the function above.

I guess I can flip the logic to

if len(match) == 2 { return match[1] } return ""

kubevirt-bot · 2021-08-31T18:57:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jean-edouard

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~~ [jean-edouard]

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

findmnt returns device[path relative in device] when looking up the source of a mount by container pid. So on separate device that often returned /<path> which is not where it is mounted in the boot device. This PR modifies the check to look up the device, and then the path where the device is mounted, and combines the two paths to get the path relative to the boot device so we can properly bind mount the hostpath volume into another container. Signed-off-by: Alexander Wels <awels@redhat.com>

Signed-off-by: Alexander Wels <awels@redhat.com>

awels · 2021-09-01T11:27:27Z

/test pull-kubevirt-e2e-k8s-1.20-sig-compute

davidvossel · 2021-09-01T20:25:22Z

This looks ok to me, just a few comments in-line.
@davidvossel @brybacki have your concerns been addressed?

yep

brybacki · 2021-09-02T06:33:52Z

This looks ok to me, just a few comments in-line.
@davidvossel @brybacki have your concerns been addressed?

yep

Yes

jean-edouard · 2021-09-02T11:55:46Z

/lgtm

kubevirt-bot · 2021-09-02T16:26:10Z

@awels: new pull request created: #6344

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.

kubevirt-bot · 2021-09-02T19:13:01Z

@awels: 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-e2e-k8s-1.21-sig-compute	`25933f4`	link	`/test pull-kubevirt-e2e-k8s-1.21-sig-compute`

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-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 26, 2021

kubevirt-bot requested review from ILpinto and mhenriks August 26, 2021 19:08

kubevirt-bot added the size/L label Aug 26, 2021

awels force-pushed the hotplug_separate_device branch from 1526e96 to f0ed15d Compare August 27, 2021 12:53

awels force-pushed the hotplug_separate_device branch 2 times, most recently from 5dfe7fd to 01761e9 Compare August 27, 2021 23:48

brybacki reviewed Aug 30, 2021

View reviewed changes

davidvossel reviewed Aug 30, 2021

View reviewed changes

jean-edouard reviewed Aug 31, 2021

View reviewed changes

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

awels added 3 commits August 31, 2021 16:28

Fix disk images not able to create 2 loop back devices

08785da

Signed-off-by: Alexander Wels <awels@redhat.com>

Addressed review comments.

25933f4

Signed-off-by: Alexander Wels <awels@redhat.com>

awels force-pushed the hotplug_separate_device branch from 01761e9 to 25933f4 Compare August 31, 2021 20:28

kubevirt-bot assigned jean-edouard Sep 2, 2021

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

kubevirt-bot merged commit 78e64e1 into kubevirt:main Sep 2, 2021

kubevirt-bot mentioned this pull request Sep 2, 2021

[release-0.44] Fix hostpath hotplug not working when volume on separate device #6344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hostpath hotplug not working when volume on separate device #6308

Fix hostpath hotplug not working when volume on separate device #6308

awels commented Aug 26, 2021 •

edited

awels commented Aug 26, 2021

kubevirt-bot commented Aug 26, 2021

awels commented Aug 27, 2021

awels commented Aug 27, 2021

brybacki Aug 30, 2021

awels Aug 31, 2021

brybacki Sep 1, 2021

davidvossel Aug 30, 2021

awels Aug 30, 2021

jean-edouard left a comment

jean-edouard Aug 31, 2021

awels Aug 31, 2021

jean-edouard Aug 31, 2021

awels Aug 31, 2021

jean-edouard Aug 31, 2021

awels Aug 31, 2021

awels Aug 31, 2021

jean-edouard Aug 31, 2021

awels Aug 31, 2021

kubevirt-bot commented Aug 31, 2021

awels commented Sep 1, 2021

davidvossel commented Sep 1, 2021

brybacki commented Sep 2, 2021

jean-edouard commented Sep 2, 2021

kubevirt-bot commented Sep 2, 2021

kubevirt-bot commented Sep 2, 2021

Fix hostpath hotplug not working when volume on separate device #6308

Fix hostpath hotplug not working when volume on separate device #6308

Conversation

awels commented Aug 26, 2021 • edited

awels commented Aug 26, 2021

kubevirt-bot commented Aug 26, 2021

awels commented Aug 27, 2021

awels commented Aug 27, 2021

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

jean-edouard left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-bot commented Aug 31, 2021

awels commented Sep 1, 2021

davidvossel commented Sep 1, 2021

brybacki commented Sep 2, 2021

jean-edouard commented Sep 2, 2021

kubevirt-bot commented Sep 2, 2021

kubevirt-bot commented Sep 2, 2021

awels commented Aug 26, 2021 •

edited