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-disk: ability to use a different cluster name with dmcrypt #11786

Merged
2 commits merged into from Feb 17, 2017

Conversation

Projects
None yet
@leseb
Contributor

leseb commented Nov 4, 2016

Prior to this commit we were not able to configure an OSD using dmcrypt
on a cluster with a different name than 'ceph'. Adding the command line
option to --cluster to fix this.

Signed-off-by: Sébastien Han seb@redhat.com

@liewegas liewegas added the build/ops label Nov 4, 2016

@liewegas liewegas added this to the kraken milestone Nov 4, 2016

@ktdreyer

This comment has been minimized.

Show comment
Hide comment
@ktdreyer

ktdreyer Nov 4, 2016

Member

Would you please file a ticket at http://tracker.ceph.com for this, and add the URL of the ticket to the commit log, so we can track this and ensure it is backported to Jewel eventually?

Member

ktdreyer commented Nov 4, 2016

Would you please file a ticket at http://tracker.ceph.com for this, and add the URL of the ticket to the commit log, so we can track this and ensure it is backported to Jewel eventually?

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb
Contributor

leseb commented Nov 8, 2016

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Nov 8, 2016

Contributor

I'll update the PR with a definitive fix today.

Contributor

leseb commented Nov 8, 2016

I'll update the PR with a definitive fix today.

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Nov 14, 2016

Contributor

@leseb could you put the line of Fixes: http://tracker.ceph.com/issues/17821 into your commit message? right before your "SIgned-off-by" line, see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#tag-the-commit .

Contributor

tchaikov commented Nov 14, 2016

@leseb could you put the line of Fixes: http://tracker.ceph.com/issues/17821 into your commit message? right before your "SIgned-off-by" line, see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#tag-the-commit .

@tchaikov tchaikov added the feature label Nov 14, 2016

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Nov 14, 2016

Contributor

@tchaikov just did, thanks!
I still have some work to do on that patch.

Contributor

leseb commented Nov 14, 2016

@tchaikov just did, thanks!
I still have some work to do on that patch.

@ktdreyer ktdreyer changed the title from ceph-disk: ability to use a different cluster name with dmcrypt to DNM: ceph-disk: ability to use a different cluster name with dmcrypt Nov 14, 2016

@ktdreyer

This comment has been minimized.

Show comment
Hide comment
@ktdreyer

ktdreyer Nov 14, 2016

Member

Please remove "DNM" from the title when this PR is ready for review and merging.

Member

ktdreyer commented Nov 14, 2016

Please remove "DNM" from the title when this PR is ready for review and merging.

@leseb leseb changed the title from DNM: ceph-disk: ability to use a different cluster name with dmcrypt to ceph-disk: ability to use a different cluster name with dmcrypt Nov 15, 2016

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Nov 15, 2016

Contributor

@ErwanAliasr1 and I reworked the patch, ready for review now :)

Contributor

leseb commented Nov 15, 2016

@ErwanAliasr1 and I reworked the patch, ready for review now :)

@ErwanAliasr1

This comment has been minimized.

Show comment
Hide comment
@ErwanAliasr1

ErwanAliasr1 Nov 16, 2016

Contributor

WDYT about this patch series guys ?

Contributor

ErwanAliasr1 commented Nov 16, 2016

WDYT about this patch series guys ?

@ktdreyer

This comment has been minimized.

Show comment
Hide comment
@ktdreyer

ktdreyer Nov 16, 2016

Member

Would you please rebase this onto the latest master to ensure that Jenkins is happy with it?

Member

ktdreyer commented Nov 16, 2016

Would you please rebase this onto the latest master to ensure that Jenkins is happy with it?

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Nov 16, 2016

Contributor

@ktdreyer done

Contributor

leseb commented Nov 16, 2016

@ktdreyer done

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 17, 2016

this needs rebasing (and the test fails)

ghost commented Nov 17, 2016

this needs rebasing (and the test fails)

Show outdated Hide outdated src/ceph-disk/ceph_disk/main.py
@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Nov 17, 2016

Contributor

Ok testing is blocked by #12033 waiting for the backport to re-validate.

