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 >=18.2.2 section ]

diff --git a/PendingReleaseNotes b/PendingReleaseNotes
index 2c82b13311d10a4d97ee7bc5ee4a650890ffb208..19087d9214c1174044a286a4baea649866ea38b1 100644 (file)
--- a/PendingReleaseNotes
+++ b/PendingReleaseNotes
@@ -6,6 +6,9 @@
   and valid), diff-iterate is now guaranteed to execute locally if exclusive
   lock is available.  This brings a dramatic performance improvement for QEMU
   live disk synchronization and backup use cases.
+* 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`.
 
 >=19.0.0
 

diff --git a/src/include/rados/librados.hpp b/src/include/rados/librados.hpp
index f2f9e47a2ad77778beaa262f45f2fc77bd44762b..2cd418627be956fafe00255640b9e1d7c89fde0a 100644 (file)
--- a/src/include/rados/librados.hpp
+++ b/src/include/rados/librados.hpp
@@ -1477,8 +1477,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 9abd923f95b03132042dc492ac179abb29939c5e..db9143e2d5b8bc7b38d50818a3045f9cc67e31bd 100644 (file)
--- a/src/librados/RadosClient.cc
+++ b/src/librados/RadosClient.cc
@@ -631,16 +631,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 052249a76db36953f16836d492e8148b361fadf1..a93f8185108dc4b2f2476a15bd023945d067172d 100644 (file)
--- a/src/librados/RadosClient.h
+++ b/src/librados/RadosClient.h
@@ -131,7 +131,7 @@ public:
   int get_pool_stats(std::list<std::string>& ls, std::map<std::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 d20c67556c03e95e5a2a2e7a4a62836886258d14..926ddf86dab444f24bcae8f3226909207643f291 100644 (file)
--- a/src/librados/librados_cxx.cc
+++ b/src/librados/librados_cxx.cc
@@ -2719,9 +2719,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 8098b2cb7817ea8f7902f9089f8bc5b5b836f2d3..95dbe5da0125aac407b964ee97afaf0332c764c7 100644 (file)
--- a/src/test/librados/snapshots_cxx.cc
+++ b/src/test/librados/snapshots_cxx.cc
@@ -25,9 +25,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());
@@ -35,7 +35,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) {
@@ -109,9 +109,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());
@@ -148,7 +148,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"));
 }
 
@@ -509,7 +509,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());
@@ -548,6 +548,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) {
   SKIP_IF_CRIMSON();

diff --git a/src/tools/rados/rados.cc b/src/tools/rados/rados.cc
index cd301916c478f4549bfc4af4ab0325fd81b8e8f2..bfeea194265bee06c30e181329fe4d698b5288b8 100644 (file)
--- 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;