rgw/sfs: Implement new versioning design #153

0xavi0 · 2023-06-07T15:36:39Z

Implements new versioning design following this ADR

Includes rgw::sal::Attrs type binding in sqliteorm code, so VersionedObject code for conversions is no longer needed.

Any value that was a BLOB before and that it's not optional should be good to be bound.
Just declare the type in the database struct and add this:
```
 #include "rgw/driver/sfs/sqlite/bindings/blob.h"
```
That file uses the previous code to convert ceph types using bufferlist ("rgw/driver/sfs/sqlite/conversion_utils.h")

Extra code for dealing with optional BLOB will be proposed in a future PR and we'll be able to delete all the conversion
layers where no strictly needed.
Adds transactions for the actions that require more than 1 database access.
Simplifies the way a new version is created and moves its logic to a sqlite transaction to avoid race conditions when creating N objects with the same name at once from several threads.
Also changed the name of the objects and versioned_objects table names to DBObject and DBVersionedObject

The following s3 tests now pass:

test_bucket_list_return_data_versioning
test_object_copy_versioning_multipart_upload
test_versioning_bucket_create_suspend
test_versioning_obj_create_read_remove
test_versioning_obj_create_read_remove_head
test_versioning_obj_create_versions_remove_all
test_versioning_obj_create_versions_remove_special_names
test_versioning_obj_create_overwrite_multipart
test_versioning_obj_list_marker
test_versioning_copy_obj_version
test_versioning_multi_object_delete
test_versioning_multi_object_delete_with_marker
test_versioning_multi_object_delete_with_marker_create
test_lifecycle_expiration_versioning_enabled
test_versioning_bucket_atomic_upload_return_version_id
test_versioning_bucket_multipart_upload_return_version_id
test_object_lock_suspend_versioning
test_object_copy_versioned_bucket
test_object_copy_versioned_url_encoding
test_multipart_copy_versioned
test_versioned_object_acl
test_versioned_object_acl_no_version_specified
test_versioned_concurrent_object_create_concurrent_remove
test_versioned_concurrent_object_create_and_remove
test_multi_object_delete
test_multi_objectv2_delete
test_bucket_recreate_not_overriding

Fixes: https://github.com/aquarist-labs/s3gw/issues/378
Fixes: https://github.com/aquarist-labs/s3gw/issues/472
Fixes: https://github.com/aquarist-labs/s3gw/issues/547
Fixes: https://github.com/aquarist-labs/s3gw/issues/526
Fixes: https://github.com/aquarist-labs/s3gw/issues/524
Fixes: https://github.com/aquarist-labs/s3gw/issues/519

Signed-off-by: Xavi Garcia xavi.garcia@suse.com

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

jecluis

generally looks good, but a few comments I'd like answers to before approving or requesting changes :)

src/rgw/driver/dbstore/common/dbstore.h

src/rgw/driver/sfs/object.cc

src/rgw/driver/sfs/sqlite/versioned_object/versioned_object_definitions.h

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

src/rgw/driver/sfs/types.cc

jecluis

cleanup related, otherwise looks good.

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

src/rgw/driver/sfs/types.cc

irq0

looks good!

I like how the code now leverages more of sqlite_orm.

General comments:

Some of the sql utility methods would benefit from names that imply if deleted is included or now.

The '_' prefix. I've seen it in the python word for private methods, but not much in c++. What does it mean here? Just private method? Utility method? Method doing actual work (like the do_ prefix in many parts of the ceph codebase)

irq0 · 2023-06-08T12:03:08Z

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

