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.

Sign up for GitHub

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

Jump to bottom

ceph-daemon: infer fsid for some commands #31702

Merged
merged 1 commit into from Nov 21, 2019
Merged

ceph-daemon: infer fsid for some commands #31702

merged 1 commit into from Nov 21, 2019

Conversation

liewegas
Copy link
Member

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@liewegas liewegas requested a review from a team as a code owner November 18, 2019 04:53
sebastian-philipp
sebastian-philipp suggested changes Nov 18, 2019
View reviewed changes
src/ceph-daemon/ceph-daemon Outdated
@@ -226,6 +226,21 @@ def is_fsid(s):
return False
return True

def infer_fsid():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a way to infer the fsid by looking as the ceph-volume LVs.

Suggested change
def infer_fsid():
def infer_fsid_from_dirname():

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe if c-v weren't so slow...

src/ceph-daemon/ceph-daemon Outdated
@@ -1252,6 +1267,7 @@ def command_run():

def command_shell():
# type: () -> int
infer_fsid()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this all over the place seems wrong to me, though. can we call infer_fsid one place only?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that deliberately so that we infer only in cases where it is not dangerous to do so. 'deploy' is used only by mgr/ssh, so we don't do it there, and we definitely don't want to for rm-cluster or rm-daemon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's cleaner todo this as a python decorator ?

mgfritch@da6fa2f

mgfritch
mgfritch requested changes Nov 19, 2019
View reviewed changes
Copy link
Contributor

@mgfritch mgfritch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's cleaner todo this as a python decorator ?

mgfritch/ceph@da6fa2f

src/ceph-daemon/ceph-daemon Outdated
for i in os.listdir(args.data_dir):
if is_fsid(i):
if first:
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise RuntimeError when more than one fsid is found?

src/ceph-daemon/ceph-daemon Outdated
@@ -1252,6 +1267,7 @@ def command_run():

def command_shell():
# type: () -> int
infer_fsid()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's cleaner todo this as a python decorator ?

mgfritch@da6fa2f

@liewegas liewegas changed the title RFC: ceph-daemon: infer fsid for some commands ceph-daemon: infer fsid for some commands Nov 20, 2019
@liewegas
Copy link
Member Author

@mgfritch I made a few minor changes to your patch--much nicer! Except that it doesn't seem to work... it just returns without running the decorated function. I don't really understand how these decorations are supposed to work.. can you take a look?

glitch:build (wip-cd-infer-fsid) 07:50 PM $ ls /var/lib/ceph
ffb8b348-0a14-11ea-8b0b-f4631fe4def3/
glitch:build (wip-cd-infer-fsid) 07:50 PM $ sudo ../src/ceph-daemon/ceph-daemon shell
INFO:ceph-daemon:Inferring fsid ffb8b348-0a14-11ea-8b0b-f4631fe4def3
glitch:build (wip-cd-infer-fsid) 07:50 PM $

@mgfritch
Copy link
Contributor

mgfritch commented Nov 20, 2019 •
edited

@liewegas oops! I forgot to return the wrapped func ...

# ./ceph-daemon shell
INFO:ceph-daemon:Inferring fsid 00000000-0000-0000-0000-ffffdeadbeef
[ceph: root@admin /]#

@mgfritch
ceph-daemon: infer fsid for shell, enter, ceph-volume, unit, logs
4312ca8 
Signed-off-by: Michael Fritch <mfritch@suse.com>
@mgfritch
Copy link
Contributor

Also fixed up argparse to no longer require --fsid for commands in which we infer the fsid.

# ./ceph-daemon enter
usage: ceph-daemon enter [-h] --fsid FSID --name NAME [command [command ...]]
ceph-daemon enter: error: argument --fsid is required

@liewegas
Copy link
Member Author

retest this please

1 similar comment
@liewegas
Copy link
Member Author

retest this please

liewegas added a commit that referenced this pull request Nov 21, 2019
@liewegas
Merge PR #31702 into master
7948f5c 
* refs/pull/31702/head:
	ceph-daemon: infer fsid for shell, enter, ceph-volume, unit, logs

Reviewed-by: Michael Fritch <mfritch@suse.com>
Reviewed-by: Sebastian Wagner <swagner@suse.com>
@liewegas liewegas merged commit 4312ca8 into ceph:master Nov 21, 2019
@liewegas liewegas deleted the wip-cd-infer-fsid branch November 21, 2019 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgfritch mgfritch mgfritch approved these changes

@sebastian-philipp sebastian-philipp sebastian-philipp approved these changes

Assignees
No one assigned
Labels
orchestrator wip-sage5-testing
Projects
None yet
Milestone
No milestone
3 participants
@liewegas @mgfritch @sebastian-philipp