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: add flag --yes-i-really-mean-it for setting pool size 1 #33281

Merged
merged 1 commit into from Mar 10, 2020
Merged

mon/OSDMonitor: add flag --yes-i-really-mean-it for setting pool size 1 #33281

merged 1 commit into from Mar 10, 2020

Conversation

ideepika
Copy link
Member

@ideepika ideepika commented Feb 13, 2020 •
edited

Adds option mon_allow_pool_size_one which will be disabled by default
to ensure pools are not configured without replicas.
If the user still wants to use pool size 1, they will have to change the
value of mon_allow_pool_size_one to true and then have to pass flag
--yes-i-really-really-mean-it to cli command:

Example:
ceph osd pool test set size 1 --yes-i-really-really-mean-it

Fixes: https://tracker.ceph.com/issues/44025
Signed-off-by: Deepika Upadhyay dupadhya@redhat.com

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

neha-ojha
neha-ojha reviewed Feb 13, 2020
View reviewed changes
Copy link
Member

@neha-ojha neha-ojha left a comment

Choose a reason for hiding this comment

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

can you also adapt the test cases in qa/workunits/mon/pool_ops.sh and qa/standalone/mon/health-mute.sh where we set size to 1

src/common/options.cc Outdated Show resolved Hide resolved
src/mon/OSDMonitor.cc Outdated Show resolved Hide resolved
xiexingguo
xiexingguo reviewed Feb 14, 2020
View reviewed changes
src/mon/MonCommands.h Outdated Show resolved Hide resolved
@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch from b0fb098 to db5b1d6 Compare February 14, 2020 08:17
@ideepika ideepika changed the title mon/OSDMonitor: add flag --yes-i-really-really-mean-it for setting pool size 1 mon/OSDMonitor: add flag --yes-i-really-mean-it for setting pool size 1 Feb 14, 2020
neha-ojha
neha-ojha approved these changes Feb 14, 2020
View reviewed changes
Copy link
Member

@neha-ojha neha-ojha left a comment

Choose a reason for hiding this comment

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

couple of nits, else LGTM! let's run this through testing and fix any other hidden occurrences.

PendingReleaseNotes Outdated Show resolved Hide resolved
PendingReleaseNotes Outdated Show resolved Hide resolved
neha-ojha
neha-ojha reviewed Feb 14, 2020
View reviewed changes
src/mon/OSDMonitor.cc Outdated Show resolved Hide resolved
@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch from c0c4ea6 to 80e45f1 Compare February 17, 2020 03:07
gregsfortytwo
gregsfortytwo requested changes Feb 17, 2020
View reviewed changes
Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

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

Good basics; few issues plus what xiexingguo said.

PendingReleaseNotes Outdated Show resolved Hide resolved
src/mon/OSDMonitor.cc Outdated Show resolved Hide resolved
@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch 2 times, most recently from 0fc80be to 20dfd1b Compare February 17, 2020 20:15
gregsfortytwo
gregsfortytwo approved these changes Feb 17, 2020
View reviewed changes
Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Greg Farnum gfarnum@redhat.com

@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch from 20dfd1b to d227311 Compare February 19, 2020 14:36
leseb
leseb reviewed Feb 19, 2020
View reviewed changes
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.

This change doesn't fit well with the operator model and its declarative nature but I suspect that this change is somehow desired and good for users.

@leseb leseb mentioned this pull request Feb 19, 2020
Closed
@gregsfortytwo
Copy link
Member

This change doesn't fit well with the operator model and its declarative nature but I suspect that this change is somehow desired and good for users.

Pools with no replicas is just never a good idea. It's technically possible to run but any OSD down blocks IO on its portion of data and actually losing one means both data loss and horrifying manual gyrations to try and get back what we can. Even in a cloud environment with eg EBS backing disks I can't think of any way this would work.

@leseb
Copy link
Member

leseb commented Feb 19, 2020

This change doesn't fit well with the operator model and its declarative nature but I suspect that this change is somehow desired and good for users.

Pools with no replicas is just never a good idea. It's technically possible to run but any OSD down blocks IO on its portion of data and actually losing one means both data loss and horrifying manual gyrations to try and get back what we can. Even in a cloud environment with eg EBS backing disks I can't think of any way this would work.

I couldn't agree more! Again I'm not saying it's a bad thing it's just that Rook for instead would have to suppress that if a user asks for pool size of 1 and we will only rely on the health warning.

