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

mon/OSDMonitor.cc : set erasure-code-profile to "" when create replicated pools. #19673

Merged
merged 1 commit into from Jan 15, 2018
Merged

mon/OSDMonitor.cc : set erasure-code-profile to "" when create replicated pools. #19673

merged 1 commit into from Jan 15, 2018

Conversation

zouaiguo
Copy link
Contributor

@zouaiguo zouaiguo commented Dec 25, 2017 •
edited

when we create a pool specify a rule, for example "ceph osd pool create foo replicated 10 rule_foo",
we will set pool foo erasure-code-profile to rule_foo,
if there has an erasure-code-profile names rule_foo, use "ceph osd erasure-code-profile rule_foo" will fail,
"Error EBUSY: foo pool(s) are using the erasure code profile 'rule_foo'", this is wrong.

we should do:

  1. set erasure-code-profile to "" when create replicated pools
  2. whether erasure-code-profile is used by pool not only judge pool erasure_code_profile property and also the pool is_erasure

Signed-off-by: zouaiguo zou.aiguo@zte.com.cn

@zouaiguo zouaiguo changed the title ;set erase-code-profile to "" when create replicated pools. set erase-code-profile to "" when create replicated pools. Dec 25, 2017
@zouaiguo zouaiguo changed the title set erase-code-profile to "" when create replicated pools. mon/OSDMonitor.cc : set erase-code-profile to "" when create replicated pools. Dec 25, 2017
@shinobu-x
Copy link
Contributor

@zouaiguo please replace erase-code-profile with erasure-code-profile in this PR.

@zouaiguo zouaiguo changed the title mon/OSDMonitor.cc : set erase-code-profile to "" when create replicated pools. mon/OSDMonitor.cc : set erasure-code-profile to "" when create replicated pools. Dec 25, 2017
@zouaiguo
Copy link
Contributor Author

OK, I have changed

jecluis
jecluis requested changes Dec 25, 2017
View reviewed changes
Copy link
Member

@jecluis jecluis left a comment

Choose a reason for hiding this comment

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

While you are at it, please also ensure you validate the case when the pool is not erasure and yet the user is trying to set an erasude-code profile on it, and make sure to throw an error at the user. This should be done somewhere in (or after) https://github.com/ceph/ceph/pull/19673/files#diff-0a5db46a44ae9900e226289a810f10e8R10149 but before you change any state.

Also, please add a test to validate not only the erroneous case (before this patch) and ensure it no longer happens, as well as a case for the success of this patch. I recommend doing that on qa/workunits/cephtool/test.sh.

@jecluis
Copy link
Member

jecluis commented Dec 25, 2017

Also, please set commit title to mon/OSDMonitor.cc : set erasure-code-profile to "" when create replicated pools as you have done for the pull request.

@zouaiguo
Copy link
Contributor Author

While you are at it, please also ensure you validate the case when the pool is not erasure and yet the user is trying to set an erasude-code profile on it, and make sure to throw an error at the user. This should be done somewhere in (or after) https://github.com/ceph/ceph/pull/19673/files#diff-0a5db46a44ae9900e226289a810f10e8R10149 but before you change any state.

@jecluis

there has a little bug or misunderstand in create a replicate pool
we should use 'erasure_code_profile' parameter as rule name to create replicate pool, not 'rule' parameter .

python code:

prefix = "osd pool create"
para_dict = {
        'pool': 'foo',
        'pool_type': 'replicated',
        'erasure_code_profile': 'rule_foo',
        'pg_num': 128
    }

so erasure_code_profile is required when use a specify rule to create a replicated pool

@zouaiguo zouaiguo force-pushed the wip_erase_code branch 3 times, most recently from fa4bcac to 0aaa667 Compare December 26, 2017 03:32
@zouaiguo
Copy link
Contributor Author 

