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

ceph-disk/ceph_disk/main.py: Replace ST_ISBLK() test by is_diskdevice() #15587

Merged
merged 2 commits into from Jul 15, 2017

Conversation

Projects
None yet
4 participants
@wjwithagen
Contributor

wjwithagen commented Jun 8, 2017

  • FreeBSD does not have blockdevices any more (since 2002)
    So disk are just Character special devices, so test on ISCHR

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

@wjwithagen wjwithagen requested a review from Jun 8, 2017

self.type = self.DEVICE
else:
raise Error('not a dir or block device', self.args.data)
raise Error('not a dir or block/disk device', self.args.data)

This comment has been minimized.

@ghost

ghost Jun 8, 2017

"not a dir or device" ?

This comment has been minimized.

@wjwithagen

wjwithagen Jun 8, 2017

Contributor

@dachary
You mean: just kill the 'block/disk' part. Can do that...

This comment has been minimized.

@ghost
if not stat.S_ISBLK(mode):
raise Error('%s is not a block device' % path)
if not is_diskdevice(path):
raise Error('%s is not a block/disk device' % path)

This comment has been minimized.

@ghost

ghost Jun 8, 2017

"not a device"

if not stat.S_ISBLK(os.lstat(path).st_mode):
raise Error('not a block device', path)
if not is_diskdevice(path):
raise Error('not a block/disk device', path)

This comment has been minimized.

@ghost

ghost Jun 8, 2017

"not a device"

if not stat.S_ISBLK(os.lstat(path).st_mode):
raise Error('not a block device', path)
if not is_diskdevice(disk):
raise Error('not a block/disk device', path)

This comment has been minimized.

@ghost

ghost Jun 8, 2017

"not a device"

@@ -4677,8 +4681,8 @@ def unset_suppress(path):
disk = os.path.realpath(path)
if not os.path.exists(disk):
raise Error('does not exist', path)
if not stat.S_ISBLK(os.lstat(path).st_mode):
raise Error('not a block device', path)
if not is_diskdevice(disk):

This comment has been minimized.

@ghost

ghost Jun 8, 2017

s/disk/path/

@@ -665,6 +665,19 @@ def get_dev_size(dev, size='megabytes'):
os.close(fd)
def is_diskdevice

This comment has been minimized.

@ghost

ghost Jun 8, 2017

is_device ? I don't think we deal with anything else but devices that contain bytes

This comment has been minimized.

@wjwithagen

wjwithagen Jun 8, 2017

Contributor

@dachary
I'm open to suggestions for another function name. I though is_blockdevice but then that is exactly what it is not. It is the bare metal thingy ultimately, the diskdevice.

This comment has been minimized.

@wjwithagen

wjwithagen Jun 8, 2017

Contributor

@dachary
I also see things happening on files. That is certainly not ISBLK.
Also there are in Filestore directories on which to build the Ceph stuff.
Again not ISBLK I think?

This comment has been minimized.

@ghost

ghost Jun 9, 2017

Right. We either have files, directories or devices in the sense that they are in /dev or are special files.

@@ -665,6 +665,19 @@ def get_dev_size(dev, size='megabytes'):
os.close(fd)
def is_diskdevice
dev = os.path.realpath(dev)

This comment has been minimized.

@ghost

ghost Jun 8, 2017

this must reside outside of the function: it matters to the caller and makes a difference when it is gone

This comment has been minimized.

@wjwithagen

wjwithagen Jun 8, 2017

Contributor

@dachary
Does it really matter for this function? I mean, it is a real device, or a link to the device.
Do you have cases where it is a link to a device, but you still would like the answer to this function to be false? Could be, given my limited knowledge of all the variations on linux devices

This comment has been minimized.

@ghost

ghost Jun 9, 2017

It does matter. see below.

@ghost ghost added core feature labels Jun 9, 2017

