Project

General

Profile

Bug #16125

ceph-disk should not spit out a traceback when a subprocess.call errors

Added by Alfredo Deza over 6 years ago. Updated almost 6 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
-
Target version:
-
% Done:

0%

Source:
other
Tags:
Backport:
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Pull request ID:
Crash signature (v1):
Crash signature (v2):

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

History

#1 Updated by Alfredo Deza over 6 years ago

  • Assignee set to Alfredo Deza

#2 Updated by Alfredo Deza over 6 years ago

  • Status changed from New to Fix Under Review

#3 Updated by Loïc Dachary about 6 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

#4 Updated by Alfredo Deza about 6 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?

#5 Updated by Nathan Cutler about 6 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?

#6 Updated by Nathan Cutler about 6 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

#7 Updated by Alfredo Deza about 6 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.

#8 Updated by Nathan Cutler about 6 years ago

But removing other people's names without asking is always going to be controversial, right?

#9 Updated by Alfredo Deza about 6 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)

#10 Updated by Loïc Dachary about 6 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.

#11 Updated by Alfredo Deza almost 6 years ago

I have made the requested changes on the branch, and opened a new PR at https://github.com/ceph/ceph/pull/12414

#12 Updated by Loïc Dachary almost 6 years ago

  • Status changed from Fix Under Review to Resolved

Also available in: Atom PDF