Skip to content

Commit

Permalink
Merge pull request ceph#54644 from idryomov/wip-63607
Browse files Browse the repository at this point in the history
librados: make querying pools for selfmanaged snaps reliable

Reviewed-by: Ramana Raja <[email protected]>
Reviewed-by: Radoslaw Zarzynski <[email protected]>
  • Loading branch information
idryomov authored Dec 28, 2023
2 parents e4c37cd + de66355 commit d17c133
Show file tree
Hide file tree
Showing 8 changed files with 98 additions and 20 deletions.
3 changes: 3 additions & 0 deletions PendingReleaseNotes
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ CephFS: Disallow delegating preallocated inode ranges to clients. Config
cannot repair the dates of existing object locks. Such objects can be identified
with a HeadObject request based on the x-amz-object-lock-retain-until-date
response header.
* 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`.

>=18.0.0

Expand Down
3 changes: 3 additions & 0 deletions qa/workunits/mon/rbd_snaps_ops.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down
7 changes: 5 additions & 2 deletions src/include/rados/librados.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 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);
Expand Down
18 changes: 12 additions & 6 deletions src/librados/RadosClient.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/librados/RadosClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ class librados::RadosClient : public Dispatcher,
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:
Expand Down
9 changes: 8 additions & 1 deletion src/librados/librados_cxx.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
60 changes: 53 additions & 7 deletions src/test/librados/snapshots_cxx.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,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) {
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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"));
}

Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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();
Expand Down
16 changes: 13 additions & 3 deletions src/tools/rados/rados.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3120,15 +3120,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;
}
}

Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit d17c133

Please sign in to comment.