From 098de076ea2577e12c964dc5bc05c04ea646f5f2 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Wed, 22 Nov 2023 14:39:13 +0100 Subject: [PATCH] librados: make querying pools for selfmanaged snaps reliable If get_pool_is_selfmanaged_snaps_mode() is invoked on a fresh RADOS client instance that still lacks an osdmap, it returns false, same as for "this pool is not in selfmanaged snaps mode". The same happens if the pool in question doesn't exist since the signature doesn't allow to return an error. The motivation for this API was to prevent users from running "rados cppool" on a pool with unmanaged snapshots and deleting the original thinking that they have a full copy. Unfortunately, it's exactly "rados cppool" that fell into this trap, so no warning is printed and --yes-i-really-mean-it flag isn't enforced. Fixes: https://tracker.ceph.com/issues/63607 Signed-off-by: Ilya Dryomov (cherry picked from commit 0999e63bfbbee46b8e19c3f05881eee64dba8b5e) Conflicts: PendingReleaseNotes [ moved to >=17.2.8 section ] --- PendingReleaseNotes | 7 ++++ src/include/rados/librados.hpp | 5 ++- src/librados/RadosClient.cc | 18 ++++++--- src/librados/RadosClient.h | 2 +- src/librados/librados_cxx.cc | 9 ++++- src/test/librados/snapshots_cxx.cc | 60 ++++++++++++++++++++++++++---- src/tools/rados/rados.cc | 14 ++++++- 7 files changed, 97 insertions(+), 18 deletions(-) diff --git a/PendingReleaseNotes b/PendingReleaseNotes index 310c8561d35..6789c409df0 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -1,3 +1,10 @@ +>=17.2.8 +-------- + +* RADOS: `get_pool_is_selfmanaged_snaps_mode` C++ API has been deprecated + due to being prone to false negative results. It's safer replacement is + `pool_is_in_selfmanaged_snaps_mode`. + >=17.2.7 -------- diff --git a/src/include/rados/librados.hpp b/src/include/rados/librados.hpp index 76021700ae6..9dd1b3e2569 100644 --- a/src/include/rados/librados.hpp +++ b/src/include/rados/librados.hpp @@ -1474,8 +1474,11 @@ inline namespace v14_2_0 { int get_pool_stats(std::list& v, std::string& category, std::map& stats); + /// check if pool has or had selfmanaged snaps - bool get_pool_is_selfmanaged_snaps_mode(const std::string& poolname); + bool get_pool_is_selfmanaged_snaps_mode(const std::string& poolname) + __attribute__ ((deprecated)); + int pool_is_in_selfmanaged_snaps_mode(const std::string& poolname); int cluster_stat(cluster_stat_t& result); int cluster_fsid(std::string *fsid); diff --git a/src/librados/RadosClient.cc b/src/librados/RadosClient.cc index 43914a89c2c..1662e0004e7 100644 --- a/src/librados/RadosClient.cc +++ b/src/librados/RadosClient.cc @@ -631,16 +631,22 @@ int librados::RadosClient::get_pool_stats(std::list& pools, return 0; } -bool librados::RadosClient::get_pool_is_selfmanaged_snaps_mode( +int librados::RadosClient::pool_is_in_selfmanaged_snaps_mode( const std::string& pool) { - bool ret = false; - objecter->with_osdmap([&](const OSDMap& osdmap) { + int r = wait_for_osdmap(); + if (r < 0) { + return r; + } + + return objecter->with_osdmap([&pool](const OSDMap& osdmap) { int64_t poolid = osdmap.lookup_pg_pool_name(pool); - if (poolid >= 0) - ret = osdmap.get_pg_pool(poolid)->is_unmanaged_snaps_mode(); + if (poolid < 0) { + return -ENOENT; + } + return static_cast( + osdmap.get_pg_pool(poolid)->is_unmanaged_snaps_mode()); }); - return ret; } int librados::RadosClient::get_fs_stats(ceph_statfs& stats) diff --git a/src/librados/RadosClient.h b/src/librados/RadosClient.h index 00a273b82d3..068153bb81b 100644 --- a/src/librados/RadosClient.h +++ b/src/librados/RadosClient.h @@ -131,7 +131,7 @@ public: int get_pool_stats(std::list& ls, std::map *result, bool *per_pool); int get_fs_stats(ceph_statfs& result); - bool get_pool_is_selfmanaged_snaps_mode(const std::string& pool); + int pool_is_in_selfmanaged_snaps_mode(const std::string& pool); /* -1 was set as the default value and monitor will pickup the right crush rule with below order: diff --git a/src/librados/librados_cxx.cc b/src/librados/librados_cxx.cc index 52925b4013f..c4b0f608966 100644 --- a/src/librados/librados_cxx.cc +++ b/src/librados/librados_cxx.cc @@ -2719,9 +2719,16 @@ int librados::Rados::get_pool_stats(std::list& v, return -EOPNOTSUPP; } +// deprecated, use pool_is_in_selfmanaged_snaps_mode() instead bool librados::Rados::get_pool_is_selfmanaged_snaps_mode(const std::string& pool) { - return client->get_pool_is_selfmanaged_snaps_mode(pool); + // errors are ignored, prone to false negative results + return client->pool_is_in_selfmanaged_snaps_mode(pool) > 0; +} + +int librados::Rados::pool_is_in_selfmanaged_snaps_mode(const std::string& pool) +{ + return client->pool_is_in_selfmanaged_snaps_mode(pool); } int librados::Rados::cluster_stat(cluster_stat_t& result) diff --git a/src/test/librados/snapshots_cxx.cc b/src/test/librados/snapshots_cxx.cc index dee17ddb8d7..2e74688643a 100644 --- a/src/test/librados/snapshots_cxx.cc +++ b/src/test/librados/snapshots_cxx.cc @@ -24,9 +24,9 @@ TEST_F(LibRadosSnapshotsPP, SnapListPP) { bufferlist bl1; bl1.append(buf, sizeof(buf)); ASSERT_EQ(0, ioctx.write("foo", bl1, sizeof(buf), 0)); - ASSERT_FALSE(cluster.get_pool_is_selfmanaged_snaps_mode(pool_name)); + ASSERT_EQ(0, cluster.pool_is_in_selfmanaged_snaps_mode(pool_name)); ASSERT_EQ(0, ioctx.snap_create("snap1")); - ASSERT_FALSE(cluster.get_pool_is_selfmanaged_snaps_mode(pool_name)); + ASSERT_EQ(0, cluster.pool_is_in_selfmanaged_snaps_mode(pool_name)); std::vector snaps; EXPECT_EQ(0, ioctx.snap_list(&snaps)); EXPECT_EQ(1U, snaps.size()); @@ -34,7 +34,7 @@ TEST_F(LibRadosSnapshotsPP, SnapListPP) { EXPECT_EQ(0, ioctx.snap_lookup("snap1", &rid)); EXPECT_EQ(rid, snaps[0]); EXPECT_EQ(0, ioctx.snap_remove("snap1")); - ASSERT_FALSE(cluster.get_pool_is_selfmanaged_snaps_mode(pool_name)); + ASSERT_EQ(0, cluster.pool_is_in_selfmanaged_snaps_mode(pool_name)); } TEST_F(LibRadosSnapshotsPP, SnapRemovePP) { @@ -108,9 +108,9 @@ TEST_F(LibRadosSnapshotsPP, SnapCreateRemovePP) { TEST_F(LibRadosSnapshotsSelfManagedPP, SnapPP) { std::vector my_snaps; my_snaps.push_back(-2); - ASSERT_FALSE(cluster.get_pool_is_selfmanaged_snaps_mode(pool_name)); + ASSERT_EQ(0, cluster.pool_is_in_selfmanaged_snaps_mode(pool_name)); ASSERT_EQ(0, ioctx.selfmanaged_snap_create(&my_snaps.back())); - ASSERT_TRUE(cluster.get_pool_is_selfmanaged_snaps_mode(pool_name)); + ASSERT_EQ(1, cluster.pool_is_in_selfmanaged_snaps_mode(pool_name)); ::std::reverse(my_snaps.begin(), my_snaps.end()); ASSERT_EQ(0, ioctx.selfmanaged_snap_set_write_ctx(my_snaps[0], my_snaps)); ::std::reverse(my_snaps.begin(), my_snaps.end()); @@ -147,7 +147,7 @@ TEST_F(LibRadosSnapshotsSelfManagedPP, SnapPP) { ASSERT_EQ(0, ioctx.selfmanaged_snap_remove(my_snaps.back())); my_snaps.pop_back(); ioctx.snap_set_read(LIBRADOS_SNAP_HEAD); - ASSERT_TRUE(cluster.get_pool_is_selfmanaged_snaps_mode(pool_name)); + ASSERT_EQ(1, cluster.pool_is_in_selfmanaged_snaps_mode(pool_name)); ASSERT_EQ(0, ioctx.remove("foo")); } @@ -458,7 +458,7 @@ TEST_F(LibRadosSnapshotsSelfManagedPP, ReusePurgedSnap) { std::vector my_snaps; my_snaps.push_back(-2); ASSERT_EQ(0, ioctx.selfmanaged_snap_create(&my_snaps.back())); - ASSERT_TRUE(cluster.get_pool_is_selfmanaged_snaps_mode(pool_name)); + ASSERT_EQ(1, cluster.pool_is_in_selfmanaged_snaps_mode(pool_name)); ::std::reverse(my_snaps.begin(), my_snaps.end()); ASSERT_EQ(0, ioctx.selfmanaged_snap_set_write_ctx(my_snaps[0], my_snaps)); ::std::reverse(my_snaps.begin(), my_snaps.end()); @@ -497,6 +497,52 @@ TEST_F(LibRadosSnapshotsSelfManagedPP, ReusePurgedSnap) { //sleep(600); } +TEST(LibRadosPoolIsInSelfmanagedSnapsMode, NotConnected) { + librados::Rados cluster; + ASSERT_EQ(0, cluster.init(nullptr)); + + EXPECT_EQ(-ENOTCONN, cluster.pool_is_in_selfmanaged_snaps_mode("foo")); +} + +TEST(LibRadosPoolIsInSelfmanagedSnapsMode, FreshInstance) { + librados::Rados cluster1; + std::string pool_name = get_temp_pool_name(); + ASSERT_EQ("", create_one_pool_pp(pool_name, cluster1)); + EXPECT_EQ(0, cluster1.pool_is_in_selfmanaged_snaps_mode(pool_name)); + { + librados::Rados cluster2; + ASSERT_EQ("", connect_cluster_pp(cluster2)); + EXPECT_EQ(0, cluster2.pool_is_in_selfmanaged_snaps_mode(pool_name)); + } + + librados::IoCtx ioctx; + cluster1.ioctx_create(pool_name.c_str(), ioctx); + uint64_t snap_id; + ASSERT_EQ(0, ioctx.selfmanaged_snap_create(&snap_id)); + EXPECT_EQ(1, cluster1.pool_is_in_selfmanaged_snaps_mode(pool_name)); + { + librados::Rados cluster2; + ASSERT_EQ("", connect_cluster_pp(cluster2)); + EXPECT_EQ(1, cluster2.pool_is_in_selfmanaged_snaps_mode(pool_name)); + } + + ASSERT_EQ(0, ioctx.selfmanaged_snap_remove(snap_id)); + EXPECT_EQ(1, cluster1.pool_is_in_selfmanaged_snaps_mode(pool_name)); + { + librados::Rados cluster2; + ASSERT_EQ("", connect_cluster_pp(cluster2)); + EXPECT_EQ(1, cluster2.pool_is_in_selfmanaged_snaps_mode(pool_name)); + } + + ASSERT_EQ(0, cluster1.pool_delete(pool_name.c_str())); + EXPECT_EQ(-ENOENT, cluster1.pool_is_in_selfmanaged_snaps_mode(pool_name)); + { + librados::Rados cluster2; + ASSERT_EQ("", connect_cluster_pp(cluster2)); + EXPECT_EQ(-ENOENT, cluster2.pool_is_in_selfmanaged_snaps_mode(pool_name)); + } +} + // EC testing TEST_F(LibRadosSnapshotsECPP, SnapListPP) { char buf[bufsize]; diff --git a/src/tools/rados/rados.cc b/src/tools/rados/rados.cc index 11d9f1f7c65..ed0381b5468 100644 --- a/src/tools/rados/rados.cc +++ b/src/tools/rados/rados.cc @@ -3120,7 +3120,12 @@ static int rados_tool_common(const std::map < std::string, std::string > &opts, cerr << "WARNING: pool copy does not preserve user_version, which some " << " apps may rely on." << std::endl; - if (rados.get_pool_is_selfmanaged_snaps_mode(src_pool)) { + ret = rados.pool_is_in_selfmanaged_snaps_mode(src_pool); + if (ret < 0) { + cerr << "failed to query pool " << src_pool << " for selfmanaged snaps: " + << cpp_strerror(ret) << std::endl; + return 1; + } else if (ret > 0) { cerr << "WARNING: pool " << src_pool << " has selfmanaged snaps, which are not preserved\n" << " by the cppool operation. This will break any snapshot user." << std::endl; @@ -3213,7 +3218,12 @@ static int rados_tool_common(const std::map < std::string, std::string > &opts, return 1; } - if (rados.get_pool_is_selfmanaged_snaps_mode(pool_name)) { + ret = rados.pool_is_in_selfmanaged_snaps_mode(pool_name); + if (ret < 0) { + cerr << "failed to query pool " << pool_name << " for selfmanaged snaps: " + << cpp_strerror(ret) << std::endl; + return 1; + } else if (ret > 0) { cerr << "can't create snapshot: pool " << pool_name << " is in selfmanaged snaps mode" << std::endl; return 1; -- 2.47.3