+  return _get_versioned_object(bucket_id, object_name, version_id);
+}
+
+DBObjectsListItems SQLiteVersionedObjects::list_last_versioned_objects(


Suggest to include the state the objects are in. The name does not imply that deleted objects are filtered out.

okay, sounds good to me. I've renamed it to get_non_deleted_versioned_object

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

irq0 · 2023-06-08T12:36:46Z

src/rgw/driver/sfs/bucket.cc

@@ -238,12 +256,10 @@ bool SFSBucket::is_owner(User* user) {
 int SFSBucket::check_empty(const DoutPrefixProvider* dpp, optional_yield y) {


(Goold candidate to turn into an sql query in a future PR. )

Q: What is empty?
there is no committed version?
there is no committed or open (== in flight) version?
🤔 is a bucket empty if there are multpart uploads going on?

Yup, I wonder if there are any s3 tests for this.

Surprising to me check_empty is only called from RGWDeleteBucket::execute before doing the remove_bucket call. Surprising because I would have expected that to be part of remove_bucket

The answer here needs to be, I think, that the multipart uploads are simply aborted if the bucket is removed. The multipart upload is a promise of eventually creating something in that bucket, and as such I don't think it's reasonable to hold the bucket to exist just because someone forgot to complete or abort a multipart upload.

Good to know, thanks!

jecluis · 2023-06-09T05:49:47Z

@irq0 are you satisfied with the changes? Anything else comes to mind? I'd love to merge this soon-ish :)

0xavi0 · 2023-06-09T08:10:07Z

I'll merge as soon as CI passes

Implements new versioning design. Includes `rgw::sal::Attrs` type binding in `sqliteorm` code, so VersionedObject code for conversions is no longer needed. Adds transactions for the actions that require more than 1 database access. Simplifies the way a new version is created and moves its logic to a sqlite transaction to avoid race conditions when creating N objects with the same name at once from several threads. Fixes: https://github.com/aquarist-labs/s3gw/issues/378 Fixes: https://github.com/aquarist-labs/s3gw/issues/472 Fixes: https://github.com/aquarist-labs/s3gw/issues/547 Fixes: https://github.com/aquarist-labs/s3gw/issues/526 Fixes: https://github.com/aquarist-labs/s3gw/issues/524 Fixes: https://github.com/aquarist-labs/s3gw/issues/519 Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

After [versioning](aquarist-labs#153) was merged there are a few more s3 tests passing. This adds 3 Life cycle plus 1 bucket creation s3 test. Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

After [versioning](#153) was merged there are a few more s3 tests passing. This adds 3 Life cycle plus 1 bucket creation s3 test. Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

After [versioning](aquarist-labs#153) was merged there are a few more s3 tests passing. This adds 3 Life cycle plus 1 bucket creation s3 test. Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

After [versioning](#153) was merged there are a few more s3 tests passing. This adds 3 Life cycle plus 1 bucket creation s3 test. Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

0xavi0 added kind/enhancement Change that positively impacts existing code kind/quality Quality improvements, Refactoring, Automation via CI, E2E, Integration, CLI or REST API labels Jun 7, 2023

0xavi0 self-assigned this Jun 7, 2023

0xavi0 force-pushed the 472-new-versioning branch from 09f046d to c988114 Compare June 7, 2023 15:39

0xavi0 requested review from irq0 and jecluis June 7, 2023 15:39

0xavi0 force-pushed the 472-new-versioning branch 2 times, most recently from 6d645aa to 9aa8030 Compare June 8, 2023 07:57

0xavi0 mentioned this pull request Jun 8, 2023

Rewrite bucket listing leveraging SQL queries and indices s3gw-tech/s3gw#509

Closed

0xavi0 force-pushed the 472-new-versioning branch from 9aa8030 to def1dd6 Compare June 8, 2023 08:13

jecluis reviewed Jun 8, 2023

View reviewed changes

0xavi0 force-pushed the 472-new-versioning branch from def1dd6 to ada0866 Compare June 8, 2023 11:12

0xavi0 requested a review from jecluis June 8, 2023 11:13

jecluis suggested changes Jun 8, 2023

View reviewed changes

src/rgw/driver/sfs/sqlite/sqlite_versioned_objects.cc Outdated Show resolved Hide resolved

src/rgw/driver/sfs/types.cc Outdated Show resolved Hide resolved

0xavi0 force-pushed the 472-new-versioning branch from ada0866 to a32f63e Compare June 8, 2023 12:05

0xavi0 requested a review from jecluis June 8, 2023 12:06

irq0 reviewed Jun 8, 2023

View reviewed changes

jecluis previously approved these changes Jun 8, 2023

View reviewed changes

0xavi0 dismissed jecluis’s stale review via 2a17d35 June 8, 2023 13:30

0xavi0 force-pushed the 472-new-versioning branch from a32f63e to 2a17d35 Compare June 8, 2023 13:30

0xavi0 requested review from irq0 and jecluis June 8, 2023 13:39

0xavi0 force-pushed the 472-new-versioning branch 2 times, most recently from eb150a2 to 8bd78d5 Compare June 8, 2023 14:12

jecluis previously approved these changes Jun 9, 2023

View reviewed changes

irq0 previously approved these changes Jun 9, 2023

View reviewed changes

0xavi0 dismissed irq0’s stale review via 86792d9 June 9, 2023 10:10

0xavi0 dismissed jecluis’s stale review via 86792d9 June 9, 2023 10:10

0xavi0 force-pushed the 472-new-versioning branch from 8bd78d5 to 86792d9 Compare June 9, 2023 10:10

jecluis approved these changes Jun 9, 2023

View reviewed changes

0xavi0 merged commit b32d1e7 into aquarist-labs:s3gw Jun 9, 2023

0xavi0 mentioned this pull request Jun 12, 2023

rgw/testing: Add a few more passing s3tests #155

Merged

11 tasks

This was referenced Jun 12, 2023

[BUG] rgw/sfs: SegFault when running a copyObject command s3gw-tech/s3gw#539

Closed

UI: Do not delete object by version s3gw-tech/s3gw#576

Closed

0xavi0 mentioned this pull request Jun 12, 2023

[BUG] Get unversioned bucket objects with versionId == null s3gw-tech/s3gw#577

Closed

giubacc mentioned this pull request Jul 7, 2023

[BUG] rgw/sfs: creating a bucket that already exists is a success operation s3gw-tech/s3gw#617

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rgw/sfs: Implement new versioning design #153

rgw/sfs: Implement new versioning design #153

0xavi0 commented Jun 7, 2023 •

edited

Loading

jecluis left a comment

jecluis left a comment

irq0 left a comment •

edited

Loading

irq0 Jun 8, 2023

0xavi0 Jun 8, 2023

irq0 Jun 8, 2023

0xavi0 Jun 8, 2023

irq0 Jun 8, 2023

jecluis Jun 9, 2023

irq0 Jun 9, 2023

jecluis commented Jun 9, 2023

0xavi0 commented Jun 9, 2023

		@@ -238,12 +256,10 @@ bool SFSBucket::is_owner(User* user) {
		int SFSBucket::check_empty(const DoutPrefixProvider* dpp, optional_yield y) {

rgw/sfs: Implement new versioning design #153

rgw/sfs: Implement new versioning design #153

Conversation

0xavi0 commented Jun 7, 2023 • edited Loading

Checklist

jecluis left a comment

Choose a reason for hiding this comment

jecluis left a comment

Choose a reason for hiding this comment

irq0 left a comment • edited Loading

Choose a reason for hiding this comment

irq0 Jun 8, 2023

Choose a reason for hiding this comment

0xavi0 Jun 8, 2023

Choose a reason for hiding this comment

irq0 Jun 8, 2023

Choose a reason for hiding this comment

0xavi0 Jun 8, 2023

Choose a reason for hiding this comment

irq0 Jun 8, 2023

Choose a reason for hiding this comment

jecluis Jun 9, 2023

Choose a reason for hiding this comment

irq0 Jun 9, 2023

Choose a reason for hiding this comment

jecluis commented Jun 9, 2023

0xavi0 commented Jun 9, 2023

0xavi0 commented Jun 7, 2023 •

edited

Loading

irq0 left a comment •

edited

Loading