Removal of snapshot with corrupt replica crashes osd #22476

dzafman · 2018-06-08T17:52:37Z

This is only a partial fix for http://tracker.ceph.com/issues/23875

dzafman · 2018-06-09T18:21:01Z

@liewegas @jdurgin @tchaikov @neha-ojha I created this pull request to start the discussion on taking some asserts out.

gregsfortytwo

The OSD code change looks good to me, but I'm not clear on the failure which is still in the ticket.

gregsfortytwo · 2018-07-11T21:18:19Z

qa/standalone/scrub/osd-scrub-snaps.sh

@@ -90,57 +90,58 @@ function create_scenario() {
    # Don't need to use ceph_objectstore_tool() function because osd stopped

    JSON="$(ceph-objectstore-tool --data-path $dir/${osd} --head --op list obj1)"
-    ceph-objectstore-tool --data-path $dir/${osd} "$JSON" --force remove
+    ceph-objectstore-tool --data-path $dir/${osd} "$JSON" --force remove || return 1


Why do we want to tolerate an error in every one of these? Do we expect stale data we might need to clean up?

Actually, this change causes the script to not tolerate errors. Before I just assumed that these commands can not fail, so didn't bother to check.

Oh dur.
👍

dzafman · 2018-07-11T21:38:11Z

@gregsfortytwo Even with this change a replica can still crash when removing snapshots with certain corruptions. It isn't clear that we shouldn't consider some corruptions as fatal anyway.

    # When removing snapshots with a corrupt replica, it crashes.
    # See http://tracker.ceph.com/issues/23875
    if [ $which = "primary" ];
    then
        for i in `seq 1 7`
        do
            rados -p $poolname rmsnap snap$i
        done
    fi

tchaikov · 2018-07-16T15:02:41Z

will review this PR early tomorrow.

tchaikov · 2018-07-17T05:17:26Z

src/tools/ceph_objectstore_tool.cc

@@ -1981,25 +1981,36 @@ int do_meta(ObjectStore *store, string object, Formatter *formatter, bool debug,
  return 0;
 }

+enum rmtype {
+  STANDARD,


so

STANDARD for removing both snap from snapmapper and the object

SNAPMAP for removing the snap from snapmapper only

NOSNAPMAP for remove the object only

i'd suggest rename STANDARD to BOTH, NOSNAPMAP to OBJECT, like

enum class Remove { BOTH, SNAPMAP, OBJECT };

and update remove_object() like,

if (type == Remove::BOTH || type == Remove::SNAPMAP) { // remove snap from snapmap } if (type == Remove::BOTH || type == Remove::OBJECT) { // remove the object }

easier to digest this way, IMHO.

dzafman · 2018-07-17T20:44:07Z

@tchaikov I rebased this pull request and made the change you requested.

dzafman · 2018-07-17T20:59:51Z

retest this please

dzafman · 2018-07-18T02:32:49Z

retest this please

gregsfortytwo · 2018-07-20T21:41:41Z

@dzafman I think you need to rebase this on master again to get in the WITH_SEASTAR fix.

tchaikov · 2018-07-21T02:57:02Z

@gregsfortytwo jenkins always tries to rebase a PR to be tested against the latest master at that moment. so what we need is just:

retest this please

=)

jdurgin · 2018-10-29T22:46:52Z

@dzafman is this ready to go after a rebase?

gregsfortytwo · 2018-10-29T22:46:55Z

@dzafman, I gather we should go through and re-review this now?

Note that the final FIXUP commit is missing a signoff statement.

dzafman · 2018-10-29T23:24:27Z

@gregsfortytwo Yes, I'll squash, rebase and make sure test passes.

My using nop() instead of setattr() we avoid crashing a replica that is missing the object for some reason. Although this is a corruption, it would be getting "fix" by removal of the snapshot. In the other cases we accept completely missing keys (ENOENT) that happens when an object is removed somehow or missed by recovery/backfill. Signed-off-by: David Zafman <dzafman@redhat.com>

…ting Signed-off-by: David Zafman <dzafman@redhat.com>

…e_scenario() Signed-off-by: David Zafman <dzafman@redhat.com>

…re-tool Use --rmtype snapmap with new obj16 to remove snapmap only, check for repair message Use --rmtype nosnapmap to remove obj5 while leaving snapmap behind Signed-off-by: David Zafman <dzafman@redhat.com>

… complete Due to deliberate corruptions snaptrim_error means snaptrim is done Signed-off-by: David Zafman <dzafman@redhat.com>

dzafman · 2018-11-09T21:50:36Z

retest this please

dzafman · 2018-11-09T21:54:53Z

Rados test suite passed with unrelated failures:
dzafman-2018-11-08_20:13:57-rados-wip-zafman-testing-22476-distro-basic-smithi
3238374 http://tracker.ceph.com/issues/20798. [ FAILED ] LibRadosLockECPP.LockExclusiveDurPP
3238479 /usr/include/rados/buffer.h:657:61: error: expected string-literal before ‘)’ token
3238552 scrub_test: assert len(objs['inconsistents']) == 1
3238596 rados/test_librados_build.sh: /usr/bin/x86_64-linux-gnu-ld: cannot find -lradospp failed

dzafman · 2018-11-09T22:03:02Z

@jdurgin @gregsfortytwo @tchaikov This is ready for a final review and merge. I rebased, made changes suggested by Kefu and passed the rados suite.

dzafman added cleanup core tests DNM labels Jun 9, 2018

gregsfortytwo reviewed Jul 11, 2018

View reviewed changes

dzafman added wip-zafman-testing needs-test and removed DNM labels Jul 13, 2018

dzafman changed the title ~~DNM: Removal of snapshot with corrupt replica crashes osd~~ Removal of snapshot with corrupt replica crashes osd Jul 13, 2018

dzafman requested review from tchaikov and jdurgin July 13, 2018 18:15

tchaikov reviewed Jul 17, 2018

View reviewed changes

dzafman force-pushed the wip-23875 branch from 1c6077f to f0eda51 Compare July 17, 2018 20:31

dzafman added DNM and removed wip-zafman-testing labels Jul 26, 2018

dzafman force-pushed the wip-23875 branch from f0eda51 to 4a3d57b Compare September 5, 2018 00:15

dzafman removed DNM needs-test labels Oct 29, 2018

dzafman force-pushed the wip-23875 branch from 4a3d57b to 862ae37 Compare October 30, 2018 14:38

dzafman added the wip-zafman-testing label Oct 30, 2018

dzafman added 5 commits November 7, 2018 14:23

ceph-objectstore-tool: Add removal options to corrupt objects for tes…

00e669a

…ting Signed-off-by: David Zafman <dzafman@redhat.com>

test: cleanup: Remove redundant cat of log and handle errors in creat…

f43faf4

…e_scenario() Signed-off-by: David Zafman <dzafman@redhat.com>

test: osd-scrub-snaps.sh: Testing with new --rmtype in ceph-objectsto…

e37f95a

…re-tool Use --rmtype snapmap with new obj16 to remove snapmap only, check for repair message Use --rmtype nosnapmap to remove obj5 while leaving snapmap behind Signed-off-by: David Zafman <dzafman@redhat.com>

test: osd-scrub-snaps.sh: After snapshot removal wait for snaptrim to…

a159f16

… complete Due to deliberate corruptions snaptrim_error means snaptrim is done Signed-off-by: David Zafman <dzafman@redhat.com>

dzafman force-pushed the wip-23875 branch from 862ae37 to a159f16 Compare November 9, 2018 02:21

dzafman added needs-review and removed wip-zafman-testing labels Nov 9, 2018

jdurgin approved these changes Nov 9, 2018

View reviewed changes

jdurgin merged commit fd2a4c5 into ceph:master Nov 9, 2018

dzafman deleted the wip-23875 branch September 3, 2019 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removal of snapshot with corrupt replica crashes osd #22476

Removal of snapshot with corrupt replica crashes osd #22476

dzafman commented Jun 8, 2018

dzafman commented Jun 9, 2018

gregsfortytwo left a comment

gregsfortytwo Jul 11, 2018

dzafman Jul 11, 2018

gregsfortytwo Jul 11, 2018

dzafman commented Jul 11, 2018

tchaikov commented Jul 16, 2018

tchaikov Jul 17, 2018 •

edited

Loading

dzafman commented Jul 17, 2018

dzafman commented Jul 17, 2018

dzafman commented Jul 18, 2018

gregsfortytwo commented Jul 20, 2018

tchaikov commented Jul 21, 2018 •

edited

Loading

jdurgin commented Oct 29, 2018

gregsfortytwo commented Oct 29, 2018

dzafman commented Oct 29, 2018

dzafman commented Nov 9, 2018

dzafman commented Nov 9, 2018

dzafman commented Nov 9, 2018

Removal of snapshot with corrupt replica crashes osd #22476

Removal of snapshot with corrupt replica crashes osd #22476

Conversation

dzafman commented Jun 8, 2018

dzafman commented Jun 9, 2018

gregsfortytwo left a comment

Choose a reason for hiding this comment

gregsfortytwo Jul 11, 2018

Choose a reason for hiding this comment

dzafman Jul 11, 2018

Choose a reason for hiding this comment

gregsfortytwo Jul 11, 2018

Choose a reason for hiding this comment

dzafman commented Jul 11, 2018

tchaikov commented Jul 16, 2018

tchaikov Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

dzafman commented Jul 17, 2018

dzafman commented Jul 17, 2018

dzafman commented Jul 18, 2018

gregsfortytwo commented Jul 20, 2018

tchaikov commented Jul 21, 2018 • edited Loading

jdurgin commented Oct 29, 2018

gregsfortytwo commented Oct 29, 2018

dzafman commented Oct 29, 2018

dzafman commented Nov 9, 2018

dzafman commented Nov 9, 2018

dzafman commented Nov 9, 2018

tchaikov Jul 17, 2018 •

edited

Loading

tchaikov commented Jul 21, 2018 •

edited

Loading