Contributor

leseb commented Nov 17, 2016

Ok testing is blocked by #12033 waiting for the backport to re-validate.

@liewegas liewegas removed this from the kraken milestone Nov 28, 2016

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Dec 9, 2016

Contributor

@dachary I just pushed a new version, just removed the commit that brings --cluster support to activate since this is not needed.

Contributor

leseb commented Dec 9, 2016

@dachary I just pushed a new version, just removed the commit that brings --cluster support to activate since this is not needed.

@alfredodeza

This comment has been minimized.

Show comment
Hide comment
@alfredodeza

alfredodeza Jan 9, 2017

Contributor

Jenkins test this please

Contributor

alfredodeza commented Jan 9, 2017

Jenkins test this please

@alfredodeza

This comment has been minimized.

Show comment
Hide comment
@alfredodeza

alfredodeza Jan 10, 2017

Contributor

@leseb looks like you have some valid test failures:

E       AssertionError: Expected call: get_dmcrypt_key('UUID', '/etc/ceph/dmcrypt-keys', True)
E       Actual call: get_dmcrypt_key('UUID', '/etc/ceph/dmcrypt-keys', True, 'ceph')
Contributor

alfredodeza commented Jan 10, 2017

@leseb looks like you have some valid test failures:

E       AssertionError: Expected call: get_dmcrypt_key('UUID', '/etc/ceph/dmcrypt-keys', True)
E       Actual call: get_dmcrypt_key('UUID', '/etc/ceph/dmcrypt-keys', True, 'ceph')
@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Jan 17, 2017

Contributor

@alfredodeza where can I get more details about these errors?

Contributor

leseb commented Jan 17, 2017

@alfredodeza where can I get more details about these errors?

@alfredodeza

This comment has been minimized.

Show comment
Hide comment
@alfredodeza

alfredodeza Jan 17, 2017

Contributor

@leseb the logs expired, we need to try this again

Contributor

alfredodeza commented Jan 17, 2017

@leseb the logs expired, we need to try this again

@alfredodeza

This comment has been minimized.

Show comment
Hide comment
@alfredodeza

alfredodeza Jan 17, 2017

Contributor

jenkins test this please

Contributor

alfredodeza commented Jan 17, 2017

jenkins test this please

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Jan 18, 2017

Contributor

@alfredodeza not sure if my patch is responsible for this error, @dachary do you mind having a quick look into this please? Thanks!

Contributor

leseb commented Jan 18, 2017

@alfredodeza not sure if my patch is responsible for this error, @dachary do you mind having a quick look into this please? Thanks!

@alfredodeza

This comment has been minimized.

Show comment
Hide comment
@alfredodeza

alfredodeza Jan 18, 2017

Contributor

@leseb these failures are related to the changes you've made. Specifically to the get_dmcrypt_key() function, which it has been altered to receive the extra argument of 'ceph'.

That is why the test fails, because it was crafted to expect a strict set of arguments that didn't include the cluster name.

Since that is no longer the case, the test fails. You need to update the test to make sure that 'ceph' is now expected.

The test failure occurs in: ../src/ceph-disk/tests/test_prepare.py line 319

The actual failure is:

E       AssertionError: Expected call: get_dmcrypt_key('UUID', '/etc/ceph/dmcrypt-keys', True)
E       Actual call: get_dmcrypt_key('UUID', '/etc/ceph/dmcrypt-keys', True, 'ceph')

The assertion that needs to change is:

        assert m['_dmcrypt_map'].called
        m['get_dmcrypt_key'].assert_called_with(
           uuid, '/etc/ceph/dmcrypt-keys', True)
Contributor

alfredodeza commented Jan 18, 2017

@leseb these failures are related to the changes you've made. Specifically to the get_dmcrypt_key() function, which it has been altered to receive the extra argument of 'ceph'.

That is why the test fails, because it was crafted to expect a strict set of arguments that didn't include the cluster name.

Since that is no longer the case, the test fails. You need to update the test to make sure that 'ceph' is now expected.

The test failure occurs in: ../src/ceph-disk/tests/test_prepare.py line 319

The actual failure is:

E       AssertionError: Expected call: get_dmcrypt_key('UUID', '/etc/ceph/dmcrypt-keys', True)
E       Actual call: get_dmcrypt_key('UUID', '/etc/ceph/dmcrypt-keys', True, 'ceph')

The assertion that needs to change is:

        assert m['_dmcrypt_map'].called
        m['get_dmcrypt_key'].assert_called_with(
           uuid, '/etc/ceph/dmcrypt-keys', True)
@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Jan 18, 2017

Contributor

@alfredodeza thanks for the detailed explanation, just pushed a change that should hopefully fix this issue.

Contributor

leseb commented Jan 18, 2017

@alfredodeza thanks for the detailed explanation, just pushed a change that should hopefully fix this issue.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 8, 2017

@leseb you have permission to push, all ceph developers have permission and you're in the group, it must be something on your side that's not configured right.

ghost commented Feb 8, 2017

@leseb you have permission to push, all ceph developers have permission and you're in the group, it must be something on your side that's not configured right.

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Feb 8, 2017

Contributor

@dachary please see:

$ git remote -v |grep ceph-cii
ceph-cii        git@github.com:ceph/ceph-ci.git (push)
ceph-cii        git@github.com:ceph/ceph-ci.git (fetch)
$ git push ceph-cii dmcrypt-cluster-name
ERROR: Permission to ceph/ceph-ci.git denied to leseb.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Am I doing something wrong?

Contributor

leseb commented Feb 8, 2017

@dachary please see:

$ git remote -v |grep ceph-cii
ceph-cii        git@github.com:ceph/ceph-ci.git (push)
ceph-cii        git@github.com:ceph/ceph-ci.git (fetch)
$ git push ceph-cii dmcrypt-cluster-name
ERROR: Permission to ceph/ceph-ci.git denied to leseb.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Am I doing something wrong?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 8, 2017

I don't see what's wrong. Here are the permissions you have:

seb

I guess someone with better knowledge of github subtle settings can help out :-)

ghost commented Feb 8, 2017

I don't see what's wrong. Here are the permissions you have:

seb

I guess someone with better knowledge of github subtle settings can help out :-)

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Feb 8, 2017

Contributor

@dachary seems to be working now, branch pushed :)

Contributor

leseb commented Feb 8, 2017

@dachary seems to be working now, branch pushed :)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 8, 2017

@leseb nice :-) What was the problem ? Do you have access to the sepia lab ?

ghost commented Feb 8, 2017

@leseb nice :-) What was the problem ? Do you have access to the sepia lab ?

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Feb 8, 2017

Contributor

@dachary well looks like I wasn't part of Ceph team, then I got an email saying you added me and it was good :).

Contributor

leseb commented Feb 8, 2017

@dachary well looks like I wasn't part of Ceph team, then I got an email saying you added me and it was good :).

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 8, 2017

well looks like I wasn't part of Ceph team, then I got an email saying you added me and it was good :).

Oo amazing. I was looking to verify if you were listed and mistook the "add" box for the "search" box... happy ending :-) Welcome to the Ceph team, always nice to have new members ...

ghost commented Feb 8, 2017

well looks like I wasn't part of Ceph team, then I got an email saying you added me and it was good :).

Oo amazing. I was looking to verify if you were listed and mistook the "add" box for the "search" box... happy ending :-) Welcome to the Ceph team, always nice to have new members ...

@yuriw

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost approved these changes Feb 17, 2017

@ghost ghost merged commit 057e7b1 into ceph:master Feb 17, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
@neurodrone

This comment has been minimized.

Show comment
Hide comment
@neurodrone

neurodrone Feb 17, 2017

Contributor

Will this be backported to Jewel by any chance?

Contributor

neurodrone commented Feb 17, 2017

Will this be backported to Jewel by any chance?

@vumrao

This comment has been minimized.

Show comment
Hide comment
@vumrao

vumrao Feb 17, 2017

Contributor

@neurodrone this is the tracker - http://tracker.ceph.com/issues/17821, it was marked backport to jewel and I have changed it to pending backport. It will get backported to jewel.

Contributor

vumrao commented Feb 17, 2017

@neurodrone this is the tracker - http://tracker.ceph.com/issues/17821, it was marked backport to jewel and I have changed it to pending backport. It will get backported to jewel.

ganeshmaharaj added a commit to ganeshmaharaj/ceph-deploy that referenced this pull request Feb 20, 2017

Add cluster as an argument for osd activate
With recent changes in ceph (ceph/ceph#11786),
this change will allow ceph-deploy osd activate to complete without
errors. Follow-on fix for http://tracker.ceph.com/issues/17821

Signed-off-by: Ganesh Mahalingam <ganesh.mahalingam@intel.com>
@vasukulkarni

This comment has been minimized.

Show comment
Hide comment
@vasukulkarni

vasukulkarni Feb 20, 2017

Member

@dachary it would have been nice to default the cluster to 'ceph' here, we got a patch for ceph-deploy to add the cluster option there, Also would be nice to run ceph-deploy suite for ceph-disk changes as well.

Member

vasukulkarni commented Feb 20, 2017

@dachary it would have been nice to default the cluster to 'ceph' here, we got a patch for ceph-deploy to add the cluster option there, Also would be nice to run ceph-deploy suite for ceph-disk changes as well.

@ganeshmaharaj

This comment has been minimized.

Show comment
Hide comment
@ganeshmaharaj

ganeshmaharaj Feb 21, 2017

Contributor

@dachary @vasukulkarni Should have updated both threads. #13527 is the patch submitted to ceph-disk to add the default.

Contributor

ganeshmaharaj commented Feb 21, 2017

@dachary @vasukulkarni Should have updated both threads. #13527 is the patch submitted to ceph-disk to add the default.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 21, 2017

@leseb I was wrong merging this pull request. I misread #11786 (comment) and thought the tests were successfull. Instead they failed and as a result ceph-disk is now broken in master. Could you please fix this asap ?

ghost commented Feb 21, 2017

@leseb I was wrong merging this pull request. I misread #11786 (comment) and thought the tests were successfull. Instead they failed and as a result ceph-disk is now broken in master. Could you please fix this asap ?

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Feb 21, 2017

Contributor

@dachary I originally proposed the same approach as in #13527 so we should merge it.

Contributor

leseb commented Feb 21, 2017

@dachary I originally proposed the same approach as in #13527 so we should merge it.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 21, 2017

@leseb of course, entirely my fault, please accept my apologies

ghost commented Feb 21, 2017

@leseb of course, entirely my fault, please accept my apologies

@ganeshmaharaj

This comment has been minimized.

Show comment
Hide comment
@ganeshmaharaj

ganeshmaharaj Feb 21, 2017

Contributor

@dachary @leseb Thanks for the update. i believe with this,ceph/ceph-deploy#430 will be needed for ceph-deploy with work. @vasukulkarni

Contributor

ganeshmaharaj commented Feb 21, 2017

@dachary @leseb Thanks for the update. i believe with this,ceph/ceph-deploy#430 will be needed for ceph-deploy with work. @vasukulkarni

@leseb

This comment has been minimized.

Show comment
Hide comment
@leseb

leseb Feb 21, 2017

Contributor

@dachary No worries :)

Contributor

leseb commented Feb 21, 2017

@dachary No worries :)

@GregMeno

This comment has been minimized.

Show comment
Hide comment
@GregMeno

GregMeno Feb 22, 2017

Contributor

@leseb @dachary Would you please help me understand the ceph-disk suite failure here?
cheers,
G

Contributor

GregMeno commented Feb 22, 2017

@leseb @dachary Would you please help me understand the ceph-disk suite failure here?
cheers,
G

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 22, 2017

@GregMeno ceph-disk activate does not have a --cluster argument but the function implementing it expects it. It does not show in the ceph-disk suite output because the part that fails is run via either systemd or udev.

ghost commented Feb 22, 2017

@GregMeno ceph-disk activate does not have a --cluster argument but the function implementing it expects it. It does not show in the ceph-disk suite output because the part that fails is run via either systemd or udev.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment