ceph-volume: do not use --key during mkfs #19276
Conversation
|
jenkins test ceph-volume lvm xenial-bluestore-create |
|
jenkins test ceph-volume lvm centos7-bluestore-create |
|
jenkins test ceph-volume lvm xenial-bluestore-create |
|
@alfredodeza is there any simple way i can trigger the test like you did in #19260? |
|
@tchaikov the |
|
i like this better! |
|
jenkins test ceph-volume lvm xenial-bluestore-create |
|
jenkins test ceph-volume lvm centos7-bluestore-create |
| stdout, stderr = process.communicate(stdin) | ||
| log_output('stdout', stdout, terminal_logging) | ||
| log_output('stderr', stderr, terminal_logging) | ||
| returncode = process.poll(): |
There was a problem hiding this comment.
this change tries to address the failure in https://jenkins.ceph.com/job/ceph-volume-prs-lvm-centos7-bluestore-create/27/consoleFull#-1970939011c19247c4-fcb7-4c61-9a5d-7e2b9731c678 and https://jenkins.ceph.com/job/ceph-volume-prs-lvm-xenial-bluestore-create/8/consoleFull#842358957c212b007-e891-4176-9ee7-2f60eca393b7
Running command: sudo ceph-osd --cluster ceph --osd-objectstore bluestore --mkfs -i 0 --monmap /var/lib/ceph/osd/ceph-0/activate.monmap --keyfile * --bluestore-block-db-path /dev/journals/journal1 --osd-data /var/lib/ceph/osd/ceph-0/ --osd-uuid 055a8ab3-abf4-48f1-9ac6-b095105c7890 --setuser ceph --setgroup ceph
STDERR:
Traceback (most recent call last):
File "/sbin/ceph-volume", line 6, in <module>
main.Volume()
File "/usr/lib/python2.7/site-packages/ceph_volume/main.py", line 37, in __init__
self.main(self.argv)
File "/usr/lib/python2.7/site-packages/ceph_volume/decorators.py", line 59, in newfunc
return f(*a, **kw)
File "/usr/lib/python2.7/site-packages/ceph_volume/main.py", line 144, in main
terminal.dispatch(self.mapper, subcommand_args)
File "/usr/lib/python2.7/site-packages/ceph_volume/terminal.py", line 131, in dispatch
instance.main()
File "/usr/lib/python2.7/site-packages/ceph_volume/devices/lvm/main.py", line 38, in main
terminal.dispatch(self.mapper, self.argv)
File "/usr/lib/python2.7/site-packages/ceph_volume/terminal.py", line 131, in dispatch
instance.main()
File "/usr/lib/python2.7/site-packages/ceph_volume/devices/lvm/create.py", line 57, in main
self.create(args)
File "/usr/lib/python2.7/site-packages/ceph_volume/decorators.py", line 16, in is_root
return func(*a, **kw)
File "/usr/lib/python2.7/site-packages/ceph_volume/devices/lvm/create.py", line 21, in create
Prepare([]).prepare(args)
File "/usr/lib/python2.7/site-packages/ceph_volume/decorators.py", line 16, in is_root
return func(*a, **kw)
File "/usr/lib/python2.7/site-packages/ceph_volume/devices/lvm/prepare.py", line 231, in prepare
fsid=osd_fsid,
File "/usr/lib/python2.7/site-packages/ceph_volume/devices/lvm/prepare.py", line 72, in prepare_bluestore
db=db
File "/usr/lib/python2.7/site-packages/ceph_volume/util/prepare.py", line 224, in osd_mkfs_bluestore
process.run(command, obfuscate='--keyfile', stdin=keyring)
File "/usr/lib/python2.7/site-packages/ceph_volume/process.py", line 120, in run
[process.stdout.fileno(), process.stderr.fileno()],
ValueError: I/O operation on closed file
There was a problem hiding this comment.
it introduces an invalid syntax though:
File "/usr/lib/python2.7/site-packages/ceph_volume/process.py", line 94
returncode = process.poll():
^
SyntaxError: invalid syntax
There was a problem hiding this comment.
Also, lets try a way to keep these things around, because it is meant to report output as it happens. With the proposed approach we would need to wait until the very end to output anything.
I believe that the correct thing to do here is to use process.call not process.run. And to remove the ability to pass stdin to process.run because as implemented it will never work (once communicate() is called the descriptors are closed)
There was a problem hiding this comment.
@alfredodeza thanks. probably we can use the "stdin" property for writing the stdin of the Popen instance?
| @@ -221,7 +221,7 @@ def osd_mkfs_bluestore(osd_id, fsid, keyring=None, wal=False, db=False): | |||
|
|
|||
| command = base_command + supplementary_command | |||
|
|
|||
| process.run(command, obfuscate='--key') | |||
| process.run(command, obfuscate='--keyfile', stdin=keyring) | |||
There was a problem hiding this comment.
so this would become:
process.call(command, stdin=keyring)
tchaikov
force-pushed
the
wip-volume-key
branch
from
bc997c8
to
fa06b52
Compare
|
jenkins test ceph-volume lvm centos7-bluestore-create |
|
jenkins test ceph-volume lvm xenial-bluestore-create |
tchaikov
force-pushed
the
wip-volume-key
branch
from
fa06b52
to
e916167
Compare
|
jenkins test ceph-volume lvm centos7-bluestore-create |
tchaikov
force-pushed
the
wip-volume-key
branch
2 times, most recently
from
ce3da73
to
ca59586
Compare
| @@ -114,7 +114,8 @@ def run(command, **kw): | |||
| ) | |||
|
|
|||
| if stdin: | |||
| process.communicate(stdin) | |||
| process.stdin.write(stdin) | |||
| process.stdin.close() | |||
There was a problem hiding this comment.
It is recommended not to write directly like this to stdin because:
Use communicate() rather than .stdin.write, .stdout.read or .stderr.read to avoid deadlocks due to any of the > other OS pipe buffers filling up and blocking the child process.
From: https://docs.python.org/2/library/subprocess.html#subprocess.Popen.communicate
IMO, this should be removed, we can't do this nicely and keep the terminal output flowing. This is currently OK as nothing else is calling process.run and using stdin here. There is another place where process.call is being used when trying stdin.
The right thing would be to just remove the stdin ability from here.
| @@ -221,7 +221,7 @@ def osd_mkfs_bluestore(osd_id, fsid, keyring=None, wal=False, db=False): | |||
|
|
|||
| command = base_command + supplementary_command | |||
|
|
|||
| process.run(command, obfuscate='--key') | |||
| process.call(command, stdin=keyring) | |||
There was a problem hiding this comment.
Yep, this is the correct thing to do here
|
jenkins test ceph-volume lvm centos7-bluestore-create |
|
jenkins test ceph-volume lvm xenial-bluestore-create |
tchaikov
force-pushed
the
wip-volume-key
branch
from
ca59586
to
e3254a5
Compare
Also, don't print the raw key to the log. Signed-off-by: Sage Weil <sage@redhat.com> Signed-off-by: Kefu Chai <kefu@redhat.com>
We do not want the key to show up on the command line (it may appear in the process list or sudo log file). Fixes: http://tracker.ceph.com/issues/22283 Signed-off-by: Sage Weil <sage@redhat.com>
tchaikov
force-pushed
the
wip-volume-key
branch
2 times, most recently
from
f62f23c
to
3893e0f
Compare
|
jenkins test ceph-volume lvm xenial-bluestore-create |
|
jenkins test ceph-volume lvm centos7-bluestore-create |
| @@ -99,22 +99,19 @@ def run(command, **kw): | |||
| """ | |||
| stop_on_error = kw.pop('stop_on_error', True) | |||
| command_msg = obfuscate(command, kw.pop('obfuscate', None)) | |||
| stdin = kw.pop('stdin', None) | |||
| assert 'stdin' not in kw, "'stdin' not supported by process.run()" | |||
There was a problem hiding this comment.
Please don't use assert like this in Python. Since there is no interface that calls process.run with stdin it is safe to remove.
There was a problem hiding this comment.
i am fine removing this. but in general, i think a use case of assert in Python is to avoid possible caller passing undesired parameter.
Since there is no interface that calls
process.runwithstdinit is safe to remove.
that's exactly why i want to add an assert here: to avoid future interfaces calling process.run with stdin, if any, instead of taking precious time debugging it, we can fail early, and have a nice backtrace.
There was a problem hiding this comment.
Two important reasons on why we don't use assert in Python to control program flow are:
- Plain asserts can be removed by Python when environment variables or flags are used (see
python -OorPYTHONOPTIMIZE) - It doesn't offer granularity as to what exceptions one should be catching. In the (unlikely) situation some other tool is using plain assertions as a generic stop-all, one cannot safely catch those and ignore some others (and vice versa)
These interfaces are very well documented, and are clear in what they support. The usage of keyword arguments (kw) makes it lenient to accept anything and everything, but if adding a check for one variable, then checks must be implemented for every variable that you think may cause an error because it is not being used (whitelist/blacklist). It would also need to be added to process.call since the same kw mechanism is being used there. We haven't added such a mechanism because we are betting on well-documented functions.
I am -1 on adding a check for process.run even with granular exception for this because it then must implement something for all the other options it must not accept via kw.
There was a problem hiding this comment.
faire enough. but if it's documented that input is not a valid parameter, i would not have used it. anyway, i will remove it.
There was a problem hiding this comment.
also, please note that i am not using assert for "controlling program flow". it serves as a way for debugging and documentation.
we cannot use process.communicate() to feed the Popen with input, because, upon return of process.communicate() the stdout,stderr are closed. see https://docs.python.org/2/library/subprocess.html#subprocess.Popen.communicate . Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov
force-pushed
the
wip-volume-key
branch
from
3893e0f
to
6df444c
Compare
|
@alfredodeza ping. |
|
jenkins test ceph-volume lvm centos7-bluestore-create |
|
jenkins test ceph-volume lvm centos7-filestore-create |
|
jenkins test ceph-volume lvm xenial-bluestore-create |
|
jenkins test ceph-volume lvm xenial-filestore-create |
No description provided.