root@s222:/home/ceph-build/ceph-dev/build# ./bin/ceph osd erasure-code-profile set foo foo
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-12-26 11:34:21.053 7f37dcfc9700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-26 11:34:21.077 7f37dcfc9700 -1 WARNING: all dangerous and experimental features are enabled.
root@s222:/home/ceph-build/ceph-dev/build#
root@s222:/home/ceph-build/ceph-dev/build# ./bin/ceph osd erasure-code-profile ls |grep foo
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-12-26 11:34:29.041 7ff16749b700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-26 11:34:29.057 7ff16749b700 -1 WARNING: all dangerous and experimental features are enabled.
foo
root@s222:/home/ceph-build/ceph-dev/build#
root@s222:/home/ceph-build/ceph-dev/build# ./bin/ceph osd crush rule create-replicated foo default host
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-12-26 11:34:35.764 7f5fd4ac4700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-26 11:34:35.784 7f5fd4ac4700 -1 WARNING: all dangerous and experimental features are enabled.
root@s222:/home/ceph-build/ceph-dev/build#
root@s222:/home/ceph-build/ceph-dev/build# ./bin/ceph osd pool create replicated 12 12 replicated foo
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-12-26 11:34:44.164 7f98f1532700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-26 11:34:44.188 7f98f1532700 -1 WARNING: all dangerous and experimental features are enabled.
pool 'replicated' created
root@s222:/home/ceph-build/ceph-dev/build#
root@s222:/home/ceph-build/ceph-dev/build# ./bin/ceph osd erasure-code-profile rm foo
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-12-26 11:34:55.107 7f5093953700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-26 11:34:55.127 7f5093953700 -1 WARNING: all dangerous and experimental features are enabled.
root@s222:/home/ceph-build/ceph-dev/build#
root@s222:/home/ceph-build/ceph-dev/build# ./bin/ceph osd pool delete replicated replicated --yes-i-really-really-mean-it
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-12-26 11:35:26.410 7fea3ff48700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-26 11:35:26.430 7fea3ff48700 -1 WARNING: all dangerous and experimental features are enabled.
pool 'replicated' removed
root@s222:/home/ceph-build/ceph-dev/build#
root@s222:/home/ceph-build/ceph-dev/build# ./bin/ceph osd crush rule rm foo
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-12-26 11:35:35.682 7fed27f5b700 -1 WARNING: all dangerous and experimental features are enabled.
2017-12-26 11:35:35.706 7fed27f5b700 -1 WARNING: all dangerous and experimental features are enabled.
root@s222:/home/ceph-build/ceph-dev/build#

@zouaiguo
Copy link
Contributor Author

zouaiguo commented Jan 3, 2018 •
edited

@jecluis can you review it for me?

@tchaikov tchaikov self-requested a review January 7, 2018 03:40
tchaikov
tchaikov reviewed Jan 8, 2018
View reviewed changes
qa/workunits/cephtool/test.sh Outdated
ceph osd pool create replicated 12 12 replicated foo
ceph osd erasure-code-profile rm foo
ceph osd pool delete replicated replicated --yes-i-really-really-mean-it
ceph osd crush rule rm foo
Copy link
Contributor

Choose a reason for hiding this comment

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

i think the test below covers this test already.

tchaikov
tchaikov reviewed Jan 8, 2018
View reviewed changes
src/mon/OSDMonitor.cc Outdated
@@ -10194,6 +10194,7 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op,
} else {
//NOTE:for replicated pool,cmd_map will put rule_name to erasure_code_profile field
rule_name = erasure_code_profile;
erasure_code_profile = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

could you update OSDMonitor::prepare_new_pool() also ? so we don't update pi->erasure_code_profile with the given erasure_code_profile if pool_type == pg_pool_t::TYPE_REPLICATED?

@zouaiguo
Copy link
Contributor Author

zouaiguo commented Jan 9, 2018

@tchaikov i have modified as your mention

@zouaiguo
mon/OSDMonitor.cc : set erasure-code-profile to "" when create replic…
a61e62c 
…ated pools.

when we create a pool specify a rule, for example "ceph osd pool create foo replicated 10 rule_foo",
we will set pool foo erasure-code-profile to rule_foo,
if there has an erasure-code-profile names rule_foo, use "ceph osd erasure-code-profile rule_foo" will fail,
"Error EBUSY: foo pool(s) are using the erasure code profile 'rule_foo'", this is wrong.

we should do:
1. set erasure-code-profile to "" when create replicated pools
2. whether erasure-code-profile is used by pool not only judge pool erasure_code_profile property and also the pool is_erasure

Signed-off-by: zouaiguo <zou.aiguo@zte.com.cn>
@tchaikov
Copy link
Contributor

tchaikov commented Jan 9, 2018

hey @jecluis does the latest change address your comment?

@tchaikov
Copy link
Contributor

failed tests are tracked by

@tchaikov
Copy link
Contributor

@jecluis could you re-review this PR?

@tchaikov tchaikov merged commit 0c38eb8 into ceph:master Jan 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

@jecluis jecluis jecluis approved these changes

Assignees

@jecluis jecluis

Labels
bug-fix mon needs-review needs-test
Projects
None yet
Milestone
No milestone
4 participants
@zouaiguo @shinobu-x @jecluis @tchaikov