mgr/cephadm: simplify handling for rgw by liewegas · Pull Request #39877 · ceph/ceph

liewegas · 2021-03-05T21:01:44Z

radosgw-admin fiddling with zone/realm config is out of scope
remove naming constraints
keep the --rgw-zone= and --rgw-realm= arguments, but make them optional. if omitted, we'll get the simple non-multisite rgw behavior with autocreated pools etc.
update rook to match

Existing octopus deployments will continue to work: they follow the old naming convention, and we're just allowing new specs to deviate from that convention.

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

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 api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

ofriedma

I think that we should add rgw-zonegroup parameter too so we will be able to create and use non-default zonegroup name, otherwise LGTM

liewegas · 2021-03-07T14:59:55Z

I think that we should add rgw-zonegroup parameter too so we will be able to create and use non-default zonegroup name, otherwise LGTM

Since cephadm isn't creating the zone, zonegroup, or realm any more, I don't think adding the argument will serve any purpose. IIUC, there isn't any rgw_zonegroup parameter needed during startup... setting the rgw_realm and rgw_zone options for the newly created service is the only reason we are providing those parameters at this point.

liewegas · 2021-03-07T15:00:07Z

jenkins run make test

src/python-common/ceph/deployment/service_spec.py

doc/cephadm/rgw.rst

src/pybind/mgr/cephadm/tests/test_cephadm.py

src/pybind/mgr/cephadm/tests/test_spec.py

jmolmo · 2021-03-09T10:14:04Z

src/python-common/ceph/deployment/service_spec.py

-    def validate(self) -> None:
-        super(RGWSpec, self).validate()
-
-        if not self.rgw_realm:


I think this check now is the opposite.....
If the user sets a realm and a zone ... It is worth to check if the realm and the zone exists ?
What if the user only specifies the realm or the zone .... should we use "default" for the missing piece?

jmolmo

I think you want to keep the orchestrator as a thin management layer, and this change really goes in that direction.
My only concern is that with this approach cephadm does not fully replace other installation tools.

Keep simple the orchestrator layer will help to provide a consistent interface (baremetal, k8s) is positive .. but in my opinion we must work in parallel in providing tools that make the final user life more easy, and over all, if we change the installation method we must avoid to lose functionality if we compare with previous installation tools.

With this change I understand that the Ceph dashboard is going to be the only tool provided to install and manage a Ceph cluster. Is that true?

epuertat · 2021-03-09T12:18:21Z

@liewegas I lack the context for this change, as I don't find any tracker/reference linked.

Please correct me if I'm wrong, but as a side effect of this, from now on when ceph-dashboard implements RGW zones/etc. management (which is a goal for Quincy), we'll have to figure out another mechanism/tool providing orchestrator-like CLI access (since radosgw-admin is not available as MonCommand), and then additionally call the orchestrator for deploying/restarting/removing the impacted RGW daemons accordingly. Why not keeping zone/zg/realm config changes tightly coupled with their associated service lifecycles, rather than spreading their management across different tools and interfaces?

I don't know about DeepSea, but compared to Ceph-ansible this is opening a feature gap, as it supported multi-site deployment.

liewegas · 2021-03-09T13:51:42Z

@liewegas I lack the context for this change, as I don't find any tracker/reference linked.

Please correct me if I'm wrong, but as a side effect of this, from now on when ceph-dashboard implements RGW zones/etc. management (which is a goal for Quincy), we'll have to figure out another mechanism/tool providing orchestrator-like CLI access (since radosgw-admin is not available as MonCommand), and then additionally call the orchestrator for deploying/restarting/removing the impacted RGW daemons accordingly. Why not keeping zone/zg/realm config changes tightly coupled with their associated service lifecycles, rather than spreading their management across different tools and interfaces?

I don't know about DeepSea, but compared to Ceph-ansible this is opening a feature gap, as it supported multi-site deployment.

I would put it this way: responsibility for fiddling with the zone/realm configuration doesn't belong in cephadm, because realm configuration is very complex and cephadm lacks the knowledge of the users' intentions to do it well. The ceph-ansible support for setting up multisite assumes a particular vanilla kind of deployment, but we need to be able to deploy daemons for more complex multisite configurations.

Alternatives:

put a series of more friendly commands in mgr/orchestrator, where it could be leveraged by both rook and cephadm, or
create a new 'ceph rgw ...' CLI subnamespace (which tbh is where i wish we could move all of radosgw-admin). This would be more analogous to mgr/volumes, which creates a 'ceph fs volume create' command that (as a last step) tells the orchestrator API to start up the MDS daemons.

To get a simplified experience, I think we should start with what the CLI/GUI UX should look like, and then work backwards from there.

The main part I'm worried about is if/how we should allow you to define a multisite realm configuration in an orchestrator .spec file... is that something we should support? If so, then we'll probably have some bit in cephadm that takes the blob describing the multisite config an calls out to generic orchestrator code to go make it so before deploying the services...

sebastian-philipp · 2021-03-09T13:56:07Z

The main part I'm worried about is if/how we should allow you to define a multisite realm configuration in an orchestrator .spec file... is that something we should support? If so, then we'll probably have some bit in cephadm that takes the blob describing the multisite config an calls out to generic orchestrator code to go make it so before deploying the services...

If we want to have some form of cluster specification to be able to repeatedly create the cluster, we would need to support this somehow. I think this is supported by ansible, right?

liewegas · 2021-03-09T14:34:52Z

The main part I'm worried about is if/how we should allow you to define a multisite realm configuration in an orchestrator .spec file... is that something we should support? If so, then we'll probably have some bit in cephadm that takes the blob describing the multisite config an calls out to generic orchestrator code to go make it so before deploying the services...

If we want to have some form of cluster specification to be able to repeatedly create the cluster, we would need to support this somehow. I think this is supported by ansible, right?

Yeah. So, maybe there is a module (in mgr/orchestrator somewhere) that knows how to parse a section describing the realm configuration and apply it? Or maybe rgw already has such a way to fully specify the config in one go.

liewegas · 2021-03-09T14:36:25Z

Or, maybe services can have "pre-exec" steps, which are just CLI commands to exec in a shell before the service is created, somewhat analogous to the new config section. Kind of a kludge, but sufficient.

sebastian-philipp · 2021-03-09T14:45:21Z

Yeah. So, maybe there is a module (in mgr/orchestrator somewhere) that knows how to parse a section describing the realm configuration and apply it? Or maybe rgw already has such a way to fully specify the config in one go.

This is getting big pretty quickly. https://github.com/rook/rook/blob/master/design/ceph/object/ceph-multisite-overview.md#ceph-multisite-data-model

realms:
  myrealm1: 
    zone_groups:
    - group1
    - group2
    master_zone_group: group1
    pull_from: ...
zone_groups:
  group1: 
    zones:
    - zone1  
    - zone2
    master_zone: zone1
zones:
  zone1:
    pools: ...

this looks pretty unrealistic to build just for cephadm.

It is simpler to consider this out of scope for the orchestrator. The user should set up their multisite realms/zones before deploying the daemons (or the daemons will not start). In the future we can wrap this with a more friendly tool, perhaps. Signed-off-by: Sage Weil <sage@newdream.net>

- Let users name the rgw service(s) whatever they like - Make the rgw_zone and rgw_realm arguments optional, so that they can omit them and let radosgw start up in the generic single-cluster configuration (whichk, notably, has no realm). - Continue to set the rgw_realm and rgw_zone options for the daemon(s), but only when those values are specified. - Adjust the CLI accordingly. Drop the subcluster argument, which was only used to generate a service_id. - Adjust rook. This is actually a simplification and improved mapping onto the rook CRD, *except* that for octopus we enforced the realm.zone naming *and* realm==zone. I doubt a single user actually did this so it is not be worth dealing with, but we need a special case for where there is a . in the service name. Signed-off-by: Sage Weil <sage@newdream.net>

This is just so we can load up a stored spec after upgrade. We'll silently drop it, since we have the service_id, and this was only used to generate that anyway. Signed-off-by: Sage Weil <sage@newdream.net>

github-actions · 2021-03-09T16:37:21Z

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

Signed-off-by: Sage Weil <sage@newdream.net>

Note that cephadm.py will no longer do anything with rgw realms and zones. That means that the setup of rgw roles here is only useful for the default zone and a non-multisite config. Signed-off-by: Sage Weil <sage@newdream.net>

Signed-off-by: Sage Weil <sage@newdream.net>

This is for backward compatibility: an octopus spec yaml can still be applied to an existing cluster. Note that it might not work on a new cluster, since cephadm no longer tries to create the realm or zone if they don't exist. Signed-off-by: Sage Weil <sage@newdream.net>

sebastian-philipp · 2021-03-09T23:20:55Z

@ofriedma want to review this?

ofriedma · 2021-03-10T11:36:20Z

LGTM

sebastian-philipp · 2021-03-10T12:50:24Z

sage: SebastianW: #39877 is passing tests

sebastian-philipp · 2021-03-10T13:00:16Z

Follow-ups:

Would be great to have https://docs.ceph.com/en/octopus/cephadm/install/#deploy-rgws updated to recommend using --rgw-realm and --rgw-zone explicitly
We're missing RGW in the cephad/upgrade suite

…ce spec. Align rgw service id pattern with cephadm: ceph#39877 - Update rgw pattern to allow service id for non-multisite config. - Extract realm and zone from service id (when detected) and add them to the service spec. Fixes: https://tracker.ceph.com/issues/44605 Signed-off-by: Alfonso Martínez <almartin@redhat.com>

…ce spec. Align rgw service id pattern with cephadm: ceph#39877 - Update rgw pattern to allow service id for non-multisite config. - Extract realm and zone from service id (when detected) and add them to the service spec. Fixes: https://tracker.ceph.com/issues/44605 Signed-off-by: Alfonso Martínez <almartin@redhat.com> (cherry picked from commit 0575844)

liewegas requested a review from a team as a code owner March 5, 2021 21:01

github-actions bot added cephadm orchestrator pybind labels Mar 5, 2021

liewegas requested review from cbodley and ofriedma March 5, 2021 21:02

liewegas mentioned this pull request Mar 5, 2021

mgr/cephadm: timeout radosgw-admin (instead of requiring HEALTH_OK) #39865

Closed

3 tasks

liewegas added the wip-sage-testing label Mar 5, 2021

github-actions bot added core documentation mgr tests labels Mar 5, 2021

liewegas force-pushed the cephadm-rgw-simplification branch from 2580261 to 472419b Compare March 6, 2021 14:05

ofriedma requested changes Mar 7, 2021

View reviewed changes

sebastian-philipp suggested changes Mar 8, 2021

View reviewed changes

liewegas force-pushed the cephadm-rgw-simplification branch from 472419b to faef3a4 Compare March 8, 2021 14:27

Daniel-Pivonka mentioned this pull request Mar 8, 2021

mgr/cephadm: more rgw streamlining #37203

Closed

liewegas requested a review from sebastian-philipp March 8, 2021 19:43

liewegas force-pushed the cephadm-rgw-simplification branch from 3560fda to 80560e6 Compare March 8, 2021 19:48

jmolmo reviewed Mar 9, 2021

View reviewed changes

jmolmo approved these changes Mar 9, 2021

View reviewed changes

liewegas added 2 commits March 9, 2021 11:01

python-common/ceph/deployment: RGWSpec: accept (and drop) subcluster arg

d35e1d7

This is just so we can load up a stored spec after upgrade. We'll silently drop it, since we have the service_id, and this was only used to generate that anyway. Signed-off-by: Sage Weil <sage@newdream.net>

github-actions bot added the needs-rebase label Mar 9, 2021

liewegas added wip-sage-testing and removed wip-sage-testing labels Mar 9, 2021

liewegas added 4 commits March 9, 2021 14:29

doc: update docs

50bc491

Signed-off-by: Sage Weil <sage@newdream.net>

qa/tasks/cephadm: drop realm.zone convention for rgw

1bfb0cd

Note that cephadm.py will no longer do anything with rgw realms and zones. That means that the setup of rgw roles here is only useful for the default zone and a non-multisite config. Signed-off-by: Sage Weil <sage@newdream.net>

PendingReleaseNodes: note changes in cephadm rgw behavior

f4ff709

Signed-off-by: Sage Weil <sage@newdream.net>

liewegas force-pushed the cephadm-rgw-simplification branch from 3560fda to cca7391 Compare March 9, 2021 19:29

github-actions bot removed the needs-rebase label Mar 9, 2021

ofriedma self-requested a review March 10, 2021 11:36

ofriedma approved these changes Mar 10, 2021

View reviewed changes

sebastian-philipp approved these changes Mar 10, 2021

View reviewed changes

sebastian-philipp merged commit b6a2dfc into ceph:master Mar 10, 2021

liewegas mentioned this pull request Mar 15, 2021

pacific: cephadm: Batch backport March (2) #40135

Merged

Conversation

liewegas commented Mar 5, 2021

Checklist

Uh oh!

ofriedma left a comment

Choose a reason for hiding this comment

Uh oh!

liewegas commented Mar 7, 2021

Uh oh!

liewegas commented Mar 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmolmo Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

jmolmo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epuertat commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liewegas commented Mar 9, 2021

Uh oh!

sebastian-philipp commented Mar 9, 2021

Uh oh!

liewegas commented Mar 9, 2021

Uh oh!

liewegas commented Mar 9, 2021

Uh oh!

sebastian-philipp commented Mar 9, 2021

Uh oh!

github-actions bot commented Mar 9, 2021

Uh oh!

sebastian-philipp commented Mar 9, 2021

Uh oh!

ofriedma commented Mar 10, 2021

Uh oh!

sebastian-philipp commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebastian-philipp commented Mar 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmolmo left a comment •

edited

Loading

epuertat commented Mar 9, 2021 •

edited

Loading

sebastian-philipp commented Mar 10, 2021 •

edited

Loading