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-volume: add drive-group subcommand #35728

Merged
merged 1 commit into from Jul 29, 2020
Merged

ceph-volume: add drive-group subcommand #35728

merged 1 commit into from Jul 29, 2020

Conversation

jan--f
Copy link
Contributor

@jan--f jan--f commented Jun 23, 2020 •
edited

This new subcommand takes a drive group specification as json and deploys
the OSDs accordingly.

Signed-off-by: Jan Fajerski jfajerski@suse.com

Fixes: https://tracker.ceph.com/issues/46689

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 classic perf
  • 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

@jan--f jan--f requested review from a team as code owners June 23, 2020 13:17
@jan--f
Copy link
Contributor Author

jan--f commented Jun 23, 2020

cc @BlaineEXE

@jan--f jan--f force-pushed the c-v-add-subcommand-parse-drive-groups branch from 25aba62 to faa2653 Compare June 23, 2020 13:26
@sebastian-philipp
Copy link
Contributor

regarding backporting to older releases. What do you need from python-common?

@jan--f jan--f force-pushed the c-v-add-subcommand-parse-drive-groups branch from faa2653 to afa4eff Compare June 26, 2020 09:13
@jan--f
Copy link
Contributor Author

jan--f commented Jun 26, 2020

Since this is py3 only backports will be limited to py3-only branches. rook is currently the only prospective user so lets see what @BlaineEXE thinks.

@jan--f
Copy link
Contributor Author

jan--f commented Jun 26, 2020

Should be rebased if/when #35800 is merged.

@BlaineEXE
Copy link
Contributor

Since in Rook we don't know which version of Ceph this feature will appear in, can we add a flag to this command ceph-volume drive-group --is-supported? This should return 0 if it is supported, and obviously if it isn't supported there will be a failure of some kind which I believe currently returns nonzero.

@jan--f
Copy link
Contributor Author

jan--f commented Jun 27, 2020

Since in Rook we don't know which version of Ceph this feature will appear in, can we add a flag to this command ceph-volume drive-group --is-supported? This should return 0 if it is supported, and obviously if it isn't supported there will be a failure of some kind which I believe currently returns nonzero.

No need for an extra argument. You can just run ceph-volume drive-groups. A version without it will return rc 2 (iirc), a version with it will print the help and exit with 0.

@BlaineEXE
Copy link
Contributor

Since in Rook we don't know which version of Ceph this feature will appear in, can we add a flag to this command ceph-volume drive-group --is-supported? This should return 0 if it is supported, and obviously if it isn't supported there will be a failure of some kind which I believe currently returns nonzero.

No need for an extra argument. You can just run ceph-volume drive-groups. A version without it will return rc 2 (iirc), a version with it will print the help and exit with 0.

In practice, that's not how it works because if I run ceph-volume drive-group it hangs waiting on stdin AFACT.

@jan--f
Copy link
Contributor Author

jan--f commented Jun 30, 2020

In practice, that's not how it works because if I run ceph-volume drive-group it hangs waiting on stdin AFACT.

Ah yes, you're right of course. Probably best to enable parsing from stdin via an argument though. I'll rebase and push that change shortly.

@jan--f jan--f force-pushed the c-v-add-subcommand-parse-drive-groups branch from afa4eff to 7493bf7 Compare June 30, 2020 12:48
@jan--f
Copy link
Contributor Author

jan--f commented Jun 30, 2020

ok so I implemented to following:

  • to pass via stdin "-" must be passed as a positional arg
  • if c-v has the drive-groups command calling ceph-volume drive-group prints the help to stderr and exits with rc 0
  • if its not present calling ceph-volume drive-group will result in rc 2

I think its weird to return 0 when no arguments are given, but since all other c-v subcommands behave that way I opted for consistency.

BlaineEXE
BlaineEXE approved these changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

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

Confirmed this working with my WIP Rook changes.
rook/rook#4916

@BlaineEXE
Copy link
Contributor

jenkins retest this please

@BlaineEXE BlaineEXE mentioned this pull request Jul 8, 2020
Merged
16 tasks
@BlaineEXE
Copy link
Contributor

jenkins retest this please

@BlaineEXE
Copy link
Contributor

BlaineEXE commented Jul 13, 2020 •
edited

jenkins retest this please (the default test is forever yellow)

@sebastian-philipp
Copy link
Contributor

jenkins retest this please

@sebastian-philipp
Copy link
Contributor

jenkins test make check

@sebastian-philipp sebastian-philipp mentioned this pull request Jul 20, 2020
Merged
3 tasks
ceph-volume: add drive-group subcommand
e5b585d 
This new subcommand takes a drive group specification as json and deploys
the OSDs accordingly.

Signed-off-by: Jan Fajerski <jfajerski@suse.com>

Fixes: https://tracker.ceph.com/issues/46689
@jan--f jan--f force-pushed the c-v-add-subcommand-parse-drive-groups branch from 7493bf7 to e5b585d Compare July 23, 2020 10:11
@BlaineEXE
Copy link
Contributor

@guits @jschmid1 are you able to review? I'd love for this to make it into Ceph v15.2.5. I tested this to be working for our desires with Rook.

@jschmid1
Copy link
Contributor

added to my list of reviews

jschmid1
jschmid1 approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@jschmid1 jschmid1 left a comment

Choose a reason for hiding this comment

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

looks fine 👍

@jan--f jan--f merged commit 95d5ccc into ceph:master Jul 29, 2020
@dsavineau
Copy link
Contributor

This PR broke the entire ceph-volume command.

I've opened https://tracker.ceph.com/issues/46759

@sebastian-philipp sebastian-philipp mentioned this pull request Aug 4, 2020
Merged
@jan--f jan--f mentioned this pull request Aug 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@BlaineEXE BlaineEXE BlaineEXE approved these changes

@jschmid1 jschmid1 jschmid1 approved these changes

@guits guits Awaiting requested review from guits

Assignees
No one assigned
Labels
ceph-volume cephadm feature
Projects
None yet
Milestone
No milestone
5 participants
@jan--f @sebastian-philipp @BlaineEXE @jschmid1 @dsavineau