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

src/ceph-disk/ceph_disk/main.py: Make 'ceph-disk list' work on FreeBSD #14483

Merged
merged 3 commits into from Apr 19, 2017

Conversation

Projects
None yet
2 participants
@wjwithagen
Contributor

wjwithagen commented Apr 12, 2017

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

@wjwithagen wjwithagen requested review from tchaikov and Apr 12, 2017

if re.match(r'^fd\d$', name):
continue
dev_part_list[name] = list_partitions(get_dev_path(name))
if not FreeBSD:

This comment has been minimized.

@ghost

ghost Apr 12, 2017

s/FreeBSD/FREEBSD/

This comment has been minimized.

@wjwithagen

wjwithagen Apr 12, 2017

Contributor

@dachary
Good catch

dev_part_list[name] = list_partitions(get_dev_path(name))
else:
for line in open(PROCDIR+"/partitions"):
columns = line.split(' ')

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor

the default separator of str.split() is whitespace, so we can just put line.split() here.

continue
dev_part_list[name] = list_partitions(get_dev_path(name))
else:
for line in open(PROCDIR+"/partitions"):

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor

nit, might want to release the file object explicitly. like

with open(PROCDIR+"/partitions") as partitions:
    for line in partitions:
      # ...
vdevline = line.split()
if os.path.exists(vdevline[1]+'/active'):
elems = vdevline[1].split('/')
print(vdevline[0]+ " ceph data, active, cluster ceph, "+elems[5]+

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor

pep8 warns at seeing this. maybe you can just put

print(vdevline[0], "ceph data, active, cluster ceph,", elems[5],
      "mounted on:", vdevline[1])

?

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor

and there are other places that violates pep8 in this change, could you run this file with pep8, and make sure it's pep8 warning/errors free?

except subprocess.CalledProcessError as e:
LOG.info('zfs list -o name,mountpoint '
'fails.\n (Error: %s)' % e)
return 1

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor

better off raising this exception in this case, main() does not use the return value from func() or main_catch(), and main_catch() just ignore the return value of func().

This comment has been minimized.

@wjwithagen

wjwithagen Apr 13, 2017

Contributor

@tchaikov
Removed the return.
But if listing fails, there is not much more we can do?

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor

we should re-raise the exception. so main_catch() can catch it, or, user can get more clue pass --verbose to ceph-disk at seeing the error, which will print out the backtrace in any exception is raised.

lines = out.splitlines()
for line in lines[2:]:
vdevline = line.split()
if os.path.exists(vdevline[1]+'/active'):

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor
os.path.exists(os.path.join(vdevline[1], 'active')
for line in lines[2:]:
vdevline = line.split()
if os.path.exists(vdevline[1]+'/active'):
elems = vdevline[1].split('/')

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor
elems = os.path.split(vdevline[1])
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Apr 13, 2017

@tchaikov @dachary
I'll look into all the remarks, and questions. Given my lack of experience with Python that migt take a few tries. So please have patience with me.

wjwithagen added some commits Apr 12, 2017

src/ceph-disk/ceph_disk/main.py: Make ceph-disk list work on FreeBSD
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
ceph-disk/tests/test_main.py: Some partition tests are no longer vali…
…d under FreeBSD

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
)
except subprocess.CalledProcessError as e:
LOG.info('zfs list -o name,mountpoint '
'fails.\n (Error: %s)' % e)

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor

need to re-raise this exception, you can just add

raise

in the except block.

src/ceph-disk/ceph_disk/main.py: raise exeception if zfs is not found
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Apr 14, 2017

@tchaikov @dachary
Issues should be addressed, can you take another look and/or set QA

@tchaikov

This comment has been minimized.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 18, 2017

@dachary did @wjwithagen address your comment?

@ghost

ghost approved these changes Apr 18, 2017

@tchaikov tchaikov merged commit 9d5fa61 into ceph:master Apr 19, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment