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 <idryomov@gmail.com>
(cherry picked from commit 0999e63bfbbee46b8e19c3f05881eee64dba8b5e)

Conflicts:
	PendingReleaseNotes [ moved to >=16.2.15 section ]

diff --git a/PendingReleaseNotes b/PendingReleaseNotes
index 120980c862c9a20bb64b10f9867bfe09fa2ae799..0cb10ea998121b4577be25f1b5ddded294320958 100644 (file)
--- a/PendingReleaseNotes
+++ b/PendingReleaseNotes
@@ -45,6 +45,10 @@
   the RADOS operation exceeding the size threshold. `mds_session_metadata_threshold`
   config controls the maximum size that a (encoded) session metadata can grow.
 
+* 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`.
+
 >= 16.2.14
 ----------
 

diff --git a/src/include/rados/librados.hpp b/src/include/rados/librados.hpp
index b8377daa62e8959355d3788fa4402f836428716f..10dd8cc5394565e1cbf6a75b1f358c13011b3c93 100644 (file)
--- a/src/include/rados/librados.hpp
+++ b/src/include/rados/librados.hpp
@@ -1465,8 +1465,11 @@ inline namespace v14_2_0 {
     int get_pool_stats(std::list<std::string>& v,
                        std::string& category,
                       std::map<std::string, stats_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 04ea14f312570c5d2310f69265793317e83b2fa7..547c084cd51e3b29855c997eb7e2671cb615e902 100644 (file)
--- a/src/librados/RadosClient.cc
+++ b/src/librados/RadosClient.cc
@@ -626,16 +626,22 @@ int librados::RadosClient::get_pool_stats(std::list<string>& 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<int>(
+        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 0db094b18009439574c6cbc0b41b88ec39d31133..7c8dd79df9f088805db84ca053546ea406024da2 100644 (file)
--- a/src/librados/RadosClient.h
+++ b/src/librados/RadosClient.h
@@ -131,7 +131,7 @@ public:
   int get_pool_stats(std::list<string>& ls, map<string,::pool_stat_t> *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 1d9ab6bb8ddca6b652ae14aa9da6eb3b5c7a960f..81da002cd8296e89d487db76d0ec2ad5f2846014 100644 (file)
--- a/src/librados/librados_cxx.cc
+++ b/src/librados/librados_cxx.cc
@@ -2715,9 +2715,16 @@ int librados::Rados::get_pool_stats(std::list<string>& 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 dee17ddb8d7a3d4abefab0957121bbef140ca5c4..2e74688643a6c09e0ffe328db98ebae841f9f674 100644 (file)
--- 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<snap_t> 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<uint64_t> 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<uint64_t> 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 7d5fdf814cd36d25b3a94b8db25d8b9407334949..820b6a3cad6b990315f279c2031213e80876a526 100644 (file)
--- a/src/tools/rados/rados.cc
+++ b/src/tools/rados/rados.cc
@@ -3104,7 +3104,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;
@@ -3197,7 +3202,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;