@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch 2 times, most recently from db309b3 to 49cae07 Compare February 20, 2020 08:27
@ideepika
Copy link
Member Author

ideepika commented Feb 20, 2020 •
edited 


Rerun:  Test Run: ideepika-2020-02-20_07:04:09-rados-wip-deepika-osd-pool-set-config-2-distro-basic-smithi
=================================================================
logs:    http://qa-proxy.ceph.com/teuthology/ideepika-2020-02-20_07:04:09-rados-wip-deepika-osd-pool-set-config-2-distro-basic-smithi/
passed:  4
=================================================================
Test Run: ideepika-2020-02-17_14:13:20-rados-wip-deepika-testing-17-02-2020-distro-basic-smithi
=================================================================
logs:    http://qa-proxy.ceph.com/teuthology/ideepika-2020-02-17_14:13:20-rados-wip-deepika-testing-17-02-2020-distro-basic-smithi/
failed:  7
passed:  52

@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch 3 times, most recently from 78c687b to 4eff355 Compare February 25, 2020 13:08
tchaikov
tchaikov reviewed Feb 26, 2020
View reviewed changes
src/common/legacy_config_opts.h Outdated Show resolved Hide resolved
src/mon/OSDMonitor.cc Outdated Show resolved Hide resolved
src/mon/OSDMonitor.cc Outdated Show resolved Hide resolved
src/mon/OSDMonitor.cc Outdated Show resolved Hide resolved
@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch 2 times, most recently from d4c1937 to 0d25db9 Compare March 2, 2020 06:52
@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch 2 times, most recently from 1162f1b to 88ff18c Compare March 2, 2020 08:45
tchaikov
tchaikov approved these changes Mar 2, 2020
View reviewed changes
PendingReleaseNotes Outdated Show resolved Hide resolved
@tchaikov
Copy link
Contributor

tchaikov commented Mar 4, 2020

http://pulpito.ceph.com/kchai-2020-03-03_13:02:55-rados-wip-kefu-testing-2020-03-03-1622-distro-basic-smithi/

the failure were either

@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch from 88ff18c to 4f05c69 Compare March 5, 2020 09:55
@ideepika
Copy link
Member Author

ideepika commented Mar 5, 2020

jenkins test make check

@ideepika ideepika requested review from neha-ojha and gregsfortytwo and removed request for gregsfortytwo and neha-ojha March 5, 2020 18:16
neha-ojha
neha-ojha approved these changes Mar 6, 2020
View reviewed changes
Copy link
Member

@neha-ojha neha-ojha left a comment

Choose a reason for hiding this comment

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

As discussed in standup, let's wait until octopus is cut to merge this.

PendingReleaseNotes Outdated Show resolved Hide resolved
@gregsfortytwo
Copy link
Member

Still looks good; 👍 on waiting until Octopus I guess.

@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch from e8bda73 to c13f1a6 Compare March 7, 2020 08:07
mon/OSDMonitor: add flag --yes-i-really-mean-it for setting pool si…
21508bd 
…ze 1

Adds option `mon_allow_pool_size_one` which will be disabled by default
to ensure pools are not configured without replicas.
If the user still wants to use pool size 1, they will have to change the
value of `mon_allow_pool_size_one` to true and then have to pass flag
`--yes-i-really-mean-it` to cli command:

Example:
`ceph osd pool test set size 1 --yes-i-really-mean-it`

Fixes: https://tracker.ceph.com/issues/44025
Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
@ideepika ideepika force-pushed the wip-set-osd-pool-size-extra-param-check branch from c13f1a6 to 21508bd Compare March 9, 2020 17:59
@neha-ojha neha-ojha merged commit 6117a0d into ceph:master Mar 10, 2020
@smithfarm smithfarm mentioned this pull request Apr 12, 2020
Merged
@ideepika ideepika deleted the wip-set-osd-pool-size-extra-param-check branch April 29, 2021 05:41
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

@leseb leseb leseb left review comments

@xiexingguo xiexingguo xiexingguo left review comments

@gregsfortytwo gregsfortytwo gregsfortytwo approved these changes

@neha-ojha neha-ojha neha-ojha approved these changes

Assignees
No one assigned
Labels
core feature needs-qa
Projects
None yet
Milestone
No milestone
6 participants
@ideepika @gregsfortytwo @leseb @tchaikov @xiexingguo @neha-ojha