Rename database file to sfs.db #245

irq0 · 2023-11-13T16:58:29Z

Use 'sfs.db' instead of 's3gw.db'. Rename 's3gw.db' -> 'sfs.db' if it
exists on startup.

Fixes: https://github.com/aquarist-labs/s3gw/issues/766

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Use constants DB_FILENAME and DB_WAL_FILENAME to refer to our database filename. Signed-off-by: Marcel Lauhoff <[email protected]>

Signed-off-by: Marcel Lauhoff <[email protected]>

irq0 · 2023-11-13T18:42:57Z

The last commit in chain has basic migration code. Happy to drop it

jecluis · 2023-11-13T19:41:24Z

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

@@ -414,4 +416,24 @@ void DBConn::maybe_upgrade_metadata() {
  }
 }

+void DBConn::maybe_rename_database_file() const {


I suspect we may also have to rename the -wal and -shm files. I think those can be left behind if the database is not properly closed.

Yeah, if you killall -9 radosgw the -wal and -shm files will remain, then the next time you start it up, you'll see something like [SQLITE] (283) recovered 26 frames from WAL file /scratch/s3gw/qa/s3gw.db-wal in the log. So if we keep the migration code, we'd need to migrate those two as well.

Thinking about it, the rename code is a bit naive. Perhaps too naive. Not only do we need to rename the extra files, there may also be temporary files (according to docs) that share the basename. If the database is still open renaming is also a big mistake.

A safer option would be the backup API - I think we should rather use that

tserong

LGTM, but I'm in favour of dropping the migration code and just putting a (large, bold) note in the release announcement that the DB name has changed and that existing deployments need to manually mv s3gw.db sfs.db.

jecluis · 2023-11-14T07:54:39Z

LGTM, but I'm in favour of dropping the migration code and just putting a (large, bold) note in the release announcement that the DB name has changed and that existing deployments need to manually mv s3gw.db sfs.db.

This would not be feasible in kubernetes. Possible, yes, but asking that from the user would be annoying. We either assume their volumes are to be blown away, or we do the migration.

TBH, I'm in favor of doing the migration (it doesn't look too difficult or error prone), and we can always remove it further down the line, before GA.

But this also really depends on when we want to consider the on-disk format "stable". If only after this, then blow away; but if we think we are already there, then migration is the way to go.

tserong · 2023-11-14T10:37:46Z

TBH, I'm in favor of doing the migration (it doesn't look too difficult or error prone), and we can always remove it further down the line, before GA.

That'll work. Put the migration in now, drop it in the next release or next release +1 and expect current early adopters to remain on the train (and mention these changes in release notes as we go). I just didn't want to keep that code forever.

Signed-off-by: Marcel Lauhoff <[email protected]>

irq0 · 2023-11-15T17:09:35Z

Last push changes the migration code to use the sqlite3 backup API

jecluis

minor things, lgtm

jecluis · 2023-11-15T18:09:48Z

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

+  if (!std::filesystem::exists(getLegacyDBPath(cct))) {
+    return;
+  }
+  if (std::filesystem::exists(getDBPath(cct))) {
+    return;
+  }


this could have been a single if

jecluis · 2023-11-15T18:11:32Z

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

+    return db_path.string();
+  }
+
+  static std::string getLegacyDBPath(CephContext* cct) {


to be perfectly honest, I really dislike that we have this function as camelCase when all other functions are snake case. More of an itch for me than anything else.

jecluis · 2023-11-15T18:16:00Z

src/test/rgw/sfs/test_rgw_sfs_wal_checkpoint.cc

@@ -78,7 +79,7 @@ class TestSFSWALCheckpoint : public ::testing::Test {
      size_t num_threads, size_t num_objects
  ) {
    std::atomic<std::uintmax_t> max_wal_size{0};
-    fs::path wal(test_dir / "s3gw.db-wal");
+    fs::path wal(test_dir / sqlite::DB_WAL_FILENAME);


can we maybe define this as a function of DB_FILENAME, instead of having to keep a #define in the header solely for this test?

That would leak an sqlite implementation detail that a reader does not need to know about. I'd rather have this

Since aquarist-labs/ceph#245 rgw/sfs uses 'sfs.db' instead of 's3gw.db' Signed-off-by: Marcel Lauhoff <[email protected]>

Marcel Lauhoff added 2 commits November 13, 2023 14:46

rgw/sfs: Replace "s3gw.db" with constant

6aa58cf

Use constants DB_FILENAME and DB_WAL_FILENAME to refer to our database filename. Signed-off-by: Marcel Lauhoff <[email protected]>

rgw/sfs: Change database basename to 'sfs'

69c0648

Signed-off-by: Marcel Lauhoff <[email protected]>

irq0 requested review from jecluis, tserong and 0xavi0 November 13, 2023 18:42

jecluis reviewed Nov 13, 2023

View reviewed changes

tserong previously approved these changes Nov 14, 2023

View reviewed changes

rgw/sfs: Rename legacy database filename to new if exists on startup

834fe55

Signed-off-by: Marcel Lauhoff <[email protected]>

irq0 dismissed tserong’s stale review via 834fe55 November 15, 2023 17:07

irq0 force-pushed the pr/database-file-rename branch from 68c6dea to 834fe55 Compare November 15, 2023 17:07

jecluis approved these changes Nov 15, 2023

View reviewed changes

jecluis added this to the v0.23.0 milestone Nov 15, 2023

jecluis added kind/enhancement Change that positively impacts existing code area/rgw-sfs RGW & SFS related priority/0 Needs to go into the next release or force a patch labels Nov 15, 2023

jecluis assigned irq0 Nov 16, 2023

irq0 merged commit 53aa541 into aquarist-labs:s3gw Nov 16, 2023
8 checks passed

irq0 deleted the pr/database-file-rename branch November 16, 2023 09:47

irq0 pushed a commit to irq0/fsck.sfs that referenced this pull request Nov 16, 2023

Change DB_FILENAME to sfs.db

4f3d9bc

Since aquarist-labs/ceph#245 rgw/sfs uses 'sfs.db' instead of 's3gw.db' Signed-off-by: Marcel Lauhoff <[email protected]>

irq0 mentioned this pull request Nov 16, 2023

Change DB_FILENAME to sfs.db s3gw-tech/fsck.sfs#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename database file to sfs.db #245

Rename database file to sfs.db #245

irq0 commented Nov 13, 2023

irq0 commented Nov 13, 2023

jecluis Nov 13, 2023

tserong Nov 14, 2023

irq0 Nov 14, 2023

tserong left a comment

jecluis commented Nov 14, 2023

tserong commented Nov 14, 2023

irq0 commented Nov 15, 2023

jecluis left a comment

jecluis Nov 15, 2023

jecluis Nov 15, 2023

jecluis Nov 15, 2023

irq0 Nov 16, 2023

Rename database file to sfs.db #245

Rename database file to sfs.db #245

Conversation

irq0 commented Nov 13, 2023

Checklist

irq0 commented Nov 13, 2023

jecluis Nov 13, 2023

Choose a reason for hiding this comment

tserong Nov 14, 2023

Choose a reason for hiding this comment

irq0 Nov 14, 2023

Choose a reason for hiding this comment

tserong left a comment

Choose a reason for hiding this comment

jecluis commented Nov 14, 2023

tserong commented Nov 14, 2023

irq0 commented Nov 15, 2023

jecluis left a comment

Choose a reason for hiding this comment

jecluis Nov 15, 2023

Choose a reason for hiding this comment

jecluis Nov 15, 2023

Choose a reason for hiding this comment

jecluis Nov 15, 2023

Choose a reason for hiding this comment

irq0 Nov 16, 2023

Choose a reason for hiding this comment