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

ceph-volume: add dmcrypt support in raw mode #35468

Merged
merged 3 commits into from Jun 26, 2020
Merged

Conversation

guits
Copy link
Contributor

@guits guits commented Jun 8, 2020

This commit adds the dmcrypt support in ceph-volume raw mode.

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

Signed-off-by: Guillaume Abrioux gabrioux@redhat.com

@guits guits requested a review from a team as a code owner June 8, 2020 07:44
@guits guits marked this pull request as draft June 8, 2020 07:44
@leseb
Copy link
Member

leseb commented Jun 9, 2020

Please attach the tracker bug for the backport: https://tracker.ceph.com/issues/45959

@guits guits marked this pull request as ready for review June 16, 2020 13:55
@guits guits force-pushed the guits-raw_dmcrypt branch 3 times, most recently from 3857883 to 42527b5 Compare June 17, 2020 15:43
Copy link
Contributor

@jan--f jan--f left a comment

Choose a reason for hiding this comment

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

Look alright to me, besides a few nits.
Generally I would really like to see some tests for the raw mode, now that this gets more features.

src/ceph-volume/ceph_volume/devices/raw/prepare.py Outdated Show resolved Hide resolved
src/ceph-volume/ceph_volume/devices/raw/prepare.py Outdated Show resolved Hide resolved
src/ceph-volume/ceph_volume/devices/raw/prepare.py Outdated Show resolved Hide resolved
@guits guits force-pushed the guits-raw_dmcrypt branch 4 times, most recently from 394923a to 32be2af Compare June 22, 2020 09:10
@guits guits requested a review from jan--f June 22, 2020 09:11
@guits guits force-pushed the guits-raw_dmcrypt branch 3 times, most recently from 08b3784 to 532a90a Compare June 23, 2020 12:58
@guits guits force-pushed the guits-raw_dmcrypt branch 6 times, most recently from 0b98e54 to ca2475b Compare June 25, 2020 11:35
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

Great job, thanks for adding unit test coverage too. @jan--f could we move forward with that? Thanks.

Copy link
Contributor

@jan--f jan--f left a comment

Choose a reason for hiding this comment

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

One last nit for the sake of consistency.

@jan--f
Copy link
Contributor

jan--f commented Jun 25, 2020

jenkins test ceph-volume tox

@jan--f
Copy link
Contributor

jan--f commented Jun 25, 2020

jenkins test ceph-volume tox

This commit adds the dmcrypt support in `ceph-volume raw` mode.

Note about `ceph-volume raw list` change:
Given `lsblk -J` (json output) option isn't available on all OS, I came up with
adding '--inverse' option to the existing command which allows us to get the
mapper devices list in that command output. Not listing root devices containing
partitions shouldn't have side effect since we are in `ceph-volume raw`
context.

example:
running `lsblk --paths --nodeps --output=NAME --noheadings` doesn't allow to
get the mapper list because the output is like following :

$ lsblk --paths --nodeps --output=NAME --noheadings
/dev/sda
/dev/sdb
/dev/sdc
/dev/sdd

the dmcrypt mappers are hidden because of the `--nodeps` given they are
displayed as a dependency.

$ lsblk --paths --output=NAME --noheadings
/dev/sda
|-/dev/mapper/ceph-3b52c90d-6548-407d-bde1-efd31809702f-sda-block-dmcrypt
`-/dev/mapper/ceph-3b52c90d-6548-407d-bde1-efd31809702f-sda-db-dmcrypt
/dev/sdb
/dev/sdc
/dev/sdd

adding `--inverse` is a trick to get around this issue, the counterpart is that
we can't list root devices if they contain at least one partition but this
shouldn't be an issue in `ceph-volume raw` context given we only deal with
raw devices.

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

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
This commit adds testing against `ceph-volume raw` subcommand.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
This function is never called in raw context, let's remove it.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
@guits
Copy link
Contributor Author

guits commented Jun 25, 2020

jenkins test ceph-volume tox

@JINHAOHU
Copy link

Why we don't need to decrypt the device during the activate process? I see that during the activate process of the lvm mode, it will call LUKS open to decrypt the device.

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