Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logical pool now can list many kinds of partitions #276

Closed
shaohef opened this issue Dec 13, 2013 · 19 comments
Closed

logical pool now can list many kinds of partitions #276

shaohef opened this issue Dec 13, 2013 · 19 comments
Assignees
Milestone

Comments

@shaohef
Copy link

shaohef commented Dec 13, 2013

Now it can list partitions that are mount on host and has file system on it.

Even user is the owner of the host, he may not know well the some purpose of partitions.

This is not so safe for user.

At least it should not expose the partitions has already mounted on host, and it should not expose the partitions has file system on it.
Even this, some partitions(no mount and no file system) can also be already used for some special purpose.

Or should we disable this feature in UI for 1.1 release?

@sming56
Copy link

sming56 commented Dec 13, 2013

Ennh, it is dangerous to export the disk already used by the system to Kimchi user. Kimchi user can use that disk to create storage pool and cause data corruption. Too bad.

@ghost ghost assigned zhoumeina Dec 13, 2013
@sming56
Copy link

sming56 commented Dec 16, 2013

I agree to remove this patch out for safety.

@alinefm
Copy link
Member

alinefm commented Dec 16, 2013

About mounted disks/partitions: we should not expose them to user to create a logical pool

All the unmounted disks/partitions should be exposed but we should add a confirmation window to the user.
"This operation can cause data loss in the selected disks. Do you want to continue?"

Also lvm partitions should not be displayed as they are already part of a volume group

@alinefm
Copy link
Member

alinefm commented Dec 16, 2013

I will work on backend fixes.

@zhoumeina could you add the confirmation window to logical pool?

@sming56
Copy link

sming56 commented Dec 16, 2013

@alinefm In reality, it is pretty for a user to know if the disk is in used or not by listing the disks/partitions only. We should add hints to user as possible as we can. Like, "it is a disk with NTFS file system" and so on.

@shaohef
Copy link
Author

shaohef commented Dec 16, 2013

@alinefm
lvm partitions still can be a part of a new volume group, but I think kimchi should not allow this.
All the unmounted disks/partitions should be exposed, this will be not safe.
Such as if we install a windows XP, and when we start our linux distros, usually the windows partitions are unmounted, but they has fs on these partitions.

So should the unmounted disks/partitions with fs be exposed?

@wudx05 and @edwardbadboy some suggestions?

to @alinefm, @edwardbadboy is good at storage management. . I think it is easy for him to work on backend fixes. You can let him do it.

@alinefm
Copy link
Member

alinefm commented Dec 16, 2013

@shaohef @sming56 so let's expose only raw disk/partitions - unmounted and without fs.

@shaohef I am OK to let @edwardbadboy work on this but it must be ready to be merged by 1.1 release (this Wed)

@shaohef
Copy link
Author

shaohef commented Dec 17, 2013

For disk, on my RHEL 6.5, kimchi partitions API do no list disks.

Not sure a disk without partitions should be exposed.

$ sudo lsblk -Pbo TYPE,FSTYPE,SIZE,MOUNTPOINT /dev/vdb
TYPE="disk" FSTYPE="ext4" SIZE="21474836480" MOUNTPOINT=""

$curl -H 'Content-type: application/json' -H 'Accept: application/json' -X GET localhost:8000/host/partitions/vdb
{
"reason":"Not found: 'Partition vdb not found in the host'",
"code":"404 Not Found",
"call_stack":"Traceback (most recent call last):\n File "/usr/lib/python2.6/site-packages/cherrypy/_cprequest.py", line 656, in respond\n response.body = self.handler()\n File "/usr/lib/python2.6/site-packages/cherrypy/lib/encoding.py", line 188, in call\n self.body = self.oldhandler(_args, *_kwargs)\n File "/usr/lib/python2.6/site-packages/cherrypy/_cpdispatch.py", line 34, in call\n return self.callable(_self.args, *_self.kwargs)\n File "/home/user/work/kimchi/src/kimchi/controller.py", line 182, in index\n raise cherrypy.HTTPError(404, "Not found: '%s'" % msg)\nHTTPError: (404, "Not found: 'Partition vdb not found in the host'")\n"
}