@@ -1536,9 +1546,7 @@ def zap(dev):
"""
Destroy the partition table and content of a given disk.
"""
dev = os.path.realpath(dev)

This comment has been minimized.

@ghost

ghost Jun 9, 2017

You must keep dev here. It is used later in the function for display and processing. https://docs.python.org/2/library/os.path.html#os.path.realpath removes symbolic links. This is the part I'm worried about because sometime we want the symbolic link, not the device it points to.

os.path.realpath is used carefully and it is complicated to figure out exactly why and where. This is why it needs to stay exactly where it is and not be used more than it should.

This comment has been minimized.

@wjwithagen

wjwithagen Jun 9, 2017

Contributor

@dachary
Oke, I'll go over the PR with a fine comb, because I think I also killed af few stat() versus lstat(), and that might then also matter. I'll put back the leader part of the function in the code.
I thought that it was also a nice cleanup as extra bonus, but obviously not.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 12, 2017

@dachary
Ping?

@ghost

I think we can get rid of ldev. The rest looks good to merge !

@@ -777,8 +803,8 @@ def get_partition_base(dev):
Get the base device for a partition
"""
dev = os.path.realpath(dev)
if not stat.S_ISBLK(os.lstat(dev).st_mode):

This comment has been minimized.

@ghost

ghost Jun 12, 2017

the lstat is useless here: os.path.realpath will return the actual file, not the symlink, therefore dev will never be a symlink

@@ -4649,7 +4672,7 @@ def is_suppressed(path):
disk = os.path.realpath(path)
try:
if (not disk.startswith('/dev/') or
not stat.S_ISBLK(os.lstat(disk).st_mode)):

This comment has been minimized.

@ghost

ghost Jun 12, 2017

lstat useless here as well, stat will do the same because of the realpath above

@@ -4677,8 +4700,8 @@ def unset_suppress(path):
disk = os.path.realpath(path)
if not os.path.exists(disk):
raise Error('does not exist', path)
if not stat.S_ISBLK(os.lstat(path).st_mode):
raise Error('not a block device', path)
if not ldev_is_diskdevice(path):

This comment has been minimized.

@ghost

ghost Jun 12, 2017

I think we can change path to disk here, for consistency. I don't see any useful use case where it would matter to use path instead of disk. This is mostly likely what was intended originally.

This comment has been minimized.

@ghost

ghost Jun 12, 2017

and use dev_is_diskdevice

This comment has been minimized.

@wjwithagen

wjwithagen Jun 12, 2017

Contributor

@dachary
Wiil go over your careful review, and fix. Thanx ...

This comment has been minimized.

@wjwithagen

wjwithagen Jun 26, 2017

Contributor

@dachary
Hi Loic, could you take a look again, and merge if oke?

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 13, 2017

@dachary
Looks like the error is known: cephtool-test-mon.sh

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 15, 2017

@dachary
Ping?

@ghost

This comment has been minimized.

ghost commented Jun 15, 2017

jenkins test this please (master is fixed)

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 21, 2017

@dachary
Jenkins seems happy. Do you want to run it thru QA?

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jul 6, 2017

@yuriw
Sorry about the merge conflict. PRs went in, in the "wrong" order.
Should be fixed now.

@ghost

ghost approved these changes Jul 6, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 7, 2017

retest this please

@@ -205,7 +205,7 @@ function test_zap() {
local osd_data=$dir/dir
$mkdir -p $osd_data
${CEPH_DISK} $CEPH_DISK_ARGS zap $osd_data 2>&1 | grep -q 'not full block device' || return 1
${CEPH_DISK} $CEPH_DISK_ARGS zap $osd_data 2>&1 | grep -q 'not full device' || return 1

This comment has been minimized.

@tchaikov

tchaikov Jul 9, 2017

Contributor

@wjwithagen why did you change the grepped error message?

/home/jenkins-build/build/workspace/ceph-pull-requests/src/ceph-disk/tests/ceph-disk.sh:208: test_zap:  grep -q 'not full device'
/home/jenkins-build/build/workspace/ceph-pull-requests/src/ceph-disk/tests/ceph-disk.sh:208: test_zap:  /home/jenkins-build/build/workspace/ceph-pull-requests/src/ceph-disk/.tox/py27/bin/coverage run --append --source=ceph_disk -- /home/jenkins-build/build/workspace/ceph-pull-requests/src/ceph-disk/.tox/py27/bin/ceph-disk --verbose --prepend-to-path= --statedir=td/test-ceph-disk --sysconfdir=td/test-ceph-disk zap td/test-ceph-disk/dir
/home/jenkins-build/build/workspace/ceph-pull-requests/src/ceph-disk/tests/ceph-disk.sh:208: test_zap:  return 1

and it looks like "make check" failed due to this change, see https://jenkins.ceph.com/job/ceph-pull-requests/28056/consoleFull#-23068421240526d21-3511-427d-909c-dd086c0d1034.

This comment has been minimized.

@wjwithagen

wjwithagen Jul 11, 2017

Contributor

@tchaikov
I changed the grepped messages because the test no longer is only for a block device.
FreeBSD does not have block devices, so not to "confuse" user, and get useless questions, I thought it was wise to make it more generic. But perhaps I missed some tests.
And things could get worse when goint into theutology...
So perhaps it is easier to just change this back.

This comment has been minimized.

@tchaikov

tchaikov Jul 11, 2017

Contributor

if you believe that "block device" is not accurate or wrong on FreeBSD, probably you can update where the exception is raised. or better off conditionalize this on FreeBSD/Linux, so Linux user can have more specific warning message.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jul 11, 2017

@tchaikov
Reverted the error messages, lets see if this passes Jenkins and QA

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 11, 2017

retest this please.

@@ -819,7 +845,7 @@ def is_partition(dev):
dev = os.path.realpath(dev)
st = os.lstat(dev)
if not stat.S_ISBLK(st.st_mode):
if not stmode_is_diskdevice(st.mode):

This comment has been minimized.

@tchaikov

tchaikov Jul 14, 2017

Contributor

s/st.mode/st.st_mode/

2017-07-13T16:47:20.982 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk::sh: ceph-disk list --format json vda vdb vdc vdd
2017-07-13T16:47:21.068 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:Traceback (most recent call last):
2017-07-13T16:47:21.069 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:File "/usr/sbin/ceph-disk", line 9, in <module>
2017-07-13T16:47:21.069 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:load_entry_point('ceph-disk==1.0.0', 'console_scripts', 'ceph-disk')()
2017-07-13T16:47:21.069 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 5725, in run
2017-07-13T16:47:21.069 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:main(sys.argv[1:])
2017-07-13T16:47:21.069 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 5678, in main
2017-07-13T16:47:21.070 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:main_catch(args.func, args)
2017-07-13T16:47:21.070 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 5703, in main_catch
2017-07-13T16:47:21.070 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:func(args)
2017-07-13T16:47:21.070 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 4638, in main_list
2017-07-13T16:47:21.071 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:main_list_protected(args)
2017-07-13T16:47:21.071 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 4642, in main_list_protected
2017-07-13T16:47:21.071 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:devices = list_devices()
2017-07-13T16:47:21.071 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 4602, in list_devices
2017-07-13T16:47:21.072 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:device = list_dev(get_dev_path(base), uuid_map, space_map)
2017-07-13T16:47:21.073 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 4473, in list_dev
2017-07-13T16:47:21.073 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:info['is_partition'] = is_partition(dev)
2017-07-13T16:47:21.073 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 869, in is_partition
2017-07-13T16:47:21.073 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:if not stmode_is_diskdevice(st.mode):
2017-07-13T16:47:21.074 INFO:tasks.workunit.client.0.vpm073.stderr:DEBUG:CephDisk:AttributeError: 'posix.stat_result' object has no attribute 'mode'
2017-07-13T16:47:21.094 INFO:tasks.workunit.client.0.vpm073.stdout:../../../clone.client.0/qa/workunits/ceph-disk/ceph-disk-test.py:345: TestCephDisk.test_activate_dmcrypt_luks_wit
h_lockbox FAILED
@@ -777,7 +803,7 @@ def get_partition_base(dev):
Get the base device for a partition
"""
dev = os.path.realpath(dev)
if not stat.S_ISBLK(os.lstat(dev).st_mode):
if ldev_is_diskdevice(dev):

