rgw/cls: Make cmpomap workable for atomic updates with different values

1. Make cmp_vals() to take in ObjectOperation to allow
it work with ObjectWriteOperation.

2. Add comments for cmp_set_vals() regarding how different
omap values may be set.

3. Add unit tests to verify the transactional nature of
ObjectWriteOperation used when setting different omap values.

Fixes: https://tracker.ceph.com/issues/76622
Signed-off-by: Yixin Jin <yjin77@yahoo.ca>

diff --git a/src/cls/cmpomap/client.cc b/src/cls/cmpomap/client.cc
index 946f33518d11dccd4a0e75b8f424cc89b09908ba..8c4a25f0a65a2a1bfb03303c7b2b17ed0c8576cc 100644 (file)
--- a/src/cls/cmpomap/client.cc
+++ b/src/cls/cmpomap/client.cc
@@ -18,7 +18,7 @@
 
 namespace cls::cmpomap {
 
-int cmp_vals(librados::ObjectReadOperation& op,
+int cmp_vals(librados::ObjectOperation& op,
              Mode mode, Op comparison, ComparisonMap values,
              std::optional<ceph::bufferlist> default_value)
 {

diff --git a/src/cls/cmpomap/client.h b/src/cls/cmpomap/client.h
index cfb0be8d97df83fed230732adfa1add247c199fa..521c5fd13cc49171caf749caa9c0448426e97846 100644 (file)
--- a/src/cls/cmpomap/client.h
+++ b/src/cls/cmpomap/client.h
@@ -28,7 +28,7 @@ static constexpr uint32_t max_keys = 1000;
 /// comparisons with Mode::U64, failure to decode an input value is reported
 /// as -EINVAL, an empty stored value is compared as 0, and failure to decode
 /// a stored value is reported as -EIO
-[[nodiscard]] int cmp_vals(librados::ObjectReadOperation& op,
+[[nodiscard]] int cmp_vals(librados::ObjectOperation& op,
                            Mode mode, Op comparison, ComparisonMap values,
                            std::optional<ceph::bufferlist> default_value);
 
@@ -38,6 +38,16 @@ static constexpr uint32_t max_keys = 1000;
 /// to decode an input value is reported as -EINVAL. an empty stored value is
 /// compared as 0, while decode failure of a stored value is treated as an
 /// unsuccessful comparison and is not reported as an error
+///
+/// NOTE: This function cannot be used to set different values when Op::EQ
+/// is used. To accomplish this, one may utilize the transactional operation
+/// with librados::ObjectWriteOperation in combination with cmp_vals(). For example,
+///
+///   librados::ObjectWriteOperation op;
+///   // cmp_vals() fails the operation with -ECANCELED if any comparison fails
+///   cls::cmpomap::cmp_vals(op, mode, comparison, cmp_values, std::nullopt);
+///   // write new values on success
+///   op.omap_set(set_values);
 [[nodiscard]] int cmp_set_vals(librados::ObjectWriteOperation& writeop,
                                Mode mode, Op comparison, ComparisonMap values,
                                std::optional<ceph::bufferlist> default_value);

diff --git a/src/test/cls_cmpomap/test_cls_cmpomap.cc b/src/test/cls_cmpomap/test_cls_cmpomap.cc
index 2ea9b260a96fb246737706508c855f6733102a6d..5b5cfc065f7e84e3a1ebffb4c4b649aad03a8361 100644 (file)
--- a/src/test/cls_cmpomap/test_cls_cmpomap.cc
+++ b/src/test/cls_cmpomap/test_cls_cmpomap.cc
@@ -446,6 +446,55 @@ TEST_F(CmpOmap, cmp_set_vals_str)
   }
 }
 
+TEST_F(CmpOmap, atomic_omap_set_conditional_match)
+{
+  const std::string oid = __PRETTY_FUNCTION__;
+  const bufferlist value1 = u64_buffer(1);
+  const bufferlist value2 = u64_buffer(42);
+  {
+    std::map<std::string, bufferlist> vals = {
+      {"eq", value1},
+    };
+    ASSERT_EQ(ioctx.omap_set(oid, vals), 0);
+  }
+
+  librados::ObjectWriteOperation op;
+  ASSERT_EQ(cmp_vals(op, Mode::U64, Op::EQ, {{"eq", value1}}, std::nullopt), 0);
+  op.omap_set({{"eq", value2}});
+  ASSERT_EQ(ioctx.operate(oid, &op), 0);
+  {
+    std::map<std::string, bufferlist> vals;
+    ASSERT_EQ(get_vals(oid, &vals), 0);
+    ASSERT_EQ(vals.size(), 1);
+    EXPECT_EQ(value2, vals["eq"]);
+  }
+}
+
+TEST_F(CmpOmap, atomic_omap_set_conditional_mismatch)
+{
+  const std::string oid = __PRETTY_FUNCTION__;
+  const bufferlist value1 = u64_buffer(1);
+  const bufferlist value2 = u64_buffer(2);
+  const bufferlist value3 = u64_buffer(42);
+  {
+    std::map<std::string, bufferlist> vals = {
+      {"eq", value1},
+    };
+    ASSERT_EQ(ioctx.omap_set(oid, vals), 0);
+  }
+
+  librados::ObjectWriteOperation op;
+  ASSERT_EQ(cmp_vals(op, Mode::U64, Op::EQ, {{"eq", value2}}, std::nullopt), 0);
+  op.omap_set({{"eq", value3}});
+  ASSERT_EQ(ioctx.operate(oid, &op), -ECANCELED);
+  {
+    std::map<std::string, bufferlist> vals;
+    ASSERT_EQ(get_vals(oid, &vals), 0);
+    ASSERT_EQ(vals.size(), 1);
+    EXPECT_EQ(value1, vals["eq"]); // Nothing is changed
+  }
+}
+
 TEST_F(CmpOmap, cmp_set_vals_u64)
 {
   const std::string oid = __PRETTY_FUNCTION__;