on my host, I can list two disks vda and vdb. For vda disk, I think it is OK that we do not list it.

$lsblk
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
vda 252:0 0 20G 0 disk
├─vda1 252:1 0 500M 0 part /boot
└─vda2 252:2 0 19.5G 0 part
vdb 252:16 0 20G 0 disk

@zhoumeina
Copy link

On 12/16/2013 10:09 PM, alinefm wrote:

I will work on backend fixes.

@zhoumeina https://github.com/zhoumeina could you add the
confirmation window to logical pool?


Reply to this email directly or view it on GitHub
#276 (comment).

ok, I'll add a confirm box first.

@shaohef
Copy link
Author

shaohef commented Dec 17, 2013

on my RHEL 6.5 /dev/vda2 is a pv, should not display.
$ sudo pvscan
PV /dev/vda2 VG VolGroup lvm2 [19.51 GiB / 0 free]
Total: 1 [19.51 GiB] / in use: 1 [19.51 GiB] / in no VG: 0 [0 ]

$ curl -H 'Content-type: application/json' -H 'Accept: application/json' -X GET localhost:8000/host/partitions
[
{
"name":"vda1",
"fstype":"ext4",
"path":"/dev/vda1",
"mountpoint":"/boot",
"type":"part",
"size":"524288000"
},
{
"name":"vda2",
"fstype":"LVM2_member",
"path":"/dev/vda2",
"mountpoint":"",
"type":"part",
"size":"20949499904"
},
{
"name":"VolGroup-lv_root",
"fstype":"ext4",
"path":"/dev/mapper/VolGroup-lv_root",
"mountpoint":"/",
"type":"lvm",
"size":"18832424960"
},
{
"name":"VolGroup-lv_swap",
"fstype":"swap",
"path":"/dev/mapper/VolGroup-lv_swap",
"mountpoint":"",
"type":"lvm",
"size":"2113929216"
}
]

@shaohef
Copy link
Author

shaohef commented Dec 17, 2013

for mpath device, there's some wrong with lsblk
$ sudo lsblk /dev/dm-0
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
35000c5002eaa8392 (dm-0) 253:0 0 465.8G 0 mpath
├─35000c5002eaa8392p1 (dm-1) 253:1 0 50G 0 part
├─35000c5002eaa8392p2 (dm-2) 253:2 0 1K 0 part
├─35000c5002eaa8392p3 (dm-3) 253:3 0 37.3G 0 part /
├─35000c5002eaa8392p4 (dm-4) 253:4 0 185.5G 0 part /home
├─35000c5002eaa8392p5 (dm-5) 253:5 0 50G 0 part
├─35000c5002eaa8392p6 (dm-6) 253:6 0 100G 0 part
├─35000c5002eaa8392p7 (dm-7) 253:7 0 5.7G 0 part [SWAP]
└─35000c5002eaa8392p8 (dm-8) 253:8 0 37.3G 0 part
[shhfeng@fsh-thinkpad-t410 kimchi]$ sudo lsblk /dev/dm-1
lsblk: dm-1: unknown device name

@edwardbadboy
Copy link
Member

As a summary I'd propose the following solution.

List all unused partitions and disks, which means we ignore LVM (PV and LV ), mounted partitions and partitions with filesystem.

Special Case 1: "blkid -o device" does not list raw disks.
This is not a problem. I tested in Fedora 19, if the disk contains partitions, it lists partitions instead of disk. If the disk contains no partitions, it lists the disk.

Special Case 2: "lsblk -Pbo TYPE,FSTYPE,SIZE,MOUNTPOINT /dev/xxx" fails on multipath devices.
We can check the return value of lsblk, if it's not zero, skip the current device and continue to check the next device.

Thanks Sheldon for testing the special cases.

@edwardbadboy
Copy link
Member

I'd submit a quick fix according to the above comment. In long term we should re-design this feature. A better solution is to list all devices regardless in use or not, and have the front end provide a check box "filter in use devices". Let the user decide if he wants to re-use a device or use a free device.

@shaohef
Copy link
Author

shaohef commented Dec 18, 2013

@edwardbadboy I have commented some issue above I have encountered.

@shaohef
Copy link
Author

shaohef commented Dec 18, 2013

@edwardbadboy, I agree your suggestion. we need a quick fix first.
@alinefm and @sming56, what about you?

@shaohef
Copy link
Author

shaohef commented Dec 18, 2013

for logical pool:
http://libvirt.org/storage.html#StorageBackendLogical
libvirt doc says:
This provides a pool based on an LVM volume group. For a pre-defined LVM volume group, simply providing the group name is sufficient, while to build a new group requires providing a list of source devices to serve as physical volumes. Volumes will be allocated by carving out chunks of storage from the volume group.

now the kimchi code always build the VG

@edwardbadboy
Copy link
Member

I think detecting existing VG deserves another patch.

alinefm pushed a commit that referenced this issue Dec 18, 2013
…ack-end

The current logical pool implementation has some problems. It lists ext4
patitions but in fact partitions with FS should not be listed, because
a dual boot system setup can make use of those unmounted partitions. It
also fails to run lsblk for some devices.

According to the discussion on github issue page[1]. We decide to list
only unused partitions and disks, which means, PV and LV will not be
listed, mounted partitions and partition with FS will not be listed.

When lsblk fails to run on a purticular device, it swallows the error
and return an empty dict.

Some kinds of free block device still can not be listed by the current
quick fix, for this is only a *quick* fix. The following cases are not
covered, they should be improved in future patches.
1. PV that doesn't belong to any VG.
  blkid and lsblk do not provide enough information to detect if a PV
  belongs to an VG.

To get the device path, it uses sysfs uevent file to get the device name and
then assume the device path is /dev/<devname>

This patch also drops the defensive coding style of the current
implementation. Defensive coding is not Pythonic. It makes the code
harder to debug, delay the discovery of code error and makes the main
logic unclear. This patch also extract a function to run and parse
lsblk to avoid repeating similar code.

For a long term solution, backend should just list all block devices,
along with information describing if they are in use. The front end
provides a checkbox "filter in use devices" and let users decide what
they want.

[1] #276

Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com>
Signed-off-by: Aline Manera <alinefm@br.ibm.com>
alinefm pushed a commit that referenced this issue Dec 18, 2013
…ront-end

The current front-end implementation filter out disks from the free
block device listings. This is not needed after the previous back-end
pat correctly listing free disks.

Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com>
@shaohef
Copy link
Author

shaohef commented Dec 19, 2013

@edwardbadboy:
I can get the /dev/sda disk on my host, but it is not safe to use this disk.
/dev/sda is Multipath disk.
and libvirt supports Multipath pool.
http://libvirt.org/storage.html#StorageBackendMultipath

you see lsblk can list the tree of disk. so I think it is better to find the relationship of the disk and its partitions.
It is helpful to remove the sda.
now in the code:
if dev['type'] == 'part':
ignore_names.append(name[:-1])
If we make 10 partitions on sda, sda1-sda10
In [5]: 'sad10'[:-1]
Out[5]: 'sad1'

$ curl -u : -H 'Content-type: application/json' -H 'Accept: application/json' -X ET localhost:8000/host/partitions
[
{
"name":"sda",
"fstype":"",
"path":"/dev/sda",
"mountpoint":"",
"type":"disk",
"size":"500107862016"
},
{}
]

$ sudo lsblk
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sda 8:0 0 465.8G 0 disk
└─35000c5002eaa8392 (dm-0) 253:0 0 465.8G 0 mpath
├─35000c5002eaa8392p1 (dm-1) 253:1 0 50G 0 part
├─35000c5002eaa8392p2 (dm-2) 253:2 0 1K 0 part
├─35000c5002eaa8392p3 (dm-3) 253:3 0 37.3G 0 part /

@ghost ghost assigned edwardbadboy Dec 19, 2013
@shaohef
Copy link
Author

shaohef commented Feb 24, 2014

c56582e, 2553e21, bf6d929 and 902d295 are merged.

@shaohef shaohef closed this as completed Feb 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants