[DRAFT] rgw: Initial exploratory commit for multiple drivers #52665

alimaredia · 2023-07-27T04:16:09Z

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

alimaredia · 2023-07-27T04:17:23Z

src/vstart.sh

-            osd 'allow rwx' \
-            mgr 'allow rw' \
-            >> "$keyring_fn"
+        if [ "$CEPH_NUM_MON" -gt 0 ]; then


This conditional was necessary to run DBStore without a monitor via vstart. The changes in this file are not meant to be committed

alimaredia · 2023-07-27T04:19:46Z

src/rgw/driver/rados/rgw_zone.cc

@@ -1114,6 +1114,11 @@ static int read_or_create_default_zone(const DoutPrefixProvider* dpp,
      return r;
    }
  }
+  r = info.sal_config.set("type", "rados");


@cbodley @dang when/where is it that you envisioned the default backend to be rados? Is it in the scenario that no json config is provided or if that json is missing a store? I put this here as a place holder for that discussion.

My thought is this, and I don't know how hard it is:
The ceph.conf chooses a config provider. If this is not given, it should default to the RADOS config provider.

Each config provider has a default json if none is stored. It should probably be the correct store for that provider (so the RADOS provider will default to radosstore, the DB provider should default to DBStore, the json file provider should default to POSIXStore). If invalid json exists, it should error out.

What do people think about this?

@dang agreed. when a ConfigStore backend's create_zone() function is called with an empty sal_config, it should initialize that for the same kind of store. read_or_create_default_zone() is generic to all ConfigStores, so the default logic doesn't belong here

alimaredia · 2023-07-27T04:26:31Z

src/rgw/rgw_sal.cc

