Use --data-dev or --data-dir for ceph-disk parameters to avoid making di...

[osynge]

ceph-disk parameters "--data-dev" or "--data-dir" are useful to avoid making directories such as /dev/sdc by mistake when you want to use a device that does not exist.

[ceph-jenkins]

Can one of the admins OK this pull request so I can run a build?

[alfredodeza]

OK to test

[alfredodeza]

this doesn't look like a robust way to detect if a path is a directory or not. Wouldn't it cause false positives?

[osynge]

You might be right. I don’t see any way we could specify relative paths with ceph-deploy. Maybe I am wrong?

I thought the format was:

ceph-deploy osd prepare $NODE:$DISK

where $DISK=="sdb" or similar for block devices.

where $DISK=="/mnt/sdc" or similar for directory.

What I don’t want is "ceph-disk" to make a directory "/dev/sdf" which it currently does. We could invert add to the logic and assume that if the path starts "/dev/" its a block device.

[alfredodeza]

I believe the you can still specify $DISK as /dev/sdb and ceph-disk will detect if it is a device or not, falling back to creating a directory if that is the case

[alfredodeza]

The one way to do this might be to check remotely if string starts with / and if that isn't a directory. For devices, python will report correctly, saying it is not a directory:

$ python
Python 2.7.3 (default, Sep 26 2013, 20:03:06)
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.isdir('/dev/sda')
False
>>> os.path.isdir('/dev/sda1')
False
>>> os.path.isdir('/tmp')
True

[osynge]

@alfredodeza you where quiet right it was producing lots of false positives. On retesting I have no idea how it ever passed my testing. This latest change has been retested and now works better.

[osynge]

@alfredodeza now added some unit tests, with one commented to expose limitations of path sanitisation.

[alfredodeza]

I suspect that it will be highly unlikely to have a device in any other place than /dev/ but it still seems that this might not be robust enough.

ceph-disk does a thorough check for devices/partitions, I am not saying we should implement it exactly like this but just to give you an idea of what we should be looking for (as opposed to matching a string of a path):

dev = os.path.realpath(dev)
if not stat.S_ISBLK(os.lstat(dev).st_mode):
    raise Error('not a block device', dev)

name = get_dev_name(dev)
if os.path.exists(os.path.join('/sys/block', name)):
    return False

# make sure it is a partition of something else
for basename in os.listdir('/sys/block'):
    if os.path.exists(os.path.join('/sys/block', basename, name)):
        return True

raise Error('not a disk or partition', dev)

[osynge]

Can you please clarify a little more?

Are you saying you want to support devices in directories other than "/dev/" with "ceph-deploy"? Would they just exist under "/proc/devices"?

Are you saying you want to support directories under "/dev/" with "ceph-deploy"? Removing this mistake is the main reason for this patch.

This command is calling ceph-disk with the parameters --data-dev or --data-dir. With these parameters ceph-disk does a thorough check for devices/partitions, without them ceph-disk just creates a directory if the file is not found. Since these checks are being done by ceph-disk which this function calls, and a clear error is presented to the user it seems redundant to check twice.

Maybe better it would be better if we followed the same policy as with "ceph-disk" and have override options so you can create directories under "/dev/" if users want and they can have devices under paths other than "/dev/". And only use the assumptions that people follow the LSB by default if these parameters are not set?

I personally think the duplicate error checking is redundant, and would prefer an override option. I suspect I am misunderstanding. Let me know what you think please?

[alfredodeza]

ceph-disk is no longer a part of ceph