Bug #16125
closedceph-disk should not spit out a traceback when a subprocess.call errors
0%
Description
It doesn't add anything useful (even if debugging) and makes it look like it broke (it didn't on this example):
Traceback (most recent call last):
File "/sbin/ceph-disk", line 9, in <module>
load_entry_point('ceph-disk==1.0.0', 'console_scripts', 'ceph-disk')()
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 4993, in run
main(sys.argv[1:])
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 4946, in main
main_catch(args.func, args)
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 4971, in main_catch
func(args)
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 1774, in main
Prepare.factory(args).prepare()
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 1762, in prepare
self.prepare_locked()
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 1794, in prepare_locked
self.data.prepare(self.journal)
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 2446, in prepare
self.prepare_device(*to_prepare_list)
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 2622, in prepare_device
to_prepare.prepare()
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 1964, in prepare
self.prepare_device()
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 2054, in prepare_device
num=num)
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 1522, in create_partition
self.path,
File "/usr/lib/python2.7/site-packages/ceph_disk/main.py", line 439, in command_check_call
return subprocess.check_call(arguments)
File "/usr/lib64/python2.7/subprocess.py", line 542, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/sbin/sgdisk', '--new=3:0:+5120M', '--change-name=3:ceph journal', '--partition-guid=3:2de7e567-6f62-4bd6-95db-fe864051fabc', '--typecode=3:45b0969e-9b03-4f30-b4c6-b4b80ceff106', '--mbrtogpt', '--', '/dev/vdb']' returned non-zero exit status 4
Catching `CalledProcessError` and properly reporting the error with logging should be a good option here
Updated by Alfredo Deza almost 8 years ago
- Status changed from New to Fix Under Review
Pull Request: https://github.com/ceph/ceph/pull/9485
Updated by Loïc Dachary over 7 years ago
@alfredo could you explain the purpose of this change in this context:
commit ca164160a3628582eecf54023898c8568cd2f5ca
Author: Alfredo Deza <adeza@redhat.com>
Date: Fri Jun 3 11:05:20 2016 -0400
ceph-disk: point to the authors file for contributors
Signed-off-by: Alfredo Deza <adeza@redhat.com>
diff --git a/src/ceph-disk/ceph_disk/main.py b/src/ceph-disk/ceph_disk/main.py
index 2328c96..27fc262 100755
--- a/src/ceph-disk/ceph_disk/main.py
+++ b/src/ceph-disk/ceph_disk/main.py
@@ -1,12 +1,10 @@
#!/usr/bin/env python
#
-# Copyright (C) 2015, 2016 Red Hat <contact@redhat.com>
+# Copyright (C) 2015-2016 Red Hat <contact@redhat.com> Authors
# Copyright (C) 2014 Inktank <info@inktank.com>
# Copyright (C) 2014 Cloudwatt <libre.licensing@cloudwatt.com>
# Copyright (C) 2014 Catalyst.net Ltd
#
-# Author: Loic Dachary <loic@dachary.org>
-#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU Library Public License as published by
# the Free Software Foundation; either version 2, or (at your option)
@@ -17,6 +15,7 @@
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Library Public License for more details.
#
+# See the AUTHORS file for names of contributors
from __future__ import print_function
Updated by Alfredo Deza over 7 years ago
I think there was a discussion on what 'authors' should be at some point. I don't remember all the details (this ticket is six months old).
Since ceph-disk has been worked on by so many, do you feel an 'AUTHORS' list doesn't better reflect the contributors?
Updated by Nathan Cutler over 7 years ago
I just did "egrep -r 'Author:' src/" in a ceph/ceph clone and saw that there are many lines similar to this one in the tree.
Alfredo, can you explain why this one needs to be dropped and not the others?
Updated by Nathan Cutler over 7 years ago
Not to diminish anyone's contribution, but Loic did author over half of the lines:
$ wc --lines src/ceph-disk/ceph_disk/main.py 5146 src/ceph-disk/ceph_disk/main.py $ git blame src/ceph-disk/ceph_disk/main.py | grep 'Loic Dachary' | wc --lines 2818
Updated by Alfredo Deza over 7 years ago
I didn't think a whole-project change was the right thing to do. I worked on ceph-disk for this ticket, so I updated this one file.
It wasn't my intention to make it controversial. It felt odd to have a single author listed there since many have contributed to it.
Updated by Nathan Cutler over 7 years ago
But removing other people's names without asking is always going to be controversial, right?
Updated by Alfredo Deza over 7 years ago
I am not removing anyone's name: Loic's name is still in the Authors file.
This is not something I am doing without asking, that is exactly why there is a pull request for this and a ticket, it is out in the open, so others
can comment and give feedback (like you are in this ticket).
If you think my explanations aren't sufficient, could you propose (in the PR maybe?) what is it that you think would be more correct? Even if that means to avoid changing
the Author (which, again, I was not removing)
Updated by Loïc Dachary over 7 years ago
For the record I added Author: Loic Dachary in files where the contribution I made was covered by copyright (i.e. non trivial) for two reasons. Back when I was working for CloudWatt it clearly identified in which part my contribution was significant and was instrumental to convince my management to let me work on Ceph rather than divert me to other tasks. The other reason is that under copyright law (at least for all signatories of the Berne convention) the individual author has a moral right (which cannot be forfeited in France, even by contract) to display his name on his work. Although it could be mistaken as me claiming to be the sole author of the file for someone ignorant of revision control, it is not unreasonable to assume people reading the sources are well informed on that matter. This is not something I ever explained anywhere before because I don't think anyone thought of removing my name from a source file until now. Other authors added their names here and there, maybe for similar reasons, maybe for other reasons. Although it leads to a non uniform and somewhat chaotic display of copyright and authors, it is a status quo that did not raise any concern so far, even when Red Hat lawyers audited the code back when Inktank was acquired.
Updated by Alfredo Deza over 7 years ago
I have made the requested changes on the branch, and opened a new PR at https://github.com/ceph/ceph/pull/12414
Updated by Loïc Dachary over 7 years ago
- Status changed from Fix Under Review to Resolved