This comment has been minimized.

@tchaikov

tchaikov Jul 14, 2017

Contributor

should be

if not ldev_is_diskdevice(dev):
2017-07-14T04:09:31.086 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:Traceback (most recent call last):
2017-07-14T04:09:31.088 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:File "/usr/sbin/ceph-disk", line 9, in <module>
2017-07-14T04:09:31.091 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:load_entry_point('ceph-disk==1.0.0', 'console_scripts', 'ceph-disk')()
2017-07-14T04:09:31.113 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 5725, in run
2017-07-14T04:09:31.116 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:main(sys.argv[1:])
2017-07-14T04:09:31.125 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 5676, in main
2017-07-14T04:09:31.127 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:args.func(args)
2017-07-14T04:09:31.130 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 4034, in main_destroy
2017-07-14T04:09:31.198 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:main_destroy_locked(args)
2017-07-14T04:09:31.236 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 4061, in main_destroy_locked
2017-07-14T04:09:31.239 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:base_dev = get_partition_base(dev_path)
2017-07-14T04:09:31.242 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:File "/usr/lib/python2.7/dist-packages/ceph_disk/main.py", line 828, in get_partition_base
2017-07-14T04:09:31.244 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:raise Error('not a block device', dev)
2017-07-14T04:09:31.250 INFO:tasks.workunit.client.0.vpm021.stderr:DEBUG:CephDisk:ceph_disk.main.Error: Error: not a block device: /dev/vdb1

/a/kchai-2017-07-14_03:23:35-ceph-disk-wip-15432-kefu-distro-basic-vps/1398707/teuthology.log

else:
# FreeBSD does not have block devices
# All disks are character devices
if FREEBSD and stat.S_ISCHR(dmode):

This comment has been minimized.

@tchaikov

tchaikov Jul 14, 2017

Contributor

nit, could just put

    return FREEBSD and stat.S_ISCHR(dmode)

here

wjwithagen added some commits Jul 6, 2017

ceph-disk/main.py: Replace ST_ISBLK() test by is_diskdevice()
 - FreeBSD does not have blockdevices any more (since 2002)
   So disk are just Character special devices, so test on ISCHR

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
ceph_disk/main.py: fixed bugs from theutology report
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jul 14, 2017

@tchaikov
Thanx for running and explaining the theutology bugs. I always have a hard time finding the actual mistake in the sea of lines.
Submitted fixes, running thru Jenkins.

@tchaikov tchaikov added the needs-qa label Jul 14, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 14, 2017

ceph-disk suite fails on master: http://pulpito.ceph.com/kchai-2017-07-14_16:33:13-ceph-disk-master-distro-basic-vps/

we need to fixed it before running this PR through the test suite.

@tchaikov tchaikov self-assigned this Jul 14, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jul 14, 2017

@tchaikov
Looks like something that has to do with a missing global p??
Doesn't llok like something I intentionally modified.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 15, 2017

@wjwithagen thanks for looking at it. i think we need to whitelist some health warnings. no, it was a bug in #16347. i should s/p/proc/, fixed already.

@tchaikov tchaikov merged commit 171104c into ceph:master Jul 15, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 24, 2017

my bad: the test above did not contain this PR .

@wjwithagen wjwithagen deleted the wjwithagen:wip-wjw-ceph-disk-is_diskdevice branch Feb 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment