pacific: librados: make querying pools for selfmanaged snaps reliable

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
 ----------
 

qa/workunits/mon/rbd_snaps_ops.sh

@@ -36,6 +36,7 @@ expect 'rbd --pool=test snap ls image' 0
 expect 'rbd --pool=test snap rm image@snapshot' 0
 
 expect 'ceph osd pool mksnap test snapshot' 22
+expect 'rados -p test mksnap snapshot' 1
 
 expect 'ceph osd pool delete test test --yes-i-really-really-mean-it' 0
 
@@ -52,6 +53,8 @@ expect 'rbd --pool test-foo snap create image@snapshot' 0
 ceph osd pool delete test-bar test-bar --yes-i-really-really-mean-it || true
 expect 'ceph osd pool create test-bar 8' 0
 expect 'ceph osd pool application enable test-bar rbd'
+# "rados cppool" without --yes-i-really-mean-it should fail
+expect 'rados cppool test-foo test-bar' 1
 expect 'rados cppool test-foo test-bar --yes-i-really-mean-it' 0
 expect 'rbd --pool test-bar snap rm image@snapshot' 95
 expect 'ceph osd pool delete test-foo test-foo --yes-i-really-really-mean-it' 0

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 selfmanaged snaps
-    bool get_pool_is_selfmanaged_snaps_mode(const std::string& poolname);
+
+    /// check if pool has or had selfmanaged snaps
+    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);

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)

src/librados/RadosClient.h

@@ -131,7 +131,7 @@ class librados::RadosClient : public Dispatcher,
   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:

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)

src/test/librados/snapshots_cxx.cc

@@ -24,17 +24,17 @@ 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());
   snap_t rid;
   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];

src/tools/rados/rados.cc

@@ -3104,15 +3104,20 @@ 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;
       if (!force) {
 	cerr << "    If you insist on making a broken copy, you can pass\n"
 	     << "    --yes-i-really-mean-it to proceed anyway."
 	     << std::endl;
-	exit(1);
+	return 1;
       }
     }
 
@@ -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;