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

rbd-mirror: support deferred deletions of mirrored images #19536

Merged
merged 16 commits into from Dec 24, 2017

Conversation

Projects
None yet
2 participants
@dillaman
Copy link
Contributor

commented Dec 14, 2017

All deleted images are first moved to the trash, and once in the trash, the image deleter will detect the new image and schedule it from removal based upon its deferment end time.

@dillaman dillaman force-pushed the dillaman:wip-rbd-mirror-trash branch from b1f20ec to 29dc8f8 Dec 14, 2017

ictx->owner_lock.put_read();
r = ictx->operations->prepare_image_update(false);
if (r < 0) {
lderr(cct) << "cannot obtain exclusive lock - not removing" << dendl;

This comment has been minimized.

Copy link
@trociny

trociny Dec 15, 2017

Contributor

nit: indentation looks wrong

@dillaman dillaman force-pushed the dillaman:wip-rbd-mirror-trash branch 2 times, most recently from ea02322 to 0cddfd0 Dec 15, 2017

@dillaman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2017

Test run (valgrind failures related to [1] and the other three are ceph-mgr unresponsive warnings): http://pulpito.ceph.com/jdillaman-2017-12-15_17:24:44-rbd-wip-rbd-mirror-trash-distro-basic-smithi/

[1] ceph/teuthology#1139

@dillaman dillaman force-pushed the dillaman:wip-rbd-mirror-trash branch from 0cddfd0 to a46a230 Dec 17, 2017

@dillaman dillaman changed the title [DNM] rbd-mirror: support deferred deletions of mirrored images rbd-mirror: support deferred deletions of mirrored images Dec 17, 2017

Mutex::Locker timer_locker(m_threads->timer_lock);
Mutex::Locker locker(m_lock);

// TODO

This comment has been minimized.

Copy link
@trociny

trociny Dec 18, 2017

Contributor

@dillaman not sure what this TODO means

This comment has been minimized.

Copy link
@dillaman

dillaman Dec 18, 2017

Author Contributor

I address it in a later commit -- at this stage, ImageDeleter uses the global_image_id but it will be switched to local_image_id

This comment has been minimized.

Copy link
@trociny

trociny Dec 18, 2017

Contributor

I still see the comment in the final version. I suppose it should be removed?

This comment has been minimized.

Copy link
@dillaman

dillaman Dec 18, 2017

Author Contributor

In that case -- definitely

*delete_info->local_io_ctx, delete_info->global_image_id,
delete_info->ignore_orphaned, &delete_info->error_result, m_work_queue,
m_local_io_ctx, delete_info->image_id, &delete_info->error_result,
m_threads->work_queue,
ctx);

This comment has been minimized.

Copy link
@trociny

trociny Dec 18, 2017

Contributor

nit: the line break is unnecessary now.

@dillaman dillaman force-pushed the dillaman:wip-rbd-mirror-trash branch from a46a230 to 1b1d942 Dec 18, 2017

@dillaman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

jenkins test docs

@trociny

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

@dillaman When testing your branch I was observing the following crash [1]:

    -1> 2017-12-18 14:04:00.226 7f876bfff700 -1 rbd::mirror::image_replayer::PrepareRemoteImageRequest: 0x7f8760047d30 handle_register_client: failed to register with remote journal: (2) No such file or directory
     0> 2017-12-18 14:04:00.262 7f872d7fa700 -1 /build/ceph-13.0.0-4120-g1316786/src/tools/rbd_mirror/ImageReplayer.cc: In function 'void rbd::mirror::ImageReplayer<ImageCtxT>::handle_shut_down(int) [with ImageCtxT = librbd::ImageCtx]' thread 7f872d7fa700 time 2017-12-18 14:04:00.236170
