Merge pull request #236 from jecluis/wip-sfs-fix-bucket-mtime

rgw/sfs: init bucket mtime and store it

src/rgw/driver/sfs/bucket.cc

@@ -48,6 +48,7 @@ namespace rgw::sal {
 SFSBucket::SFSBucket(SFStore* _store, sfs::BucketRef _bucket)
     : StoreBucket(_bucket->get_info()), store(_store), bucket(_bucket) {
   set_attrs(bucket->get_attrs());
+  mtime = _bucket->get_mtime();
 
   auto it = attrs.find(RGW_ATTR_ACL);
   if (it != attrs.end()) {
@@ -339,6 +340,7 @@ int SFSBucket::remove_bucket(
     return -ENOENT;
   }
   db_bucket->deleted = true;
+  db_bucket->mtime = ceph::real_time::clock::now();
   db_buckets.store_bucket(*db_bucket);
   store->_delete_bucket(get_name());
   return 0;
@@ -371,7 +373,7 @@ int SFSBucket::set_acl(
   attrs[RGW_ATTR_ACL] = aclp_bl;
 
   sfs::get_meta_buckets(get_store().db_conn)
-      ->store_bucket(sfs::sqlite::DBOPBucketInfo(get_info(), get_attrs()));
+      ->store_bucket(get_db_op_bucket_info());
 
   store->_refresh_buckets_safe();
   return 0;
@@ -422,7 +424,7 @@ int SFSBucket::merge_and_store_attrs(
   }
 
   sfs::get_meta_buckets(get_store().db_conn)
-      ->store_bucket(sfs::sqlite::DBOPBucketInfo(get_info(), get_attrs()));
+      ->store_bucket(get_db_op_bucket_info());
 
   store->_refresh_buckets_safe();
   return 0;
@@ -637,7 +639,7 @@ int SFSBucket::put_info(
   }
 
   sfs::get_meta_buckets(get_store().db_conn)
-      ->store_bucket(sfs::sqlite::DBOPBucketInfo(get_info(), get_attrs()));
+      ->store_bucket(get_db_op_bucket_info());
 
   store->_refresh_buckets_safe();
   return 0;

src/rgw/driver/sfs/bucket.h

@@ -25,6 +25,7 @@
 #include "driver/sfs/types.h"
 #include "rgw_sal.h"
 #include "rgw_sal_store.h"
+#include "sqlite/buckets/bucket_definitions.h"
 
 namespace rgw::sal {
 
@@ -200,6 +201,13 @@ class SFSBucket : public StoreBucket {
   SFStore& get_store() { return *store; }
 
   const SFStore& get_store() const { return *store; }
+
+ private:
+  sfs::sqlite::DBOPBucketInfo get_db_op_bucket_info() {
+    auto ret = sfs::sqlite::DBOPBucketInfo(get_info(), get_attrs());
+    ret.mtime = ceph::real_time::clock::now();
+    return ret;
+  }
 };
 
 }  // namespace rgw::sal

src/rgw/driver/sfs/sqlite/buckets/bucket_conversions.cc

@@ -39,6 +39,7 @@ DBOPBucketInfo get_rgw_bucket(const DBBucket& bucket) {
   assign_optional_value(bucket.object_lock, rgw_bucket.binfo.obj_lock);
   assign_optional_value(bucket.bucket_attrs, rgw_bucket.battrs);
   rgw_bucket.deleted = bucket.deleted;
+  rgw_bucket.mtime = bucket.mtime;
 
   return rgw_bucket;
 }
@@ -63,6 +64,7 @@ DBBucket get_db_bucket(const DBOPBucketInfo& bucket) {
   assign_db_value(bucket.binfo.obj_lock, db_bucket.object_lock);
   assign_db_value(bucket.battrs, db_bucket.bucket_attrs);
   db_bucket.deleted = bucket.deleted;
+  db_bucket.mtime = bucket.mtime;
 
   return db_bucket;
 }

src/rgw/driver/sfs/sqlite/buckets/bucket_definitions.h

@@ -55,7 +55,7 @@ struct DBBucket {
   std::optional<BLOB> bucket_attrs;
   std::optional<int> bucket_version;
   std::optional<std::string> bucket_version_tag;
-  std::optional<BLOB> mtime;
+  ceph::real_time mtime;
   bool deleted;
 };
 
@@ -64,6 +64,7 @@ struct DBOPBucketInfo {
   RGWBucketInfo binfo;
   Attrs battrs;
   bool deleted{false};
+  ceph::real_time mtime;
 
   DBOPBucketInfo() = default;
 

src/rgw/driver/sfs/sqlite/dbconn.h

@@ -137,6 +137,7 @@ inline auto _make_storage(const std::string& path) {
           sqlite_orm::make_column("deleted", &DBBucket::deleted),
           sqlite_orm::make_column("bucket_attrs", &DBBucket::bucket_attrs),
           sqlite_orm::make_column("object_lock", &DBBucket::object_lock),
+          sqlite_orm::make_column("mtime", &DBBucket::mtime),
           sqlite_orm::foreign_key(&DBBucket::owner_id)
               .references(&DBUser::user_id)
       ),

src/rgw/driver/sfs/types.h

@@ -136,6 +136,7 @@ class Bucket {
   RGWBucketInfo info;
   rgw::sal::Attrs attrs;
   bool deleted{false};
+  ceph::real_time mtime;
 
  public:
   ceph::mutex multipart_map_lock = ceph::make_mutex("multipart_map_lock");
@@ -168,13 +169,14 @@ class Bucket {
   Bucket(
       CephContext* _cct, rgw::sal::SFStore* _store,
       const RGWBucketInfo& _bucket_info, const RGWUserInfo& _owner,
-      const rgw::sal::Attrs& _attrs
+      const rgw::sal::Attrs& _attrs, ceph::real_time& _mtime
   )
       : cct(_cct),
         store(_store),
         owner(_owner),
         info(_bucket_info),
-        attrs(_attrs) {}
+        attrs(_attrs),
+        mtime(_mtime) {}
 
   const RGWBucketInfo& get_info() const { return info; }
 
@@ -202,7 +204,8 @@ class Bucket {
 
   uint32_t get_flags() const { return info.flags; }
 
- public:
+  ceph::real_time get_mtime() const { return mtime; }
+
   /// Create object version for key
   ObjectRef create_version(const rgw_obj_key& key) const;
 

src/rgw/rgw_sal_sfs.h

@@ -419,12 +419,13 @@ class SFStore : public StoreDriver {
     info.requester_pays = false;
     info.quota = db_binfo.binfo.quota;
     db_binfo.battrs = attrs;
+    db_binfo.mtime = ceph::real_time::clock::now();
 
     auto meta_buckets = sfs::get_meta_buckets(db_conn);
     meta_buckets->store_bucket(db_binfo);
 
     sfs::BucketRef b = std::make_shared<sfs::Bucket>(
-        ctx(), this, db_binfo.binfo, owner, db_binfo.battrs
+        ctx(), this, db_binfo.binfo, owner, db_binfo.battrs, db_binfo.mtime
     );
     buckets[bucket.name] = b;
     return b;
@@ -444,7 +445,7 @@ class SFStore : public StoreDriver {
       if (!b.deleted) {
         auto user = users.get_user(b.binfo.owner.id);
         sfs::BucketRef ref = std::make_shared<sfs::Bucket>(
-            ctx(), this, b.binfo, user->uinfo, b.battrs
+            ctx(), this, b.binfo, user->uinfo, b.battrs, b.mtime
         );
         buckets[b.binfo.bucket.name] = ref;
       }

src/test/rgw/sfs/test_rgw_sfs_concurrency.cc

@@ -100,7 +100,8 @@ class TestSFSConcurrency
     RGWUserInfo bucket_owner;
 
     bucket = std::make_shared<Bucket>(
-        cct.get(), store.get(), db_binfo.binfo, bucket_owner, db_binfo.battrs
+        cct.get(), store.get(), db_binfo.binfo, bucket_owner, db_binfo.battrs,
+        db_binfo.mtime
     );
 
     predef_object = bucket->create_version(rgw_obj_key("predef_object"));

src/test/rgw/sfs/test_rgw_sfs_object_state_machine.cc

@@ -61,7 +61,8 @@ class TestSFSObjectStateMachine : public ::testing::Test {
     db_buckets.store_bucket(db_binfo);
 
     bucket = std::make_shared<Bucket>(
-        cct, store.get(), db_binfo.binfo, bucket_owner, db_binfo.battrs
+        cct, store.get(), db_binfo.binfo, bucket_owner, db_binfo.battrs,
+        db_binfo.mtime
     );
   }
 

src/test/rgw/sfs/test_rgw_sfs_sfs_bucket.cc

@@ -75,14 +75,18 @@ class TestSFSBucket : public ::testing::Test {
 
   void createTestBucket(
       const std::string& bucket_id, const std::string& user_id, DBConnRef conn,
-      bool versioned = false
+      bool versioned = false, ceph::real_time* mtime = nullptr
   ) {
     SQLiteBuckets db_buckets(conn);
     DBOPBucketInfo bucket;
     bucket.binfo.bucket.name = bucket_id + "_name";
     bucket.binfo.bucket.bucket_id = bucket_id;
     bucket.binfo.owner.id = user_id;
     bucket.deleted = false;
+    bucket.mtime = ceph::real_time::clock::now();
+    if (mtime != nullptr) {
+      *mtime = bucket.mtime;
+    }
     if (versioned) {
       bucket.binfo.flags |= BUCKET_VERSIONED;
     }
@@ -223,6 +227,8 @@ TEST_F(TestSFSBucket, UserCreateBucketCheckGotFromCreate) {
   }
 
   EXPECT_EQ(bucket_from_create->get_acl(), arg_aclp);
+  EXPECT_FALSE(real_clock::is_zero(bucket_from_create->get_modification_time())
+  );
 
   //@warning this triggers segfault
   //EXPECT_EQ(bucket_from_create->get_owner()->get_id().id, "usr_id");
@@ -307,6 +313,9 @@ TEST_F(TestSFSBucket, UserCreateBucketCheckGotFromStore) {
   }
 
   EXPECT_EQ(bucket_from_store->get_acl(), bucket_from_create->get_acl());
+  EXPECT_TRUE(
+      bucket_from_store->get_modification_time().time_since_epoch().count() > 0
+  );
 }
 
 TEST_F(TestSFSBucket, BucketSetAcl) {
@@ -364,6 +373,9 @@ TEST_F(TestSFSBucket, BucketSetAcl) {
       0
   );
 
+  ceph::real_time create_mtime = bucket_from_create->get_modification_time();
+  EXPECT_TRUE(create_mtime.time_since_epoch().count() > 0);
+
   std::unique_ptr<rgw::sal::Bucket> bucket_from_store;
 
   EXPECT_EQ(
@@ -372,6 +384,7 @@ TEST_F(TestSFSBucket, BucketSetAcl) {
       ),
       0
   );
+  EXPECT_EQ(bucket_from_store->get_modification_time(), create_mtime);
 
   RGWAccessControlPolicy arg_aclp_1 = get_aclp_1();
 
@@ -390,6 +403,7 @@ TEST_F(TestSFSBucket, BucketSetAcl) {
   );
 
   EXPECT_EQ(bucket_from_store->get_acl(), bucket_from_store_1->get_acl());
+  EXPECT_GT(bucket_from_store_1->get_modification_time(), create_mtime);
 }
 
 TEST_F(TestSFSBucket, BucketMergeAndStoreAttrs) {
@@ -447,6 +461,8 @@ TEST_F(TestSFSBucket, BucketMergeAndStoreAttrs) {
       0
   );
 
+  ceph::real_time create_mtime = bucket_from_create->get_modification_time();
+
   std::unique_ptr<rgw::sal::Bucket> bucket_from_store;
 
   EXPECT_EQ(
@@ -456,6 +472,8 @@ TEST_F(TestSFSBucket, BucketMergeAndStoreAttrs) {
       0
   );
 
+  EXPECT_EQ(bucket_from_store->get_modification_time(), create_mtime);
+
   rgw::sal::Attrs new_attrs;
   RGWAccessControlPolicy arg_aclp_1 = get_aclp_1();
   {
@@ -483,6 +501,7 @@ TEST_F(TestSFSBucket, BucketMergeAndStoreAttrs) {
 
   EXPECT_EQ(bucket_from_store_1->get_attrs(), new_attrs);
   EXPECT_EQ(bucket_from_store->get_acl(), bucket_from_store_1->get_acl());
+  EXPECT_GT(bucket_from_store_1->get_modification_time(), create_mtime);
 }
 
 TEST_F(TestSFSBucket, DeleteBucket) {
@@ -540,6 +559,8 @@ TEST_F(TestSFSBucket, DeleteBucket) {
       0
   );
 
+  ceph::real_time create_mtime = bucket_from_create->get_modification_time();
+
   std::unique_ptr<rgw::sal::Bucket> bucket_from_store;
 
   EXPECT_EQ(
@@ -577,6 +598,10 @@ TEST_F(TestSFSBucket, DeleteBucket) {
   ASSERT_TRUE(b_metadata.has_value());
   EXPECT_TRUE(b_metadata->deleted);
 
+  // mtime should have been updated
+  ceph::real_time b_mtime = b_metadata->mtime;
+  EXPECT_GT(b_mtime, create_mtime);
+
   // now create the bucket again (should be ok, but bucket_id should differ)
   auto prev_bucket_id = bucket_from_create->get_bucket_id();
   EXPECT_EQ(
@@ -614,6 +639,9 @@ TEST_F(TestSFSBucket, DeleteBucket) {
   ASSERT_TRUE(b_metadata.has_value());
   EXPECT_FALSE(b_metadata->deleted);
 
+  // mtime of new bucket must be greater than last mtime, no reuse
+  EXPECT_GT(b_metadata->mtime, b_mtime);
+
   // if we query in metadata for buckets with the same name it should
   // return 2 entries.
   auto bucket_name = bucket_from_create->get_name();
@@ -636,8 +664,13 @@ TEST_F(TestSFSBucket, TestListObjectsAndVersions) {
   // create the test user
   createUser("test_user", store->db_conn);
 
+  // keep creation mtime
+  ceph::real_time create_mtime;
+
   // create test bucket
-  createTestBucket("test_bucket", "test_user", store->db_conn, true);
+  createTestBucket(
+      "test_bucket", "test_user", store->db_conn, true, &create_mtime
+  );
 
   // create a few objects in test_bucket with a few versions
   uint version_id = 1;
@@ -677,6 +710,9 @@ TEST_F(TestSFSBucket, TestListObjectsAndVersions) {
 
   ASSERT_NE(bucket_from_store, nullptr);
 
+  // creating an object should not change the bucket's mtime
+  EXPECT_EQ(bucket_from_store->get_modification_time(), create_mtime);
+
   rgw::sal::Bucket::ListParams params;
 
   // list with empty prefix

src/test/rgw/sfs/test_rgw_sfs_wal_checkpoint.cc

@@ -67,7 +67,8 @@ class TestSFSWALCheckpoint : public ::testing::Test {
     RGWUserInfo bucket_owner;
 
     bucket = std::make_shared<Bucket>(
-        cct.get(), store.get(), db_binfo.binfo, bucket_owner, db_binfo.battrs
+        cct.get(), store.get(), db_binfo.binfo, bucket_owner, db_binfo.battrs,
+        db_binfo.mtime
     );
   }