RGW - Add POSIX Driver #52933

dang · 2023-08-10T18:58:26Z

This is the MVP for a driver for RGW that operates on top of a POSIX filesystem. It supports get, put, list, copy, multipart, external access via the filesystem itself, and ordered bucket listings via an LRU-based cache.

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
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

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

cbodley

a lot to review, but looks wonderful overall

src/rgw/driver/posix/lmdb-safe.cc

src/rgw/CMakeLists.txt

cbodley · 2023-08-10T19:16:16Z

src/rgw/driver/posix/bucket_cache.h

+#if 1
+	// depends on safe_link
+	if (! name_hook.is_linked()) {
+	  // this should not happen!
+	  abort();
+	}
+#endif


the #if seems unnecessary. can we replace that with an assertion?

well, what I was doing here was a) preferring to use normal_link; but b) have this to print info

it's an #if iow because I'd like this normally to have normal_link; I can templatize this or something later. I don't want asserts or ceph_asserts in the path. if we're going to change this now, make it normal_link in the decl and just remove this block

src/rgw/rgw_sal_filter.h

src/rgw/rgw_op.cc

cbodley · 2023-08-10T20:40:35Z

src/rgw/driver/posix/rgw_sal_posix.h

+  int get_shadow_bucket(const DoutPrefixProvider* dpp, optional_yield y,
+			const std::string& ns, const std::string& tenant,
+			const std::string& name, bool create,
+			std::unique_ptr<POSIXBucket>* shadow);


i take it that 'shadow bucket' is a subdirectory that stores the parts for a given multipart upload? sal::Bucket seems like a very wide interface to represent this. i think we should avoid using the sal interfaces to represent things that aren't s3 buckets or objects. what would a minimal 'shadow directory' interface need, if we created an internal class for it instead?

I did it this way to avoid code duplication.

understood, but you can share code without sharing the interface. i'm just picturing a class POSIXBucket and class MultipartDirectory that both call helper functions that operate on file descriptors and paths

with this separation, it would be much easier to make changes or bugfixes to one without introducing weird regressions in the other

src/rgw/driver/posix/rgw_sal_posix.cc

src/rgw/driver/posix/rgw_sal_posix.h

src/rgw/driver/posix/rgw_sal_posix.cc

thotz · 2023-08-11T06:57:28Z

src/common/options/rgw.yaml.in

@@ -3618,6 +3618,7 @@ options:
  - none
  - base
  - d4n
+  - posix


part of rgw_config_store?

I don't understand the question? In the future (work Ali is working on) all this will be replaced with JSON stored in the zonegroup, but for now it has to live in ceph.conf.

i don't think it's necessary to have a posix-file backend for rgw::sal::ConfigStore. rgw_config_store=dbstore already works on a local filesystem

is "JSON stored in the zonegroup" equivalent to creating a posix-file backend for rgw::sal::ConfigStore? I don't follow

is "JSON stored in the zonegroup" equivalent to creating a posix-file backend for rgw::sal::ConfigStore? I don't follow

No, it describes the driver stack, so it's a replacement for rgw_backend_store and rgw_filter currently in ceph.conf. The actual zonegroup is stored in the ConfigStore, so whichever ConfigStore you use will store the JSON that describes the driver stack. I agree with Casey, we'll probably use DBStore's ConfigStore for POSIXDriver.

part of rgw_config_store?

Ah, was the original question asking if this should be in rgw_config_store instead of rgw_filter? If so, then yes, eventually. Currently, POSIXDriver is a filter, since it doesn't implement any User APIs, but instead depends on DBUser. So it has to be a filter stacked on top of DBStore until we finish the User APIs.

dang · 2023-08-29T12:10:29Z

jenkins test api

dang · 2023-08-29T12:11:34Z

https://pulpito.ceph.com/dang-2023-08-25_09:21:58-rgw-wip-dang-posix-driver-distro-default-smithi/

This isn't a great run, but it's all valgind and health warns, nothing that could be caused by the (small) refactors that aren't in posixdriver.

dang · 2023-09-07T12:13:49Z

https://pulpito.ceph.com/dang-2023-09-06_10:06:10-rgw-wip-dang-posix-driver-distro-default-smithi/

Finally a clean run. Are we good? Or are there other changes to make?

cbodley · 2023-09-12T18:52:51Z

jenkins test windows

cbodley · 2023-09-12T18:54:58Z

src/test/rgw/CMakeLists.txt

+# unittest_posix_bucket_cache
+add_executable(unittest_posix_bucket_cache
+	test_posix_bucket_cache.cc)
+target_compile_definitions(unittest_posix_bucket_cache PUBLIC LMDB_SAFE_NO_CPP_UTILITIES)
+target_include_directories(unittest_posix_bucket_cache
+  SYSTEM PRIVATE "${CMAKE_SOURCE_DIR}/src/rgw"
+  SYSTEM PRIVATE "${CMAKE_SOURCE_DIR}/src/rgw/driver/posix")
+target_link_libraries(unittest_posix_bucket_cache  ${UNITTEST_LIBS}
+  ${rgw_libs} ${LMDB_LIBRARIES})


there should be a add_ceph_unittest() to make this run during 'make check'

cbodley

good to merge once we see unittest_posix_bucket_cache passing in 'make check'

cbodley · 2023-09-13T19:04:31Z

195/286 Test #232: unittest_posix_bucket_cache ...............***Failed 17.59 sec
...
[==========] 23 tests from 1 test suite ran. (17392 ms total)
[ PASSED ] 20 tests.
[ FAILED ] 3 tests, listed below:
[ FAILED ] BucketCache.ListMarker1
[ FAILED ] BucketCache.ListInotify1
[ FAILED ] BucketCache.List2Inotify1

dang · 2023-09-14T12:59:53Z

195/286 Test #232: unittest_posix_bucket_cache ...............***Failed 17.59 sec
...
[==========] 23 tests from 1 test suite ran. (17392 ms total)
[ PASSED ] 20 tests.
[ FAILED ] 3 tests, listed below:
[ FAILED ] BucketCache.ListMarker1
[ FAILED ] BucketCache.ListInotify1
[ FAILED ] BucketCache.List2Inotify1

Yep, I'm looking at it.

This is the MVP for a driver for RGW that operates on top of a POSIX filesystem. It supports get, put, list, copy, multipart, external access via the filesystem itself, and ordered bucket listings via an LRU-based cache. Note that this is currently a Filter, indended to run on top of dbstore. This is because it currently doesn't have any User implementation, so it depends on dbstore's User. Everything else is implemented in POSIXDriver. Once there is a User implementation, this will become a Store, instead of a Filter. Commit messages from bucket listing cache: rgw/posixdriver: recycle lmdb database handles as required While LMDB workflows often do not close/return database handles, ours continually reuses them. This requires us to close each handle (atomically) when a cache entry is recycled. rgw/posixdriver: don't instantiate bucket cache entries from notify events rgw/posixdriver: incorporate lmdb-safe for now The current inclusion is based on https://github.com/Martchus/lmdb-safe, which is actively maintained but currently has some packaging issues the author has agreed to accept fixes for. For now, skip the submodule to save time and remove an external dependency. rgw/posixdriver: fix listing of cached, empty bucket * check lmdb enumeration result in all cases and w/better style * add unit test for enumeration of an empty cached directory rgw/posixdriver: nest lmdbs in a directory under the dbroot path to avoid cleanup issues rgw/posixdriver: refactor for posix integration * Derive BucketCache types as templates on a SAL driver and SAL bucket pair. * Integrate cache fills as callbacks into SAL layer (or mock, for tests) * Renaming and cleanups rgw/posixdriver: add bucket cache implementation and tests Adds free-standing cache of buckets and object names, with bucket names (and listing attributes, upcoming) managed in a hashed set of lmdb databases, which provides ordering and a high-performance listing cache. An framework for notification on new object creation (e.g., outside S3 workflow) is provided, and a Linux implementation using inotify. FindLMDB.cmake taken with attribution and license. rgw/posixdriver: add zpp_bits serialization (FAST) Signed-off-by: Daniel Gryniewicz <dang@redhat.com> Signed-off-by: Ali Maredia <amaredia@redhat.com> Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>

dang · 2023-09-14T17:19:16Z

jenkins test make check

mattbenjamin

lgtm (mint_listing_entry)

mattbenjamin · 2023-09-14T17:45:43Z

src/rgw/driver/posix/rgw_sal_posix.cc

+    POSIXObject *pobj;
+    int ret;
+
+    ret = get_bucket(nullptr, nullptr, std::string(), bname, &b, null_yield);


mattbenjamin · 2023-09-15T13:58:24Z

yay

dang requested review from mattbenjamin, cbodley, alimaredia and soumyakoduri August 10, 2023 18:58

dang requested a review from a team as a code owner August 10, 2023 18:58

github-actions bot added build/ops common rgw tests labels Aug 10, 2023

cbodley reviewed Aug 10, 2023

View reviewed changes

thotz reviewed Aug 11, 2023

View reviewed changes

dang force-pushed the wip-dang-posix-driver branch 2 times, most recently from f38b739 to 8e44ec3 Compare August 24, 2023 15:34

dang force-pushed the wip-dang-posix-driver branch from 8e44ec3 to a3f8154 Compare August 30, 2023 14:11

github-actions bot added the documentation label Aug 30, 2023

cbodley reviewed Sep 12, 2023

View reviewed changes

cbodley approved these changes Sep 12, 2023

View reviewed changes

dang force-pushed the wip-dang-posix-driver branch from a3f8154 to e99f00f Compare September 13, 2023 18:04

dang force-pushed the wip-dang-posix-driver branch 2 times, most recently from 265527c to 3ff66c7 Compare September 14, 2023 15:21

dang force-pushed the wip-dang-posix-driver branch from 3ff66c7 to 5258bcb Compare September 14, 2023 16:09

mattbenjamin approved these changes Sep 14, 2023

View reviewed changes

dang merged commit a4b1913 into ceph:main Sep 15, 2023
10 of 11 checks passed

cbodley mentioned this pull request Sep 19, 2023

rgw/crypt: don't deref null manifest_bl #53448

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RGW - Add POSIX Driver #52933

RGW - Add POSIX Driver #52933

dang commented Aug 10, 2023 •

edited

cbodley left a comment

cbodley Aug 10, 2023

mattbenjamin Aug 10, 2023

mattbenjamin Aug 14, 2023

cbodley Aug 10, 2023

dang Aug 11, 2023

cbodley Aug 11, 2023

thotz Aug 11, 2023

dang Aug 11, 2023

cbodley Aug 11, 2023

mattbenjamin Aug 14, 2023 •

edited

dang Aug 17, 2023

dang Aug 17, 2023

dang commented Aug 29, 2023

dang commented Aug 29, 2023

dang commented Sep 7, 2023

cbodley commented Sep 12, 2023

cbodley Sep 12, 2023

cbodley left a comment

cbodley commented Sep 13, 2023

dang commented Sep 14, 2023

dang commented Sep 14, 2023

mattbenjamin left a comment

mattbenjamin Sep 14, 2023

mattbenjamin commented Sep 15, 2023

@@ @@ -3618,6 +3618,7 @@ options: @@
                 - none
                 - base
                 - d4n
+                - posix

RGW - Add POSIX Driver #52933

RGW - Add POSIX Driver #52933

Conversation

dang commented Aug 10, 2023 • edited

Contribution Guidelines

Checklist

cbodley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbenjamin Aug 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dang commented Aug 29, 2023

dang commented Aug 29, 2023

dang commented Sep 7, 2023

cbodley commented Sep 12, 2023

Choose a reason for hiding this comment

cbodley left a comment

Choose a reason for hiding this comment

cbodley commented Sep 13, 2023

dang commented Sep 14, 2023

dang commented Sep 14, 2023

mattbenjamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbenjamin commented Sep 15, 2023

dang commented Aug 10, 2023 •

edited

mattbenjamin Aug 14, 2023 •

edited