Add WithLoopDevices to report loop devices #310

Itxaka · 2022-03-21T13:12:44Z

Currently we skipp over all loop devices in the system and with no
possibility of reporting those.

This patch introduces a new option to Block that allows us to report
loop devices as disk and their partitions like if they were normal
disks.

This patch does not modify any existing functionality or alters anything
with the default values.

With WithLoopDevices enabled this reports the same info for loop devices
as with other disks, including size, fs, mountpoints and so on.

Signed-off-by: Itxaka [email protected]

Itxaka · 2022-03-24T13:54:48Z

@fromanirh any comments on this? thanks!

ffromani · 2022-03-24T14:02:20Z

thanks @Itxaka for your contribution! I'll review today/tomorrow

jaypipes · 2022-03-24T14:13:43Z

@Itxaka what is the use case for a go-hardware library tracking loopback devices? :)

Itxaka · 2022-03-24T14:25:54Z

well, we could make the same question about returning mount points and filesystems no? Those are not hardware related at all, more like system status, so thinking in terms of ghw providing info about the current system in which its running, I want to see all the mountpoints, including those from loop devices which are mounted. I.E. I want to scan all mountpoints in the system to do something with them, report of scan or whatever, loop devices mounted are part of that system.

Thats the best I can do to sell this feature LOL :P

If you want a specific use case, we use loop devices in our installer[0] to test real installs, not really to a real filesystem, but to a loop device so we dont break any real disks, so we do some partition discovery and mounting. We currently use lsblk directly, but it would be much nicer to use a pure golang lib instead of depending on lsblk. Having loop mounts discovery in ghw helps us :)

[0] https://github.com/rancher-sandbox/elemental

jaypipes · 2022-03-24T15:12:11Z

well, we could make the same question about returning mount points and filesystems no? Those are not hardware related at all, more like system status, so thinking in terms of ghw providing info about the current system in which its running, I want to see all the mountpoints, including those from loop devices which are mounted. I.E. I want to scan all mountpoints in the system to do something with them, report of scan or whatever, loop devices mounted are part of that system.

Thats the best I can do to sell this feature LOL :P

If you want a specific use case, we use loop devices in our installer[0] to test real installs, not really to a real filesystem, but to a loop device so we dont break any real disks, so we do some partition discovery and mounting. We currently use lsblk directly, but it would be much nicer to use a pure golang lib instead of depending on lsblk. Having loop mounts discovery in ghw helps us :)

[0] https://github.com/rancher-sandbox/elemental

Fair enough. You've sold me on your use case.

I'll do a full review of the PR shortly.

Currently we skipp over all loop devices in the system and with no possibility of reporting those. This patch introduces a new option to Block that allows us to report loop devices as disk and their partitions like if they were normal disks. This patch does not modify any existing functionality or alters anything with the default values. With WithLoopDevices enabled this reports the same info for loop devices as with other disks, including size, fs, mountpoints and so on. Signed-off-by: Itxaka <[email protected]>

Itxaka · 2022-03-30T08:28:24Z

any update on this @jaypipes ?

Signed-off-by: Itxaka <[email protected]>

jaypipes · 2022-03-30T16:02:13Z

pkg/block/block_linux.go

+		if driveType == DRIVE_TYPE_LOOP && size == 0 {
+			// Do nothing for empty loop devices
+			// There can be several loop devices present on /dev but that doesnt mean they are in use
+			// With this we skip any non-used loop devices
+		} else {


I would prefer to just return all the loop devices that are listed by the OS, regardless of their size. Calling programs can filter out returned results any way they want.

Thing is, if you mount something on a loop device, several loop devices will appear in /dev, even if not mounted. This was a kind fo fix to report only those loop devices in use.

$ sudo losetup -f --show cOS-Seed-green-0.8.8-g8522da85-x86_64.iso /dev/loop0 $ ls /dev/loop* /dev/loop0 /dev/loop2 /dev/loop4 /dev/loop6 /dev/loop-control /dev/loop1 /dev/loop3 /dev/loop5 /dev/loop7 $ sudo blkid |grep loop /dev/loop0: BLOCK_SIZE="2048" UUID="2022-03-23-20-47-00-00" LABEL="COS_LIVE" TYPE="iso9660" PTTYPE="dos"

So in reality, in this situation, only the loop0 device is being used, the rest are there for some reason I guess, but not used. Returning unused loop devices seemed to me a waste of user time filtering those out as they are never gonna need them for anything.

Thoughts?

@Itxaka OK, I guess I can get behind that reasoning :)

Do me a favor though... in order to reduce the number of lines of code changed here, can you edit the above to this instead?