/build/ceph-13.0.0-4120-g1316786/src/tools/rbd_mirror/ImageReplayer.cc: 1627: FAILED assert(m_remote_image.image_id.empty())

 ceph version 13.0.0-4120-g1316786 (1316786a79c87e5dbc20017c1a58ab57516f1ec4) mimic (dev)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x102) [0x7f8775d20022]
 2: (rbd::mirror::ImageReplayer<librbd::ImageCtx>::handle_shut_down(int)+0xab8) [0x55eef625b8a8]
 3: (FunctionContext::finish(int)+0x2c) [0x55eef61fe47c]
 4: (Context::complete(int)+0x9) [0x55eef61fd239]
 5: (rbd::mirror::ImageReplayer<librbd::ImageCtx>::handle_mirror_status_update(int)+0x182) [0x55eef6259102]
 6: (librados::C_AioComplete::finish(int)+0x1d) [0x7f877e7a5e8d]
 7: (Context::complete(int)+0x9) [0x7f877e785c59]
 8: (Finisher::finisher_thread_entry()+0x370) [0x7f8775d1e560]
 9: (()+0x76ba) [0x7f877589c6ba]
 10: (clone()+0x6d) [0x7f87749253dd]
 NOTE: a copy of the executable, or `objdump -rdS <executable>` is needed to interpret this.

So it looks like due to when PrepareRemoteImageRequest returns -ENOENT, in ImageReplayer<I>::handle_prepare_remote_image we set m_delete_requested and initiate shut down, but m_remote_image at that moment contains nonempty image_id, and this triggers the assert. Right now I don't see how it is related to your PR, but I have never seen this before.

[1] http://qa-proxy.ceph.com/teuthology/trociny-2017-12-18_13:01:38-rbd-wip-mgolub-testing-distro-basic-smithi/1976263/teuthology.log

dillaman added some commits Nov 22, 2017

rbd-mirror: free memory after image deleter fails to open image
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
librbd: async trash move state machine
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
librbd: watch/notify types for trash add/remove events
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
librbd: incorporate new trash notification messages
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
rbd-mirror: use one image deleter per pool
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
librbd: async journal reset state machine
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
test/librados_test_stub: add mock version of `create`
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
rbd-mirror: avoid logging an error for expected bootstrap failures
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
rbd-mirror: watch notifications for images added to trash
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
rbd-mirror: integrate trash watcher within image deleter
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
rbd-mirror: async mirroring trash move state machine
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
rbd-mirror: new 'rbd_mirroring_delete_delay' configuration option
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
rbd-mirror: move images to trash when propagating deletions
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
rbd-mirror: image deleter now only removes images from the trash
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
rbd-mirror: only the leader should initialize the image deleter
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
rbd-mirror: propagate deletion only if remote image doesn't exist
Signed-off-by: Jason Dillaman <dillaman@redhat.com>

@dillaman dillaman force-pushed the dillaman:wip-rbd-mirror-trash branch from 1b1d942 to 2a8ab2d Dec 19, 2017

@dillaman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

@trociny Nice find -- looks like I introduced that issue w/ #16642. I append a commit to this PR to address it.

@dillaman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

jenkins test docs

@dillaman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

retest this please

@trociny

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2017

@dillaman I was also observing 'split-brain' test failures [1,2] on your yesterday branch: it expected "up+error split-brain" status but got "up+replaying". And in the rbd-mirror log there was no expected "split-brain detected" message. I don't have an explanation and failed to reproduce running locally in loop. It might be unrelated but looks a little alarming.

[1] http://qa-proxy.ceph.com/teuthology/trociny-2017-12-18_13:02:16-rbd-wip-mgolub-testing-distro-basic-ovh/1976292/teuthology.log
[2] http://qa-proxy.ceph.com/teuthology/trociny-2017-12-18_21:47:26-rbd-wip-mgolub-testing-distro-basic-smithi/1976650/teuthology.log

@dillaman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

@trociny Looking at that test case, I am surprised it ever really worked on teuthology. It would only be guaranteed to work if the cluster2 rbd-mirror daemon isn't running. Otherwise, it's possible that cluster2 rbd-mirror would be fast enough to detect cluster1 as the new primary, sync itself w/ cluster1's image, and then once (re-)promoted, cluster1 would be able to sync w/ it again. I'll open a new tracker ticket to fix that test case since it should be backported.

@trociny
Copy link
Contributor

left a comment

LGTM

@trociny

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@ceph-jenkins retest this please

@dillaman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2017

jenkins test docs

@trociny

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

@ceph-jenkins test docs

@trociny

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

@ceph-jenkins retest this please

1 similar comment
@trociny

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2017

@ceph-jenkins retest this please

@trociny trociny merged commit 8249ffd into ceph:master Dec 24, 2017

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@dillaman dillaman deleted the dillaman:wip-rbd-mirror-trash branch Dec 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.