+                                              bool use_gc, optional_yield y) {
+
+  driver = newRadosStore();
+  RGWRados* rados = static_cast<rgw::sal::RadosStore* >(driver)->getRados();


@dang is all of this RadosStore specific initialization supposed to happen right after the driver gets set to a RadosStore? Instead of just passing a dpp, sal_config, and driver to the new functions, I had to also add the other 10 arguments to init_storage_provider() for the time being. Should these be passed into the RadosStores json config? Or is there some other way that you had in mind?

In principle, I suppose, all this can apply to any store, but the way it's supposed to work is that you have a Factory, and you pass the json into the Factory, and it picks the correct factory instance, which understands all the config in that json. So you shouldn't need all these config to be passed in, it will live either in the json, or in the ceph.conf (for the RADOS store specific stuff).

most of these bool options come from ceph.conf, so can be overridden per-radosgw instance. anything we put in the zone json must apply to all radosgw[-admin]s in the zone

alimaredia · 2023-07-27T04:32:58Z

@cbodley: In case you want to play around with this, the simplest workflow using vstart.sh looks like this:

ninja vstart && MON=1 OSD=1 MDS=0 RGW=1 MGR=0 ../src/vstart.sh -n -d
./bin/radosgw-admin -c ceph.conf zone get --rgw-zone=default > infile.json
jq '. |= .+ { "sal_config" : { "type" : "base", "next" : { "type" : "rados" } } }' infile.json > infile_full_sal_config.json
./bin/radosgw-admin -c ceph.conf zone set --rgw-zone=default --infile=infile_full_sal_config.json
kill $(cat out/radosgw.8000.pid)
sleep 2
./bin/radosgw -c ceph.conf --log-file=out/radosgw.8000.log --admin-socket=out/radosgw.8000.asok --pid-file=out/radosgw.8000.pid --rgw_luarocks_location=out/luarocks --debug-rgw=20 --debug-ms=1 -n client.rgw.8000 --rgw_frontends='beast port=8000'

For reasons I'd like to discuss in further detail, hard coding a sal_config when the default zone is created (as seen in driver/rados/rgw_zone.cc) was not working because the RGWZoneInfo's sal_config was cleared out.

alimaredia · 2023-07-27T04:34:36Z

src/rgw/driver/rados/rgw_zone.h

@@ -122,6 +122,7 @@ struct RGWZoneParams : RGWSystemMetaObj {
  std::string realm_id;

  JSONFormattable tier_config;
+  JSONFormattable sal_config;


@dang I ended up choosing sal_config for the name of the json that will be part of the zone's info. Do you have another suggestion for the name? driver_config?

I think sal_config is fine. It's more than drivers (filters too), so SAL is probably appropriate.

dang · 2023-07-27T14:36:43Z

src/rgw/driver/rados/rgw_zone.cc

@@ -1114,6 +1114,11 @@ static int read_or_create_default_zone(const DoutPrefixProvider* dpp,
      return r;
    }
  }
+  r = info.sal_config.set("type", "rados");


My thought is this, and I don't know how hard it is:
The ceph.conf chooses a config provider. If this is not given, it should default to the RADOS config provider.

Each config provider has a default json if none is stored. It should probably be the correct store for that provider (so the RADOS provider will default to radosstore, the DB provider should default to DBStore, the json file provider should default to POSIXStore). If invalid json exists, it should error out.

What do people think about this?

dang · 2023-07-27T14:37:52Z

src/rgw/driver/rados/rgw_zone.h

@@ -122,6 +122,7 @@ struct RGWZoneParams : RGWSystemMetaObj {
  std::string realm_id;

  JSONFormattable tier_config;
+  JSONFormattable sal_config;


I think sal_config is fine. It's more than drivers (filters too), so SAL is probably appropriate.

dang · 2023-07-27T14:43:02Z

src/rgw/rgw_admin.cc

@@ -4238,6 +4242,8 @@ int main(int argc, const char **argv)
      cerr << "couldn't init config storage provider" << std::endl;
      return EIO;
    }
+    JSONFormattable sal_config;
+    sal_config.set("type", "rados");


This isn't going to work, unless it's just a placeholder for now. Currently, radosgw-admin get's it's config from ceph.conf, and will use the correct store. This allows you to create users for DBStore, for example. So going forward, radosgw-admin needs to load the json like the main RGW does, and use the correct store.

dang · 2023-07-27T14:48:45Z

src/rgw/rgw_sal.cc

+                                              bool use_gc, optional_yield y) {
+
+  driver = newRadosStore();
+  RGWRados* rados = static_cast<rgw::sal::RadosStore* >(driver)->getRados();


In principle, I suppose, all this can apply to any store, but the way it's supposed to work is that you have a Factory, and you pass the json into the Factory, and it picks the correct factory instance, which understands all the config in that json. So you shouldn't need all these config to be passed in, it will live either in the json, or in the ceph.conf (for the RADOS store specific stuff).

Signed-off-by: Ali Maredia <amaredia@redhat.com>

build is working but not tested yet Signed-off-by: Ali Maredia <amaredia@redhat.com>

Signed-off-by: Ali Maredia <amaredia@redhat.com>

github-actions · 2024-01-15T21:01:29Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2024-01-26T03:30:47Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

github-actions · 2024-03-26T06:01:41Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

alimaredia · 2024-03-26T08:29:44Z

unstale

github-actions · 2024-05-25T09:01:38Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

dang · 2024-05-28T15:50:20Z

unstale

alimaredia requested a review from a team as a code owner July 27, 2023 04:16

github-actions bot added the rgw label Jul 27, 2023

alimaredia commented Jul 27, 2023

View reviewed changes

alimaredia force-pushed the wip-rgw-sal-config branch from 361700e to 4f94723 Compare July 27, 2023 04:27

alimaredia commented Jul 27, 2023

View reviewed changes

dang reviewed Jul 27, 2023

View reviewed changes

alimaredia added 6 commits November 1, 2023 21:35

rgw: Initial exploratory commit for multiple drivers

f161644

Signed-off-by: Ali Maredia <amaredia@redhat.com>

rgw: initial dbstore from sal_config commit

c4061f8

build is working but not tested yet Signed-off-by: Ali Maredia <amaredia@redhat.com>

REMOVE-ME: vstart changes

3830b8e

Signed-off-by: Ali Maredia <amaredia@redhat.com>

REMOVE-ME: add comment into rgw_admin.cc for repeated sal_configs

31a1f6b

Signed-off-by: Ali Maredia <amaredia@redhat.com>

SQUASH-ME: put default sal config in rgw_zone.cc

5afbe77

Signed-off-by: Ali Maredia <amaredia@redhat.com>

SQUASH-ME: change backend_store to config_store in rgw_global_init

a6cf16c

Signed-off-by: Ali Maredia <amaredia@redhat.com>

alimaredia mentioned this pull request Nov 6, 2023

[DRAFT] RGW: Refactor create_zone out of Rados Store #54370

Closed

14 tasks

alimaredia added 2 commits November 9, 2023 03:25

SQUASH-Me: have zone creation working the way I'd like

87ac7c3

Signed-off-by: Ali Maredia <amaredia@redhat.com>

rgw: adding get_default_sal_config() in read_or_create_default_zone()

e06667f

Signed-off-by: Ali Maredia <amaredia@redhat.com>

alimaredia force-pushed the wip-rgw-sal-config branch from 4f94723 to e06667f Compare November 16, 2023 19:54

github-actions bot added the stale label Jan 15, 2024

github-actions bot added needs-rebase and removed stale labels Jan 26, 2024

github-actions bot added the stale label Mar 26, 2024

github-actions bot removed the stale label Mar 26, 2024

github-actions bot added the stale label May 25, 2024

github-actions bot removed the stale label May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] rgw: Initial exploratory commit for multiple drivers #52665

[DRAFT] rgw: Initial exploratory commit for multiple drivers #52665

alimaredia commented Jul 27, 2023

alimaredia Jul 27, 2023

alimaredia Jul 27, 2023

dang Jul 27, 2023

cbodley Jul 31, 2023

alimaredia Jul 27, 2023

dang Jul 27, 2023

cbodley Jul 31, 2023

alimaredia commented Jul 27, 2023

alimaredia Jul 27, 2023

dang Jul 27, 2023

dang Jul 27, 2023

dang Jul 27, 2023

dang Jul 27, 2023

dang Jul 27, 2023

github-actions bot commented Jan 15, 2024

github-actions bot commented Jan 26, 2024

github-actions bot commented Mar 26, 2024

alimaredia commented Mar 26, 2024

github-actions bot commented May 25, 2024

dang commented May 28, 2024

[DRAFT] rgw: Initial exploratory commit for multiple drivers #52665

Are you sure you want to change the base?

[DRAFT] rgw: Initial exploratory commit for multiple drivers #52665

Conversation

alimaredia commented Jul 27, 2023

Contribution Guidelines

Checklist

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

alimaredia commented Jul 27, 2023

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

github-actions bot commented Jan 15, 2024

github-actions bot commented Jan 26, 2024

github-actions bot commented Mar 26, 2024

alimaredia commented Mar 26, 2024

github-actions bot commented May 25, 2024

dang commented May 28, 2024