if storageController == STORAGE_CONTROLLER_LOOP && size == 0 { // We don't care about unused loop devices... continue }

and please place above the d := &Disk{...} definition line.

jaypipes · 2022-03-30T16:10:59Z

pkg/block/block.go

@@ -29,6 +29,7 @@ const (
 	DRIVE_TYPE_FDD               // Floppy disk drive
 	DRIVE_TYPE_ODD               // Optical disk drive
 	DRIVE_TYPE_SSD               // Solid-state drive
+	DRIVE_TYPE_LOOP              // loop device


I'm questioning whether "loop" should be a DRIVE_TYPE... is it really a type of drive? I'm not sure... maybe we should make a new DRIVE_TYPE_VIRTUAL or DRIVE_TYPE_PSEUDO.

DRIVE_TYPE_VIRTUAL sounds good to me. I wanted to return something for this field but not overcomplicate things. virtual seems perfect

OK, let's go with DRIVE_TYPE_VIRTUAL then, @Itxaka.

jaypipes · 2022-03-30T16:17:43Z

pkg/option/option.go

@@ -137,6 +137,9 @@ type Option struct {
 	// to learn about the system resources.
 	PathOverrides PathOverrides

+	// LoopDevices optionally allows reporting disk/partitions that are loop devices
+	LoopDevices *bool


I'm tempted to say let's get rid of this option and just always process loop devices, and let callers filter out any results the want... @fromanirh what do you think?

@Itxaka yeah, just remove this config option... if there is a huge demand for it, we can always add an option in the future.

Hey! I didn't had time to think this change (overall PR not this specific bit) deep enough. Will do next week. In general I tend to agree to add to the API surface gradually.

- Remove option to show loopdevices - Change storage/type to virtual - Simplify empty loop device skipping - Always report loop devices Signed-off-by: Itxaka <[email protected]>

Itxaka · 2022-04-01T09:55:41Z

umm, failing on ubuntu but I cant reproduce it locally. As that its picking up the underlaying os devices Im sure there is something wrong in the code..Will try to debug

Signed-off-by: Itxaka <[email protected]>

Itxaka · 2022-04-01T10:55:37Z

Found it, missing a value :D

jaypipes · 2022-04-03T16:24:58Z

pkg/block/block.go

@@ -93,6 +96,7 @@ const (
 	STORAGE_CONTROLLER_NVME                      // Non-volatile Memory Express
 	STORAGE_CONTROLLER_VIRTIO                    // Virtualized storage controller/driver
 	STORAGE_CONTROLLER_MMC                       // Multi-media controller (used for mobile phone storage devices)
+	STORAGE_CONTROLLER_VIRTUAL                   // virtual controller, i.e. for a loop device


Sorry @Itxaka I didn't make myself clear in the original review... I thought STORAGE_CONTROLLER_LOOP was good. It's only the DRIVE_TYPE_LOOP that I wanted to change to DRIVE_TYPE_VIRTUAL... Can we swtich the storage controller back please? :)

Set back the storage controller to identify loop devices and try to fix a potential problem with running the generic block tests in a host with loop devices. Signed-off-by: Itxaka <[email protected]>

jaypipes

Very nice work, @Itxaka thank you!

jaypipes · 2022-04-04T15:19:56Z

pkg/block/block_test.go

+	// Skip loop devices on generic tests as we don't know what the underlying system is going to have
+	// And loop devices don't have Serial Numbers for example.
+	for _, d := range disks {
+		if d.StorageController != block.STORAGE_CONTROLLER_LOOP {
+			d0 = d
+			break
+		}
+	}


jaypipes mentioned this pull request Mar 24, 2022

Properly detect dm devices #298

Open

Itxaka added 2 commits March 25, 2022 11:38

Merge branch 'main' into feature_report_loop_devices

c5ddb13

fix formatting

4592eeb

Signed-off-by: Itxaka <[email protected]>

Itxaka mentioned this pull request Mar 30, 2022

Drop lsblk in favour of ghw library rancher/elemental-cli#155

Merged

jaypipes reviewed Mar 30, 2022

View reviewed changes

Adress feedback

500a11a

- Remove option to show loopdevices - Change storage/type to virtual - Simplify empty loop device skipping - Always report loop devices Signed-off-by: Itxaka <[email protected]>

Itxaka requested review from jaypipes and ffromani April 1, 2022 09:49

Add missing DRIVE_TYPE_VIRTUAL

e75e68f

Signed-off-by: Itxaka <[email protected]>

jaypipes requested changes Apr 3, 2022

View reviewed changes

Fix store type back to loop and a possible test issue

7e3184b

Set back the storage controller to identify loop devices and try to fix a potential problem with running the generic block tests in a host with loop devices. Signed-off-by: Itxaka <[email protected]>

Itxaka requested a review from jaypipes April 4, 2022 07:31

jaypipes approved these changes Apr 4, 2022

View reviewed changes

jaypipes merged commit 2ea05cb into jaypipes:main Apr 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WithLoopDevices to report loop devices #310

Add WithLoopDevices to report loop devices #310

Itxaka commented Mar 21, 2022

Itxaka commented Mar 24, 2022

ffromani commented Mar 24, 2022

jaypipes commented Mar 24, 2022

Itxaka commented Mar 24, 2022

jaypipes commented Mar 24, 2022

Itxaka commented Mar 30, 2022

jaypipes Mar 30, 2022

Itxaka Mar 31, 2022

jaypipes Apr 1, 2022

jaypipes Mar 30, 2022

Itxaka Mar 31, 2022

jaypipes Apr 1, 2022

jaypipes Mar 30, 2022

jaypipes Apr 1, 2022

ffromani Apr 1, 2022

Itxaka commented Apr 1, 2022

Itxaka commented Apr 1, 2022

jaypipes Apr 3, 2022

Itxaka Apr 3, 2022

jaypipes left a comment

jaypipes Apr 4, 2022

Add WithLoopDevices to report loop devices #310

Add WithLoopDevices to report loop devices #310

Conversation

Itxaka commented Mar 21, 2022

Itxaka commented Mar 24, 2022

ffromani commented Mar 24, 2022

jaypipes commented Mar 24, 2022

Itxaka commented Mar 24, 2022

jaypipes commented Mar 24, 2022

Itxaka commented Mar 30, 2022

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

Itxaka commented Apr 1, 2022

Itxaka commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaypipes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment