Skip to content
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

Issue: 24044 #21887

Closed
wants to merge 1 commit into from
Closed

Issue: 24044 #21887

wants to merge 1 commit into from

Conversation

ron-slc
Copy link
Contributor

@ron-slc ron-slc commented May 8, 2018

Fix improper boolean analysis, of return value from check_id

Signed-off-by: Ron Allred ron@itrefined.com

Fix improper boolean analysis, of return value from check_id

Signed-off-by: Ron Allred <ron@itrefined.com>
@alfredodeza
Copy link
Contributor

@ron-slc commits in ceph-volume are usually prefixed with "ceph-volume " and followed by the component name (you can see examples in the commit history for the file you are changing). Could you please update your commit?

Another thing that might be useful here would be updating that function's name. Since it is returning a boolean, something like osd_id_exists would help clarify here.

@alfredodeza
Copy link
Contributor

jenkins test ceph-volume tox

@alfredodeza
Copy link
Contributor

This PR is making all tests fail, the jenkins job is at https://jenkins.ceph.com/job/ceph-volume-test/12/

You can see a related log at https://jenkins.ceph.com/job/ceph-volume-scenario/279/artifact/logs/osd0/ceph-volume.log/*view*/

Exception is:

[2018-05-16 21:53:24,358][ceph_volume][ERROR ] exception caught by decorator
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/ceph_volume/decorators.py", line 59, in newfunc
    return f(*a, **kw)
  File "/usr/lib/python2.7/dist-packages/ceph_volume/main.py", line 158, in main
    terminal.dispatch(self.mapper, subcommand_args)
  File "/usr/lib/python2.7/dist-packages/ceph_volume/terminal.py", line 182, in dispatch
    instance.main()
  File "/usr/lib/python2.7/dist-packages/ceph_volume/devices/lvm/main.py", line 38, in main
    terminal.dispatch(self.mapper, self.argv)
  File "/usr/lib/python2.7/dist-packages/ceph_volume/terminal.py", line 182, in dispatch
    instance.main()
  File "/usr/lib/python2.7/dist-packages/ceph_volume/devices/lvm/create.py", line 69, in main
    self.create(args)
  File "/usr/lib/python2.7/dist-packages/ceph_volume/decorators.py", line 16, in is_root
    return func(*a, **kw)
  File "/usr/lib/python2.7/dist-packages/ceph_volume/devices/lvm/create.py", line 26, in create
    prepare_step.safe_prepare(args)
  File "/usr/lib/python2.7/dist-packages/ceph_volume/devices/lvm/prepare.py", line 216, in safe_prepare
    self.prepare(args)
  File "/usr/lib/python2.7/dist-packages/ceph_volume/decorators.py", line 16, in is_root
    return func(*a, **kw)
  File "/usr/lib/python2.7/dist-packages/ceph_volume/devices/lvm/prepare.py", line 245, in prepare
    self.osd_id = prepare_utils.create_id(osd_fsid, json.dumps(secrets), osd_id=args.osd_id)
  File "/usr/lib/python2.7/dist-packages/ceph_volume/util/prepare.py", line 72, in create_id
    show_command=True
  File "/usr/lib/python2.7/dist-packages/ceph_volume/process.py", line 185, in call
    command_msg = "Running command: %s" % ' '.join(command)
TypeError: sequence item 12: expected string, NoneType found

The problem is that if osd_id is a None then check_id will return False. So this change would need to account for the fact that osd_id can be None when not explicitly passed in through the CLI flag.

@ron-slc
Copy link
Contributor Author

ron-slc commented May 24, 2018

Hi Alfredo! Thank you much for the tips, and patience. I'm new to Python, and Github itself.

This being said, I made a fatal newbie mistake on my Github fork of ceph :( . So I'm unable to update my previous PR. I apologize, I'm used to local Git.

I have been forced to create a new PR#22220 . On this new PR I have tried to match the labels, and naming patterns as well. Thanks again!

@alfredodeza
Copy link
Contributor

Closing PR as this is now being addressed at #22220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants