From 504f4e78ef51fd964e6f7cb89993c1c50646cc1a Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Thu, 10 Jan 2019 09:43:25 -0500 Subject: [PATCH] librbd: tweaked trash purge API Removed the internal usage of the public librbd API, avoided double-opening image to determine data pool, and fixed issues with the unit test case. Signed-off-by: Jason Dillaman --- src/librbd/api/Trash.cc | 83 +++++++++++-------- src/pybind/rbd/rbd.pyx | 11 ++- .../librados_test_stub/TestRadosClient.cc | 18 ++++ src/test/librbd/test_librbd.cc | 71 +++++----------- src/test/pybind/test_rbd.py | 15 +--- 5 files changed, 100 insertions(+), 98 deletions(-) diff --git a/src/librbd/api/Trash.cc b/src/librbd/api/Trash.cc index fdc2d03a6430..89fa4565777a 100644 --- a/src/librbd/api/Trash.cc +++ b/src/librbd/api/Trash.cc @@ -14,6 +14,7 @@ #include "librbd/Operations.h" #include "librbd/TrashWatcher.h" #include "librbd/Utils.h" +#include "librbd/api/DiffIterate.h" #include "librbd/api/Image.h" #include "librbd/mirror/DisableRequest.h" #include "librbd/mirror/EnableRequest.h" @@ -275,8 +276,7 @@ template int Trash::purge(IoCtx& io_ctx, time_t expire_ts, float threshold, ProgressContext& pctx) { auto *cct((CephContext *) io_ctx.cct()); - - librbd::RBD rbd; + ldout(cct, 20) << &io_ctx << dendl; std::vector trash_entries; int r = librbd::api::Trash<>::list(io_ctx, trash_entries); @@ -290,7 +290,7 @@ int Trash::purge(IoCtx& io_ctx, time_t expire_ts, } ); - std::vector to_be_removed; + std::set to_be_removed; if (threshold != -1) { if (threshold < 0 || threshold > 1) { lderr(cct) << "argument 'threshold' is out of valid range" @@ -318,7 +318,7 @@ int Trash::purge(IoCtx& io_ctx, time_t expire_ts, double pool_percent_used = 0; uint64_t pool_total_bytes = 0; - std::map> datapools; + std::map> datapools; std::sort(trash_entries.begin(), trash_entries.end(), [](librbd::trash_image_info_t a, librbd::trash_image_info_t b) { @@ -327,12 +327,16 @@ int Trash::purge(IoCtx& io_ctx, time_t expire_ts, ); for (const auto &entry : trash_entries) { - librbd::Image image; - std::string data_pool; - r = rbd.open_by_id_read_only(io_ctx, image, entry.id.c_str(), NULL); - if (r < 0) continue; + int64_t data_pool_id = -1; + r = cls_client::get_data_pool(&io_ctx, util::header_name(entry.id), + &data_pool_id); + if (r < 0 && r != -ENOENT && r != -EOPNOTSUPP) { + lderr(cct) << "failed to query data pool: " << cpp_strerror(r) << dendl; + return r; + } else if (data_pool_id == -1) { + data_pool_id = io_ctx.get_id(); + } - int64_t data_pool_id = image.get_data_pool_id(); if (data_pool_id != io_ctx.get_id()) { librados::IoCtx data_io_ctx; r = util::create_ioctx(io_ctx, "image", data_pool_id, @@ -341,10 +345,10 @@ int Trash::purge(IoCtx& io_ctx, time_t expire_ts, lderr(cct) << "error accessing data pool" << dendl; continue; } - data_pool = data_io_ctx.get_pool_name(); - datapools[data_pool].push_back(entry.id.c_str()); + auto data_pool = data_io_ctx.get_pool_name(); + datapools[data_pool].push_back(entry.id); } else { - datapools[pool_name].push_back(entry.id.c_str()); + datapools[pool_name].push_back(entry.id); } } @@ -367,27 +371,41 @@ int Trash::purge(IoCtx& io_ctx, time_t expire_ts, auto bytes_threshold = (uint64_t) (pool_total_bytes * (pool_percent_used - threshold)); - librbd::Image curr_img; for (const auto &it : img->second) { - r = rbd.open_by_id_read_only(io_ctx, curr_img, it, NULL); - if (r < 0) continue; - - uint64_t img_size; - curr_img.size(&img_size); - r = curr_img.diff_iterate2(nullptr, 0, img_size, false, true, - [](uint64_t offset, size_t len, int exists, void *arg) { + auto ictx = new ImageCtx("", it, nullptr, io_ctx, false); + r = ictx->state->open(OPEN_FLAG_SKIP_OPEN_PARENT); + if (r == -ENOENT) { + continue; + } else if (r < 0) { + lderr(cct) << "failed to open image " << it << ": " + << cpp_strerror(r) << dendl; + } + + r = librbd::api::DiffIterate::diff_iterate( + ictx, cls::rbd::UserSnapshotNamespace(), nullptr, 0, ictx->size, + false, true, + [](uint64_t offset, size_t len, int exists, void *arg) { auto *to_free = reinterpret_cast(arg); if (exists) (*to_free) += len; return 0; - }, &bytes_to_free - ); - if (r < 0) continue; - to_be_removed.push_back(it); - if (bytes_to_free >= bytes_threshold) break; + }, &bytes_to_free); + + ictx->state->close(); + if (r < 0) { + lderr(cct) << "failed to calculate disk usage for image " << it + << ": " << cpp_strerror(r) << dendl; + continue; + } + + to_be_removed.insert(it); + if (bytes_to_free >= bytes_threshold) { + break; + } } } } + if (bytes_to_free == 0) { ldout(cct, 10) << "pool usage is lower than or equal to " << (threshold * 100) @@ -404,7 +422,7 @@ int Trash::purge(IoCtx& io_ctx, time_t expire_ts, for (const auto &entry : trash_entries) { if (expire_ts >= entry.deferment_end_time) { - to_be_removed.push_back(entry.id.c_str()); + to_be_removed.insert(entry.id); } } @@ -415,14 +433,13 @@ int Trash::purge(IoCtx& io_ctx, time_t expire_ts, if (r < 0) { if (r == -ENOTEMPTY) { ldout(cct, 5) << "image has snapshots - these must be deleted " - << "with 'rbd snap purge' before the image can be removed." - << dendl; + << "with 'rbd snap purge' before the image can be " + << "removed." << dendl; } else if (r == -EBUSY) { - ldout(cct, 5) << "error: image still has watchers" - << std::endl - << "This means the image is still open or the client using " - << "it crashed. Try again after closing/unmapping it or " - << "waiting 30s for the crashed client to timeout." + ldout(cct, 5) << "error: image still has watchers" << std::endl + << "This means the image is still open or the client " + << "using it crashed. Try again after closing/unmapping " + << "it or waiting 30s for the crashed client to timeout." << dendl; } else if (r == -EMLINK) { ldout(cct, 5) << "Remove the image from the group and try again." diff --git a/src/pybind/rbd/rbd.pyx b/src/pybind/rbd/rbd.pyx index d91de1b72963..dff5fc190ada 100644 --- a/src/pybind/rbd/rbd.pyx +++ b/src/pybind/rbd/rbd.pyx @@ -29,6 +29,7 @@ except ImportError: from collections import Iterable from datetime import datetime from itertools import chain +import time cimport rados @@ -1247,7 +1248,7 @@ class RBD(object): if ret != 0: raise make_ex(ret, 'error moving image to trash') - def trash_purge(self, ioctx, expire_ts=datetime.now(), threshold=-1): + def trash_purge(self, ioctx, expire_ts=None, threshold=-1): """ Delete RBD images from trash in bulk. @@ -1265,11 +1266,15 @@ class RBD(object): :param threshold: percentage of pool usage to be met (0 to 1) :type threshold: float """ - as_time_t = int((expire_ts - datetime.utcfromtimestamp(0)).total_seconds()) + if expire_ts: + expire_epoch_ts = time.mktime(expire_ts.timetuple()) + else: + expire_epoch_ts = 0 + cdef: rados_ioctx_t _ioctx = convert_ioctx(ioctx) + time_t _expire_ts = expire_epoch_ts float _threshold = threshold - time_t _expire_ts = as_time_t with nogil: ret = rbd_trash_purge(_ioctx, _expire_ts, _threshold) if ret != 0: diff --git a/src/test/librados_test_stub/TestRadosClient.cc b/src/test/librados_test_stub/TestRadosClient.cc index 65c011fbda11..fcd5099af281 100644 --- a/src/test/librados_test_stub/TestRadosClient.cc +++ b/src/test/librados_test_stub/TestRadosClient.cc @@ -12,6 +12,7 @@ #include #include +#include static int get_concurrency() { int concurrency = 0; @@ -168,6 +169,23 @@ int TestRadosClient::mon_command(const std::vector& cmd, return 0; } else if ((*j_it)->get_data() == "config-key rm") { return 0; + } else if ((*j_it)->get_data() == "df") { + std::stringstream str; + str << R"({"pools": [)"; + + std::list> pools; + pool_list(pools); + for (auto& pool : pools) { + if (pools.begin()->first != pool.first) { + str << ","; + } + str << R"({"name": ")" << pool.second << R"(", "stats": )" + << R"({"percent_used": 1.0, "bytes_used": 0, "max_avail": 0}})"; + } + + str << "]}"; + outbl->append(str.str()); + return 0; } } return -ENOSYS; diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc index 13ff63b9367d..4fd5f18ef6ad 100644 --- a/src/test/librbd/test_librbd.cc +++ b/src/test/librbd/test_librbd.cc @@ -6594,6 +6594,8 @@ TEST_F(TestLibRBD, DefaultFeatures) { } TEST_F(TestLibRBD, TestTrashMoveAndPurge) { + REQUIRE_FORMAT_V2(); + librados::IoCtx ioctx; ASSERT_EQ(0, _rados.ioctx_create(m_pool_name.c_str(), ioctx)); @@ -6606,14 +6608,7 @@ TEST_F(TestLibRBD, TestTrashMoveAndPurge) { librbd::Image image; ASSERT_EQ(0, rbd.open(ioctx, image, name.c_str(), nullptr)); - uint8_t old_format; - ASSERT_EQ(0, image.old_format(&old_format)); - if (old_format) { - ASSERT_EQ(-EOPNOTSUPP, rbd.trash_move(ioctx, name.c_str(), 0)); - image.close(); - return; - } std::string image_id; ASSERT_EQ(0, image.get_id(&image_id)); image.close(); @@ -6645,6 +6640,8 @@ TEST_F(TestLibRBD, TestTrashMoveAndPurge) { } TEST_F(TestLibRBD, TestTrashMoveAndPurgeNonExpiredDelay) { + REQUIRE_FORMAT_V2(); + librados::IoCtx ioctx; ASSERT_EQ(0, _rados.ioctx_create(m_pool_name.c_str(), ioctx)); @@ -6657,14 +6654,7 @@ TEST_F(TestLibRBD, TestTrashMoveAndPurgeNonExpiredDelay) { librbd::Image image; ASSERT_EQ(0, rbd.open(ioctx, image, name.c_str(), nullptr)); - uint8_t old_format; - ASSERT_EQ(0, image.old_format(&old_format)); - if (old_format) { - ASSERT_EQ(-EOPNOTSUPP, rbd.trash_move(ioctx, name.c_str(), 0)); - image.close(); - return; - } std::string image_id; ASSERT_EQ(0, image.get_id(&image_id)); image.close(); @@ -6681,6 +6671,8 @@ TEST_F(TestLibRBD, TestTrashMoveAndPurgeNonExpiredDelay) { } TEST_F(TestLibRBD, TestTrashPurge) { + REQUIRE_FORMAT_V2(); + librados::IoCtx ioctx; ASSERT_EQ(0, _rados.ioctx_create(m_pool_name.c_str(), ioctx)); @@ -6695,14 +6687,7 @@ TEST_F(TestLibRBD, TestTrashPurge) { librbd::Image image1; ASSERT_EQ(0, rbd.open(ioctx, image1, name1.c_str(), nullptr)); - uint8_t old_format; - ASSERT_EQ(0, image1.old_format(&old_format)); - if (old_format) { - ASSERT_EQ(-EOPNOTSUPP, rbd.trash_move(ioctx, name1.c_str(), 0)); - image1.close(); - return; - } std::string image_id1; ASSERT_EQ(0, image1.get_id(&image_id1)); image1.close(); @@ -6711,13 +6696,10 @@ TEST_F(TestLibRBD, TestTrashPurge) { librbd::Image image2; ASSERT_EQ(0, rbd.open(ioctx, image2, name2.c_str(), nullptr)); - ASSERT_EQ(0, image2.old_format(&old_format)); + ceph::bufferlist bl; + bl.append(std::string(1024, '0')); + ASSERT_EQ(1024, image2.write(0, 1024, bl)); - if (old_format) { - ASSERT_EQ(-EOPNOTSUPP, rbd.trash_move(ioctx, name2.c_str(), 0)); - image2.close(); - return; - } std::string image_id2; ASSERT_EQ(0, image2.get_id(&image_id2)); image2.close(); @@ -6727,29 +6709,27 @@ TEST_F(TestLibRBD, TestTrashPurge) { std::vector entries; ASSERT_EQ(0, rbd.trash_list(ioctx, entries)); - ASSERT_FALSE(entries.empty()); - bool found = false; - for(auto& entry : entries) { - if (entry.id == image_id1 && entry.name == name1) - found = true; - } - ASSERT_FALSE(found); + ASSERT_EQ(1U, entries.size()); + ASSERT_EQ(image_id2, entries[0].id); + ASSERT_EQ(name2, entries[0].name); entries.clear(); struct timespec now; clock_gettime(CLOCK_REALTIME, &now); - ASSERT_EQ(0, rbd.trash_purge(ioctx, now.tv_sec+1000, 0)); - ASSERT_EQ(0, rbd.trash_list(ioctx, entries)); - - found = false; - for(auto& entry : entries) { - if (entry.id == image_id2 && entry.name == name2) - found = true; + float threshold = 0.0; + if (!is_librados_test_stub(_rados)) { + // real cluster usage reports have a long latency to update + threshold = -1.0; } - ASSERT_FALSE(found); + + ASSERT_EQ(0, rbd.trash_purge(ioctx, now.tv_sec+1000, threshold)); + ASSERT_EQ(0, rbd.trash_list(ioctx, entries)); + ASSERT_EQ(0U, entries.size()); } TEST_F(TestLibRBD, TestTrashMoveAndRestore) { + REQUIRE_FORMAT_V2(); + librados::IoCtx ioctx; ASSERT_EQ(0, _rados.ioctx_create(m_pool_name.c_str(), ioctx)); @@ -6762,14 +6742,7 @@ TEST_F(TestLibRBD, TestTrashMoveAndRestore) { librbd::Image image; ASSERT_EQ(0, rbd.open(ioctx, image, name.c_str(), nullptr)); - uint8_t old_format; - ASSERT_EQ(0, image.old_format(&old_format)); - if (old_format) { - ASSERT_EQ(-EOPNOTSUPP, rbd.trash_move(ioctx, name.c_str(), 0)); - image.close(); - return; - } std::string image_id; ASSERT_EQ(0, image.get_id(&image_id)); image.close(); diff --git a/src/test/pybind/test_rbd.py b/src/test/pybind/test_rbd.py index f858c2b910b8..57aaf7c8cf57 100644 --- a/src/test/pybind/test_rbd.py +++ b/src/test/pybind/test_rbd.py @@ -1874,24 +1874,13 @@ class TestTrash(object): image_name2 = image_name image_id2 = image.id() - create_image() - with Image(ioctx, image_name) as image: - image_name3 = image_name - image_id3 = image.id() - RBD().trash_move(ioctx, image_name1, 0) RBD().trash_move(ioctx, image_name2, 1000) - RBD().trash_move(ioctx, image_name3, 1000) - RBD().trash_purge(ioctx, datetime.now()) - entries = list(RBD().trash_list(ioctx)) - for e in entries: - assert(e['id'] != image_id1) - RBD.trash_purge(ioctx, datetime.now() + timedelta(seconds=5000), 0) entries = list(RBD().trash_list(ioctx)) - for e in entries: - assert(e['id'] not in [image_id2, image_id3]) + eq([image_id2], [x['id'] for x in entries]) + RBD().trash_remove(ioctx, image_id2, True) def test_remove_denied(self): create_image() -- 2.47.3