Skip to content

Commit

Permalink
Merge pull request #10484 from dillaman/wip-16538
Browse files Browse the repository at this point in the history 
rbd-mirror: image deleter should use pool id + global image uuid for key

Reviewed-by: Mykola Golub <mgolub@mirantis.com>
  • Loading branch information
Mykola Golub committed Aug 2, 2016
2 parents 48cd11f + 87b32d1 commit 42905a6
Show file tree
Hide file tree
Showing 10 changed files with 120 additions and 65 deletions.
31 changes: 22 additions & 9 deletions src/librbd/internal.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1687,7 +1687,7 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force,
err_close_child:
c_imctx->state->close();
err_remove:
partial_r = remove(c_ioctx, c_name, no_op);
partial_r = remove(c_ioctx, c_name, "", no_op);
if (partial_r < 0) {
lderr(cct) << "Error removing failed clone: "
<< cpp_strerror(partial_r) << dendl;
Expand Down Expand Up @@ -2145,16 +2145,20 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force,
return 0;
}

int remove(IoCtx& io_ctx, const char *imgname, ProgressContext& prog_ctx,
int remove(IoCtx& io_ctx, const std::string &image_name,
const std::string &image_id, ProgressContext& prog_ctx,
bool force)
{
CephContext *cct((CephContext *)io_ctx.cct());
ldout(cct, 20) << "remove " << &io_ctx << " " << imgname << dendl;
ldout(cct, 20) << "remove " << &io_ctx << " "
<< (image_id.empty() ? image_name : image_id) << dendl;

string id;
std::string name(image_name);
std::string id(image_id);
bool old_format = false;
bool unknown_format = true;
ImageCtx *ictx = new ImageCtx(imgname, "", NULL, io_ctx, false);
ImageCtx *ictx = new ImageCtx(
(id.empty() ? name : std::string()), id, nullptr, io_ctx, false);
int r = ictx->state->open();
if (r < 0) {
ldout(cct, 2) << "error opening image: " << cpp_strerror(-r) << dendl;
Expand All @@ -2163,6 +2167,7 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force,
string header_oid = ictx->header_oid;
old_format = ictx->old_format;
unknown_format = false;
name = ictx->name;
id = ictx->id;

ictx->owner_lock.get_read();
Expand Down Expand Up @@ -2257,7 +2262,7 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force,

if (old_format || unknown_format) {
ldout(cct, 2) << "removing rbd image from v1 directory..." << dendl;
r = tmap_rm(io_ctx, imgname);
r = tmap_rm(io_ctx, name);
old_format = (r == 0);
if (r < 0 && !unknown_format) {
if (r != -ENOENT) {
Expand All @@ -2270,12 +2275,20 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force,
if (!old_format) {
if (id.empty()) {
ldout(cct, 5) << "attempting to determine image id" << dendl;
r = cls_client::dir_get_id(&io_ctx, RBD_DIRECTORY, imgname, &id);
r = cls_client::dir_get_id(&io_ctx, RBD_DIRECTORY, name, &id);
if (r < 0 && r != -ENOENT) {
lderr(cct) << "error getting id of image" << dendl;
return r;
}
} else if (name.empty()) {
ldout(cct, 5) << "attempting to determine image name" << dendl;
r = cls_client::dir_get_name(&io_ctx, RBD_DIRECTORY, id, &name);
if (r < 0 && r != -ENOENT) {
lderr(cct) << "error getting name of image" << dendl;
return r;
}
}

if (!id.empty()) {
ldout(cct, 10) << "removing journal..." << dendl;
r = Journal<>::remove(io_ctx, id);
Expand All @@ -2302,15 +2315,15 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force,
}

ldout(cct, 2) << "removing id object..." << dendl;
r = io_ctx.remove(util::id_obj_name(imgname));
r = io_ctx.remove(util::id_obj_name(name));
if (r < 0 && r != -ENOENT) {
lderr(cct) << "error removing id object: " << cpp_strerror(r)
<< dendl;
return r;
}

ldout(cct, 2) << "removing rbd image from v2 directory..." << dendl;
r = cls_client::dir_remove_image(&io_ctx, RBD_DIRECTORY, imgname, id);
r = cls_client::dir_remove_image(&io_ctx, RBD_DIRECTORY, name, id);
if (r < 0) {
if (r != -ENOENT) {
lderr(cct) << "error removing image from v2 directory: "
Expand Down
5 changes: 3 additions & 2 deletions src/librbd/internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,9 @@ namespace librbd {
int set_image_notification(ImageCtx *ictx, int fd, int type);
int is_exclusive_lock_owner(ImageCtx *ictx, bool *is_owner);

int remove(librados::IoCtx& io_ctx, const char *imgname,
ProgressContext& prog_ctx, bool force=false);
int remove(librados::IoCtx& io_ctx, const std::string &image_name,
const std::string &image_id, ProgressContext& prog_ctx,
bool force=false);
int snap_list(ImageCtx *ictx, std::vector<snap_info_t>& snaps);
int snap_exists(ImageCtx *ictx, const char *snap_name, bool *exists);
int snap_get_limit(ImageCtx *ictx, uint64_t *limit);
Expand Down
8 changes: 4 additions & 4 deletions src/librbd/librbd.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ namespace librbd {
TracepointProvider::initialize<tracepoint_traits>(get_cct(io_ctx));
tracepoint(librbd, remove_enter, io_ctx.get_pool_name().c_str(), io_ctx.get_id(), name);
librbd::NoOpProgressContext prog_ctx;
int r = librbd::remove(io_ctx, name, prog_ctx);
int r = librbd::remove(io_ctx, name, "", prog_ctx);
tracepoint(librbd, remove_exit, r);
return r;
}
Expand All @@ -377,7 +377,7 @@ namespace librbd {
{
TracepointProvider::initialize<tracepoint_traits>(get_cct(io_ctx));
tracepoint(librbd, remove_enter, io_ctx.get_pool_name().c_str(), io_ctx.get_id(), name);
int r = librbd::remove(io_ctx, name, pctx);
int r = librbd::remove(io_ctx, name, "", pctx);
tracepoint(librbd, remove_exit, r);
return r;
}
Expand Down Expand Up @@ -1786,7 +1786,7 @@ extern "C" int rbd_remove(rados_ioctx_t p, const char *name)
TracepointProvider::initialize<tracepoint_traits>(get_cct(io_ctx));
tracepoint(librbd, remove_enter, io_ctx.get_pool_name().c_str(), io_ctx.get_id(), name);
librbd::NoOpProgressContext prog_ctx;
int r = librbd::remove(io_ctx, name, prog_ctx);
int r = librbd::remove(io_ctx, name, "", prog_ctx);
tracepoint(librbd, remove_exit, r);
return r;
}
Expand All @@ -1799,7 +1799,7 @@ extern "C" int rbd_remove_with_progress(rados_ioctx_t p, const char *name,
TracepointProvider::initialize<tracepoint_traits>(get_cct(io_ctx));
tracepoint(librbd, remove_enter, io_ctx.get_pool_name().c_str(), io_ctx.get_id(), name);
librbd::CProgressContext prog_ctx(cb, cbdata);
int r = librbd::remove(io_ctx, name, prog_ctx);
int r = librbd::remove(io_ctx, name, "", prog_ctx);
tracepoint(librbd, remove_exit, r);
return r;
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/librbd/image/test_mock_RefreshRequest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessChild) {
}

librbd::NoOpProgressContext no_op;
ASSERT_EQ(0, librbd::remove(m_ioctx, clone_name.c_str(), no_op));
ASSERT_EQ(0, librbd::remove(m_ioctx, clone_name, "", no_op));
ASSERT_EQ(0, ictx->operations->snap_unprotect("snap"));
};

Expand Down
17 changes: 15 additions & 2 deletions src/test/librbd/test_internal.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ TEST_F(TestInternal, FlattenFailsToLockImage) {
parent->unlock_image();
}
librbd::NoOpProgressContext no_op;
ASSERT_EQ(0, librbd::remove(m_ioctx, clone_name.c_str(), no_op));
ASSERT_EQ(0, librbd::remove(m_ioctx, clone_name, "", no_op));
} BOOST_SCOPE_EXIT_END;

ASSERT_EQ(0, open_image(clone_name, &ictx2));
Expand Down Expand Up @@ -808,7 +808,7 @@ TEST_F(TestInternal, WriteFullCopyup) {
}

librbd::NoOpProgressContext remove_no_op;
ASSERT_EQ(0, librbd::remove(m_ioctx, clone_name.c_str(), remove_no_op));
ASSERT_EQ(0, librbd::remove(m_ioctx, clone_name, "", remove_no_op));
} BOOST_SCOPE_EXIT_END;

ASSERT_EQ(0, open_image(clone_name, &ictx2));
Expand All @@ -835,3 +835,16 @@ TEST_F(TestInternal, WriteFullCopyup) {
read_bl.c_str(), 0));
ASSERT_TRUE(bl.contents_equal(read_bl));
}

TEST_F(TestInternal, RemoveById) {
REQUIRE_FEATURE(RBD_FEATURE_LAYERING);

librbd::ImageCtx *ictx;
ASSERT_EQ(0, open_image(m_image_name, &ictx));

std::string image_id = ictx->id;
close_image(ictx);

librbd::NoOpProgressContext remove_no_op;
ASSERT_EQ(0, librbd::remove(m_ioctx, "", image_id, remove_no_op));
}
53 changes: 35 additions & 18 deletions src/test/rbd_mirror/test_ImageDeleter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class TestImageDeleter : public TestFixture {
promote_image();
}
NoOpProgressContext ctx;
int r = remove(m_local_io_ctx, m_image_name.c_str(), ctx, force);
int r = remove(m_local_io_ctx, m_image_name, "", ctx, force);
EXPECT_EQ(1, r == 0 || r == -ENOENT);
}

Expand Down Expand Up @@ -216,7 +216,8 @@ TEST_F(TestImageDeleter, Delete_NonPrimary_Image) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(0, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -232,7 +233,8 @@ TEST_F(TestImageDeleter, Fail_Delete_Primary_Image) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(-rbd::mirror::ImageDeleter::EISPRM, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -247,7 +249,8 @@ TEST_F(TestImageDeleter, Fail_Delete_Diff_GlobalId) {
m_image_name, "diff global id");

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, "diff global id",
&ctx);
EXPECT_EQ(-EINVAL, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -261,7 +264,8 @@ TEST_F(TestImageDeleter, Delete_Image_With_Child) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(0, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -276,7 +280,8 @@ TEST_F(TestImageDeleter, Delete_Image_With_Children) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(0, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -290,7 +295,8 @@ TEST_F(TestImageDeleter, Delete_Image_With_ProtectedChild) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(0, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -305,7 +311,8 @@ TEST_F(TestImageDeleter, Delete_Image_With_ProtectedChildren) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(0, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -319,7 +326,8 @@ TEST_F(TestImageDeleter, Delete_Image_With_Clone) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(-EBUSY, ctx.wait());

ASSERT_EQ(1u, m_deleter->get_delete_queue_items().size());
Expand All @@ -329,11 +337,13 @@ TEST_F(TestImageDeleter, Delete_Image_With_Clone) {
"clone1", GLOBAL_CLONE_IMAGE_ID);

C_SaferCond ctx2;
m_deleter->wait_for_scheduled_deletion("clone1", &ctx2);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_CLONE_IMAGE_ID,
&ctx2);
EXPECT_EQ(0, ctx2.wait());

C_SaferCond ctx3;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx3);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx3);
EXPECT_EQ(0, ctx3.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -352,7 +362,8 @@ TEST_F(TestImageDeleter, Delete_NonExistent_Image) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(0, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -376,7 +387,8 @@ TEST_F(TestImageDeleter, Delete_NonExistent_Image_With_MirroringState) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(0, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -392,7 +404,8 @@ TEST_F(TestImageDeleter, Delete_NonExistent_Image_Without_MirroringState) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(-ENOENT, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -410,7 +423,8 @@ TEST_F(TestImageDeleter, Fail_Delete_NonPrimary_Image) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(-EBUSY, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -430,13 +444,15 @@ TEST_F(TestImageDeleter, Retry_Failed_Deletes) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(-EBUSY, ctx.wait());

EXPECT_EQ(0, ictx->state->close());

C_SaferCond ctx2;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx2);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx2);
EXPECT_EQ(0, ctx2.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand All @@ -454,7 +470,8 @@ TEST_F(TestImageDeleter, Delete_Is_Idempotent) {
m_image_name, GLOBAL_IMAGE_ID);

C_SaferCond ctx;
m_deleter->wait_for_scheduled_deletion(m_image_name, &ctx);
m_deleter->wait_for_scheduled_deletion(m_local_pool_id, GLOBAL_IMAGE_ID,
&ctx);
EXPECT_EQ(-EBUSY, ctx.wait());

ASSERT_EQ(0u, m_deleter->get_delete_queue_items().size());
Expand Down
6 changes: 3 additions & 3 deletions src/test/rbd_mirror/test_ImageReplayer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ TEST_F(TestImageReplayer, Resync)

C_SaferCond delete_ctx;
m_image_deleter->wait_for_scheduled_deletion(
m_replayer->get_local_image_name(), &delete_ctx);
m_local_ioctx.get_id(), m_replayer->get_global_image_id(), &delete_ctx);
EXPECT_EQ(0, delete_ctx.wait());

C_SaferCond cond;
Expand Down Expand Up @@ -632,7 +632,7 @@ TEST_F(TestImageReplayer, Resync_While_Stop)

C_SaferCond delete_ctx;
m_image_deleter->wait_for_scheduled_deletion(
m_replayer->get_local_image_name(), &delete_ctx);
m_local_ioctx.get_id(), m_replayer->get_global_image_id(), &delete_ctx);
EXPECT_EQ(0, delete_ctx.wait());

C_SaferCond cond3;
Expand Down Expand Up @@ -673,7 +673,7 @@ TEST_F(TestImageReplayer, Resync_StartInterrupted)

C_SaferCond delete_ctx;
m_image_deleter->wait_for_scheduled_deletion(
m_replayer->get_local_image_name(), &delete_ctx);
m_local_ioctx.get_id(), m_replayer->get_global_image_id(), &delete_ctx);
EXPECT_EQ(0, delete_ctx.wait());

C_SaferCond cond2;
Expand Down

0 comments on commit 42905a6

Please sign in to comment.