From 2c46c0741962e0e6a5ddbc960dfd21948daf0947 Mon Sep 17 00:00:00 2001 From: Redouane Kachach Date: Wed, 26 Oct 2022 11:33:38 +0200 Subject: [PATCH] mgr/cephadm: Adding extra arguments support for RGW frontend Fixes: https://tracker.ceph.com/issues/57931 Signed-off-by: Redouane Kachach --- doc/cephadm/services/rgw.rst | 27 ++++++ .../orch/cephadm/mgr-nfs-upgrade/4-final.yaml | 2 +- src/pybind/mgr/cephadm/inventory.py | 9 +- src/pybind/mgr/cephadm/migrations.py | 87 ++++++++++++++++++- .../mgr/cephadm/services/cephadmservice.py | 6 ++ .../mgr/cephadm/tests/test_migration.py | 55 ++++++++++++ src/pybind/mgr/cephadm/tests/test_services.py | 18 ++-- .../ceph/deployment/service_spec.py | 10 +++ .../ceph/tests/test_service_spec.py | 22 +++++ 9 files changed, 223 insertions(+), 13 deletions(-) diff --git a/doc/cephadm/services/rgw.rst b/doc/cephadm/services/rgw.rst index 2c1c06261f48f..818648cf5fee4 100644 --- a/doc/cephadm/services/rgw.rst +++ b/doc/cephadm/services/rgw.rst @@ -74,6 +74,33 @@ example spec file: spec: rgw_frontend_port: 8080 +Passing Frontend Extra Arguments +-------------------------------- + +The RGW service specification can be used to pass extra arguments to the rgw frontend by using +the `rgw_frontend_extra_args` arguments list. + +example spec file: + +.. code-block:: yaml + + service_type: rgw + service_id: foo + placement: + label: rgw + count_per_host: 2 + spec: + rgw_realm: myrealm + rgw_zone: myzone + rgw_frontend_type: "beast" + rgw_frontend_port: 5000 + rgw_frontend_extra_args: + - "tcp_nodelay=1" + - "max_header_size=65536" + +.. note:: cephadm combines the arguments from the `spec` section and the ones from + the `rgw_frontend_extra_args` into a single space-separated arguments list + which is used to set the value of `rgw_frontends` configuration parameter. Multisite zones --------------- diff --git a/qa/suites/orch/cephadm/mgr-nfs-upgrade/4-final.yaml b/qa/suites/orch/cephadm/mgr-nfs-upgrade/4-final.yaml index 2a834ead4c069..3a91696590902 100644 --- a/qa/suites/orch/cephadm/mgr-nfs-upgrade/4-final.yaml +++ b/qa/suites/orch/cephadm/mgr-nfs-upgrade/4-final.yaml @@ -7,4 +7,4 @@ tasks: - ceph nfs cluster ls | grep foo - ceph nfs export ls foo --detailed - rados -p .nfs --all ls - - - ceph config get mgr mgr/cephadm/migration_current | grep 5 + - ceph config get mgr mgr/cephadm/migration_current | grep 6 diff --git a/src/pybind/mgr/cephadm/inventory.py b/src/pybind/mgr/cephadm/inventory.py index fcbad42b7d929..8d1e720d7dd14 100644 --- a/src/pybind/mgr/cephadm/inventory.py +++ b/src/pybind/mgr/cephadm/inventory.py @@ -18,7 +18,7 @@ from orchestrator import OrchestratorError, HostSpec, OrchestratorEvent, service from cephadm.services.cephadmservice import CephadmDaemonDeploySpec from .utils import resolve_ip -from .migrations import queue_migrate_nfs_spec +from .migrations import queue_migrate_nfs_spec, queue_migrate_rgw_spec if TYPE_CHECKING: from .module import CephadmOrchestrator @@ -247,6 +247,13 @@ class SpecStore(): ): self.mgr.log.debug(f'found legacy nfs spec {j}') queue_migrate_nfs_spec(self.mgr, j) + + if ( + (self.mgr.migration_current or 0) < 6 + and j['spec'].get('service_type') == 'rgw' + ): + queue_migrate_rgw_spec(self.mgr, j) + spec = ServiceSpec.from_json(j['spec']) created = str_to_datetime(cast(str, j['created'])) self._specs[service_name] = spec diff --git a/src/pybind/mgr/cephadm/migrations.py b/src/pybind/mgr/cephadm/migrations.py index a3a35a900e906..2e8587abfc613 100644 --- a/src/pybind/mgr/cephadm/migrations.py +++ b/src/pybind/mgr/cephadm/migrations.py @@ -1,8 +1,9 @@ import json +import re import logging -from typing import TYPE_CHECKING, Iterator, Optional, Dict, Any +from typing import TYPE_CHECKING, Iterator, Optional, Dict, Any, List -from ceph.deployment.service_spec import PlacementSpec, ServiceSpec, HostPlacementSpec +from ceph.deployment.service_spec import PlacementSpec, ServiceSpec, HostPlacementSpec, RGWSpec from cephadm.schedule import HostAssignment import rados @@ -12,7 +13,7 @@ from orchestrator import OrchestratorError, DaemonDescription if TYPE_CHECKING: from .module import CephadmOrchestrator -LAST_MIGRATION = 5 +LAST_MIGRATION = 6 logger = logging.getLogger(__name__) @@ -36,6 +37,9 @@ class Migrations: v = mgr.get_store('nfs_migration_queue') self.nfs_migration_queue = json.loads(v) if v else [] + r = mgr.get_store('rgw_migration_queue') + self.rgw_migration_queue = json.loads(r) if r else [] + # for some migrations, we don't need to do anything except for # incrementing migration_current. # let's try to shortcut things here. @@ -96,6 +100,10 @@ class Migrations: if self.migrate_4_5(): self.set(5) + if self.mgr.migration_current == 5: + if self.migrate_5_6(): + self.set(6) + def migrate_0_1(self) -> bool: """ Migration 0 -> 1 @@ -336,6 +344,79 @@ class Migrations: self.mgr.log.info('Done migrating registry login info') return True + def migrate_rgw_spec(self, spec: Dict[Any, Any]) -> Optional[RGWSpec]: + """ Migrate an old rgw spec to the new format.""" + new_spec = spec.copy() + field_content: List[str] = re.split(' +', new_spec['spec']['rgw_frontend_type']) + valid_spec = False + if 'beast' in field_content: + new_spec['spec']['rgw_frontend_type'] = 'beast' + field_content.remove('beast') + valid_spec = True + elif 'civetweb' in field_content: + new_spec['spec']['rgw_frontend_type'] = 'civetweb' + field_content.remove('civetweb') + valid_spec = True + else: + # Error: Should not happen as that would be an invalid RGW spec. In that case + # we keep the spec as it, mark it as unmanaged to avoid the daemons being deleted + # and raise a health warning so the user can fix the issue manually later. + self.mgr.log.error("Cannot migrate RGW spec, bad rgw_frontend_type value: {spec['spec']['rgw_frontend_type']}.") + + if valid_spec: + new_spec['spec']['rgw_frontend_extra_args'] = [] + new_spec['spec']['rgw_frontend_extra_args'].extend(field_content) + + return RGWSpec.from_json(new_spec) + + def rgw_spec_needs_migration(self, spec: Dict[Any, Any]) -> bool: + return 'rgw_frontend_type' in spec['spec'] \ + and spec['spec']['rgw_frontend_type'] is not None \ + and spec['spec']['rgw_frontend_type'].strip() not in ['beast', 'civetweb'] + + def migrate_5_6(self) -> bool: + """ + Migration 5 -> 6 + + Old RGW spec used to allow 'bad' values on the rgw_frontend_type field. For example + the following value used to be valid: + + rgw_frontend_type: "beast endpoint=10.16.96.54:8043 tcp_nodelay=1" + + As of 17.2.6 release, these kind of entries are not valid anymore and a more strict check + has been added to validate this field. + + This migration logic detects this 'bad' values and tries to transform them to the new + valid format where rgw_frontend_type field can only be either 'beast' or 'civetweb'. + Any extra arguments detected on rgw_frontend_type field will be parsed and passed in the + new spec field rgw_frontend_extra_args. + """ + self.mgr.log.debug(f'Starting rgw migration (queue length is {len(self.rgw_migration_queue)})') + for s in self.rgw_migration_queue: + spec = s['spec'] + if self.rgw_spec_needs_migration(spec): + rgw_spec = self.migrate_rgw_spec(spec) + if rgw_spec is not None: + logger.info(f"Migrating {spec} to new RGW with extra args format {rgw_spec}") + self.mgr.spec_store.save(rgw_spec) + else: + logger.info(f"No Migration is needed for rgw spec: {spec}") + self.rgw_migration_queue = [] + return True + + +def queue_migrate_rgw_spec(mgr: "CephadmOrchestrator", spec_dict: Dict[Any, Any]) -> None: + """ + As aprt of 17.2.6 a stricter RGW spec validation has been added so the field + rgw_frontend_type cannot be used to pass rgw-frontends parameters. + """ + service_id = spec_dict['spec']['service_id'] + queued = mgr.get_store('rgw_migration_queue') or '[]' + ls = json.loads(queued) + ls.append(spec_dict) + mgr.set_store('rgw_migration_queue', json.dumps(ls)) + mgr.log.info(f'Queued rgw.{service_id} for migration') + def queue_migrate_nfs_spec(mgr: "CephadmOrchestrator", spec_dict: Dict[Any, Any]) -> None: """ diff --git a/src/pybind/mgr/cephadm/services/cephadmservice.py b/src/pybind/mgr/cephadm/services/cephadmservice.py index d5437c109332b..f8a0c52ffd3d3 100644 --- a/src/pybind/mgr/cephadm/services/cephadmservice.py +++ b/src/pybind/mgr/cephadm/services/cephadmservice.py @@ -916,6 +916,12 @@ class RgwService(CephService): args.append(f"port={build_url(host=daemon_spec.ip, port=port).lstrip('/')}") else: args.append(f"port={port}") + else: + raise OrchestratorError(f'Invalid rgw_frontend_type parameter: {ftype}. Valid values are: beast, civetweb.') + + if spec.rgw_frontend_extra_args is not None: + args.extend(spec.rgw_frontend_extra_args) + frontend = f'{ftype} {" ".join(args)}' ret, out, err = self.mgr.check_mon_command({ diff --git a/src/pybind/mgr/cephadm/tests/test_migration.py b/src/pybind/mgr/cephadm/tests/test_migration.py index 93481553ab5fd..ed4c8fc42fa95 100644 --- a/src/pybind/mgr/cephadm/tests/test_migration.py +++ b/src/pybind/mgr/cephadm/tests/test_migration.py @@ -257,3 +257,58 @@ def test_migrate_set_sane_value(cephadm_module: CephadmOrchestrator): ongoing = cephadm_module.migration.is_migration_ongoing() assert ongoing assert cephadm_module.migration_current == 0 + + +@mock.patch("cephadm.serve.CephadmServe._run_cephadm", _run_cephadm('[]')) +def test_migrate_rgw_spec(cephadm_module: CephadmOrchestrator): + with with_host(cephadm_module, 'host1'): + cephadm_module.set_store( + SPEC_STORE_PREFIX + 'rgw', + json.dumps({ + 'spec': { + 'service_type': 'rgw', + 'service_name': 'rgw.foo', + 'service_id': 'foo', + 'placement': { + 'hosts': ['host1'] + }, + 'spec': { + 'rgw_frontend_type': 'beast tcp_nodelay=1 request_timeout_ms=65000 rgw_thread_pool_size=512', + 'rgw_frontend_port': '5000', + }, + }, + 'created': datetime_to_str(datetime_now()), + }, sort_keys=True), + ) + + # make sure rgw_migration_queue is populated accordingly + cephadm_module.migration_current = 1 + cephadm_module.spec_store.load() + ls = json.loads(cephadm_module.get_store('rgw_migration_queue')) + assert 'rgw' == ls[0]['spec']['service_type'] + + # shortcut rgw_migration_queue loading by directly assigning + # ls output to rgw_migration_queue list + cephadm_module.migration.rgw_migration_queue = ls + + # skip other migrations and go directly to 5_6 migration (RGW spec) + cephadm_module.migration_current = 5 + cephadm_module.migration.migrate() + assert cephadm_module.migration_current == LAST_MIGRATION + + # make sure the spec has been migrated and the the param=value entries + # that were part of the rgw_frontend_type are now in the new + # 'rgw_frontend_extra_args' list + assert 'rgw.foo' in cephadm_module.spec_store.all_specs + rgw_spec = cephadm_module.spec_store.all_specs['rgw.foo'] + assert dict(rgw_spec.to_json()) == {'service_type': 'rgw', + 'service_id': 'foo', + 'service_name': 'rgw.foo', + 'placement': {'hosts': ['host1']}, + 'spec': { + 'rgw_frontend_extra_args': ['tcp_nodelay=1', + 'request_timeout_ms=65000', + 'rgw_thread_pool_size=512'], + 'rgw_frontend_port': '5000', + 'rgw_frontend_type': 'beast', + }} diff --git a/src/pybind/mgr/cephadm/tests/test_services.py b/src/pybind/mgr/cephadm/tests/test_services.py index 8e37836c77b0b..e440bc8d11fa6 100644 --- a/src/pybind/mgr/cephadm/tests/test_services.py +++ b/src/pybind/mgr/cephadm/tests/test_services.py @@ -944,18 +944,19 @@ spec: class TestRGWService: @pytest.mark.parametrize( - "frontend, ssl, expected", + "frontend, ssl, extra_args, expected", [ - ('beast', False, 'beast endpoint=[fd00:fd00:fd00:3000::1]:80'), - ('beast', True, - 'beast ssl_endpoint=[fd00:fd00:fd00:3000::1]:443 ssl_certificate=config://rgw/cert/rgw.foo'), - ('civetweb', False, 'civetweb port=[fd00:fd00:fd00:3000::1]:80'), - ('civetweb', True, + ('beast', False, ['tcp_nodelay=1'], + 'beast endpoint=[fd00:fd00:fd00:3000::1]:80 tcp_nodelay=1'), + ('beast', True, ['tcp_nodelay=0', 'max_header_size=65536'], + 'beast ssl_endpoint=[fd00:fd00:fd00:3000::1]:443 ssl_certificate=config://rgw/cert/rgw.foo tcp_nodelay=0 max_header_size=65536'), + ('civetweb', False, [], 'civetweb port=[fd00:fd00:fd00:3000::1]:80'), + ('civetweb', True, None, 'civetweb port=[fd00:fd00:fd00:3000::1]:443s ssl_certificate=config://rgw/cert/rgw.foo'), ] ) @patch("cephadm.serve.CephadmServe._run_cephadm", _run_cephadm('{}')) - def test_rgw_update(self, frontend, ssl, expected, cephadm_module: CephadmOrchestrator): + def test_rgw_update(self, frontend, ssl, extra_args, expected, cephadm_module: CephadmOrchestrator): with with_host(cephadm_module, 'host1'): cephadm_module.cache.update_host_networks('host1', { 'fd00:fd00:fd00:3000::/64': { @@ -965,7 +966,8 @@ class TestRGWService: s = RGWSpec(service_id="foo", networks=['fd00:fd00:fd00:3000::/64'], ssl=ssl, - rgw_frontend_type=frontend) + rgw_frontend_type=frontend, + rgw_frontend_extra_args=extra_args) with with_service(cephadm_module, s) as dds: _, f, _ = cephadm_module.check_mon_command({ 'prefix': 'config get', diff --git a/src/python-common/ceph/deployment/service_spec.py b/src/python-common/ceph/deployment/service_spec.py index 393100b45a791..a94704d822820 100644 --- a/src/python-common/ceph/deployment/service_spec.py +++ b/src/python-common/ceph/deployment/service_spec.py @@ -872,6 +872,7 @@ class RGWSpec(ServiceSpec): rgw_frontend_port: Optional[int] = None, rgw_frontend_ssl_certificate: Optional[List[str]] = None, rgw_frontend_type: Optional[str] = None, + rgw_frontend_extra_args: Optional[List[str]] = None, unmanaged: bool = False, ssl: bool = False, preview_only: bool = False, @@ -916,6 +917,8 @@ class RGWSpec(ServiceSpec): self.rgw_frontend_ssl_certificate: Optional[List[str]] = rgw_frontend_ssl_certificate #: civetweb or beast (default: beast). See :ref:`rgw_frontends` self.rgw_frontend_type: Optional[str] = rgw_frontend_type + #: List of extra arguments for rgw_frontend in the form opt=value. See :ref:`rgw_frontends` + self.rgw_frontend_extra_args: Optional[List[str]] = rgw_frontend_extra_args #: enable SSL self.ssl = ssl self.rgw_realm_token = rgw_realm_token @@ -942,6 +945,13 @@ class RGWSpec(ServiceSpec): if self.rgw_zone and not self.rgw_realm: raise SpecValidationError('Cannot add RGW: Zone specified but no realm specified') + if self.rgw_frontend_type is not None: + if self.rgw_frontend_type not in ['beast', 'civetweb']: + raise SpecValidationError( + 'Invalid rgw_frontend_type value. Valid values are: beast, civetweb.\n' + 'Additional rgw type parameters can be passed using rgw_frontend_extra_args.' + ) + yaml.add_representer(RGWSpec, ServiceSpec.yaml_representer) diff --git a/src/python-common/ceph/tests/test_service_spec.py b/src/python-common/ceph/tests/test_service_spec.py index 6d3ae905ef3f9..041cbbbd4ec79 100644 --- a/src/python-common/ceph/tests/test_service_spec.py +++ b/src/python-common/ceph/tests/test_service_spec.py @@ -255,6 +255,28 @@ def test_servicespec_map_test(s_type, o_spec, s_id): assert spec.validate() is None ServiceSpec.from_json(spec.to_json()) + +@pytest.mark.parametrize( + "realm, zone, frontend_type, raise_exception, msg", + [ + ('realm', 'zone1', 'beast', False, ''), + ('realm', 'zone2', 'civetweb', False, ''), + ('realm', None, 'beast', True, 'Cannot add RGW: Realm specified but no zone specified'), + (None, 'zone1', 'beast', True, 'Cannot add RGW: Zone specified but no realm specified'), + ('realm', 'zone', 'invalid-beast', True, '^Invalid rgw_frontend_type value'), + ('realm', 'zone', 'invalid-civetweb', True, '^Invalid rgw_frontend_type value'), + ]) +def test_rgw_servicespec_parse(realm, zone, frontend_type, raise_exception, msg): + spec = RGWSpec(service_id='foo', + rgw_realm=realm, + rgw_zone=zone, + rgw_frontend_type=frontend_type) + if raise_exception: + with pytest.raises(SpecValidationError, match=msg): + spec.validate() + else: + spec.validate() + def test_osd_unmanaged(): osd_spec = {"placement": {"host_pattern": "*"}, "service_id": "all-available-devices", -- 2.39.5