Cannot attach existing (but empty) logical volume to VM

[a-a]

I have a storage pool of type "LOGICAL" defined, named "rust".
I added a logical volume "sms0" to this pool manually, and confirmed it's existence as follows:

makoto@vmh0-gld1:~$ sudo lvcreate -L 55GB -n sms0 rust
  Logical volume "sms0" created.
makoto@vmh0-gld1:~$ sudo lvs
  LV                                         VG   Attr       LSize  Pool Origin Data%  Meta%  Move Log Cpy%Sync Convert
  c1e1d541-c3ef-46a2-8b81-14bf75907964-0.img rust -wi-a----- 30.00g
  sms0                                       rust -wi-a----- 55.00g

In the above output I also have "c1e1d541-c3ef-46a2-8b81-14bf75907964-0.img" which was successfully added automatically by kimchi when creating a VM from a template, so at least something is working correctly.

From the web interface, I edit an existing VM with no volumes currently attached to it.
This is via Storage > Add
I change type from cdrom, to disk, select "Select an existing disk", and change the storage pool from "ISO" to my Physical Volume named "rust", however no Storage Volumes become visible and the Storage Volumes dropdown remains greyed out.

At the point of clicking on the new storage pool, the following two lines are written out to the log:

KCHISO0006E: Unexpected volume type for primary volume in ISO /dev/rust/c1e1d541-c3ef-46a2-8b81-14bf75907964-0.img
KCHISO0006E: Unexpected volume type for primary volume in ISO /dev/rust/sms0

The source of such an error appears to be within isoinfo.py - but I would ask why we are treating a logical volume as an ISO, this does not seem correct, but actually may be a red herring in this case.

kimchi/isoinfo.py

Lines 406 to 415 in 2054832

	def _scan_primary_vol(self, data):
	"""
	Scan one sector for a Primary Volume Descriptor and extract the
	Volume ID from the table
	"""
	primary_vol_data = data[0: -1]
	info = self._unpack(IsoImage.VOL_DESC, primary_vol_data)
	(vd_type, vd_ident, vd_ver, pad0, sys_id, vol_id) = info
	if vd_type != 1:
	raise IsoFormatError("KCHISO0006E", {'filename': self.path})

Digging slightly deeper, looking at the http requests, I see the following request:
https://10.100.100.190:8001/plugins/kimchi/storagepools/rust/storagevolumes

The response is as follows:

0 | {…}
-- | --
allocation | 32212254720
isvalid | false
capacity | 32212254720
name | c1e1d541-c3ef-46a2-8b81-14bf75907964-0.img
format | raw
has_permission | false
path | /dev/rust/c1e1d541-c3ef-46a2-8b81-14bf75907964-0.img
used_by | […]
0 | test
type | block
1 | {…}
allocation | 59055800320
isvalid | false
capacity | 59055800320
name | sms0
format | raw
has_permission | true
path | /dev/rust/sms0
used_by | []
type | block

Looking at storagevolumes.py - I see we try to see if a raw device is actually an ISO, which is perhaps the stage where the ISO error warnings in my error log occur.

kimchi/model/storagevolumes.py

Lines 310 to 320 in b67c319

	# 'raw' volumes from 'logical' pools may actually be 'iso';
	# libvirt always reports them as 'raw'
	pool_info = self.storagepool.lookup(pool)
	if pool_info['type'] == 'logical' and fmt == 'raw':
	try:
	iso_img = IsoImage(path)
	except IsoFormatError:
	# not 'iso' afterall
	pass
	else:
	fmt = 'iso'

Further on from there, we have the following checks:

kimchi/model/storagevolumes.py

Lines 322 to 333 in b67c319

	# 'raw' volumes can not be valid image disks (e.g. XML, PDF, TXT are
	# raw files), so it's necessary check the 'content' of them
	isvalid = True
	if fmt == 'raw':
	try:
	ms = magic.open(magic.NONE)
	ms.load()
	if ms.file(path).lower() not in VALID_RAW_CONTENT:
	isvalid = False
	ms.close()
	except UnicodeDecodeError:
	isvalid = False

It should be roughly around here where the checks are happening, and the disk is set isvalid == false, even though it may actually be valid as a raw block device.

I'll add some extra debug around here and see if I can catch what is being tripped up on, and try to formulate an appropriate patch if I can work it out.

[a-a]

What is happening is we run ms.file(path).lower() against a list of known file types.
Trying it against my /dev/rust/sms0 lv, it returns "symbolic link to ../dm-1" which is not inside VALID_RAW_CONTENT.

I have a rather crude fix, as below, which allows me to now attach the raw block devices for the logical volumes to my VMs. There are probably better checks to do, but for now, this can let me attach them to my VMs :)

makoto@vmh0-gld1:~/kimchi$ git diff
diff --git a/model/storagevolumes.py b/model/storagevolumes.py
index aea2760b..0bdbe998 100644
--- a/model/storagevolumes.py
+++ b/model/storagevolumes.py
@@ -323,14 +323,23 @@ class StorageVolumeModel(object):
         # raw files), so it's necessary check the 'content' of them
         isvalid = True
         if fmt == 'raw':
-            try:
-                ms = magic.open(magic.NONE)
-                ms.load()
-                if ms.file(path).lower() not in VALID_RAW_CONTENT:
+            if not os.path.islink(path): # Check if file is a symlink to a real block device, if so, don't check it's contents for validity
+                try:
+                    ms = magic.open(magic.NONE)
+                    ms.load()
+                    if ms.file(path).lower() not in VALID_RAW_CONTENT:
+                        isvalid = False
+                    ms.close()
+                except UnicodeDecodeError:
                     isvalid = False
-                ms.close()
-            except UnicodeDecodeError:
-                isvalid = False
+            else: # We are a symlink
+                 if "/dev/dm-" in os.path.realpath(path):
+                     # This is most likely a real blockdevice
+                     isvalid = True
+                     wok_log.error("symlink detected, validated the disk")
+                 else:
+                     # Doesn't point to a known blockdevice
+                     isvalid = False

         used_by = get_disk_used_by(self.conn, path)
         if (self.libvirt_user is None):
makoto@vmh0-gld1:~/kimchi$

[a-a]

Sent a PR since this is probably more useful than a random diff in comments.
#1258

[dumol]

PR diff fixes it for me too. Thanks @a-a!