kimchi-project / kimchi

An HTML5 management interface for KVM guests
https://github.com/kimchi-project/kimchi/releases/latest
Other
3.12k stars 364 forks source link

Cannot attach existing (but empty) logical volume to VM #1222

Closed a-a closed 5 years ago

a-a commented 6 years ago

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. https://github.com/kimchi-project/kimchi/blob/205483244644c219c79cac366f562c774ea63108/isoinfo.py#L406-L415

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. https://github.com/kimchi-project/kimchi/blob/b67c319d2d1407027e7115c5ebeb6e91764d8d06/model/storagevolumes.py#L310-L320

Further on from there, we have the following checks: https://github.com/kimchi-project/kimchi/blob/b67c319d2d1407027e7115c5ebeb6e91764d8d06/model/storagevolumes.py#L322-L333

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 commented 6 years ago

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 commented 6 years ago

Sent a PR since this is probably more useful than a random diff in comments. https://github.com/kimchi-project/kimchi/pull/1258

dumol commented 5 years ago

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