Merge pull request #52301 from adk3798/cephadm-fix-rgw-migration

mgr/cephadm: fix rgw spec migration with simple specs Reviewed-by: John Mulligan <jmulligan@redhat.com> Reviewed-by: Redouane Kachach <rkachach@redhat.com>
ceph · Jul 7, 2023 · b985fad · b985fad
2 parents 9a9bcf3 + 1860ef8
commit b985fad
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 32 deletions.
diff --git a/qa/suites/orch/cephadm/upgrade/3-upgrade/simple.yaml b/qa/suites/orch/cephadm/upgrade/3-upgrade/simple.yaml
@@ -8,6 +8,8 @@ tasks:
       - radosgw-admin zone create --rgw-zonegroup=default --rgw-zone=z --master --default
       - radosgw-admin period update --rgw-realm=r --commit
       - ceph orch apply rgw foo --realm r --zone z --placement=2 --port=8000
+      # simple rgw spec (will have no "spec" field) to make sure that works with rgw spec migration
+      - ceph orch apply rgw smpl
       # setup iscsi
       - ceph osd pool create foo
       - rbd pool init foo

diff --git a/src/pybind/mgr/cephadm/migrations.py b/src/pybind/mgr/cephadm/migrations.py
@@ -370,6 +370,11 @@ def migrate_rgw_spec(self, spec: Dict[Any, Any]) -> Optional[RGWSpec]:
         return RGWSpec.from_json(new_spec)
 
     def rgw_spec_needs_migration(self, spec: Dict[Any, Any]) -> bool:
+        if 'spec' not in spec:
+            # if users allowed cephadm to set up most of the
+            # attributes, it's possible there is no "spec" section
+            # inside the spec. In that case, no migration is needed
+            return False
         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']

diff --git a/src/pybind/mgr/cephadm/tests/test_migration.py b/src/pybind/mgr/cephadm/tests/test_migration.py
@@ -1,4 +1,5 @@
 import json
+import pytest
 
 from ceph.deployment.service_spec import PlacementSpec, ServiceSpec, HostPlacementSpec
 from ceph.utils import datetime_to_str, datetime_now
@@ -259,26 +260,43 @@ def test_migrate_set_sane_value(cephadm_module: CephadmOrchestrator):
     assert cephadm_module.migration_current == 0
 
 
+@pytest.mark.parametrize(
+    "rgw_spec_store_entry, should_migrate",
+    [
+        ({
+            '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()),
+        }, True),
+        ({
+            'spec': {
+                'service_type': 'rgw',
+                'service_name': 'rgw.foo',
+                'service_id': 'foo',
+                'placement': {
+                    'hosts': ['host1']
+                },
+            },
+            'created': datetime_to_str(datetime_now()),
+        }, False),
+    ]
+)
 @mock.patch("cephadm.serve.CephadmServe._run_cephadm", _run_cephadm('[]'))
-def test_migrate_rgw_spec(cephadm_module: CephadmOrchestrator):
+def test_migrate_rgw_spec(cephadm_module: CephadmOrchestrator, rgw_spec_store_entry, should_migrate):
     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),
+            json.dumps(rgw_spec_store_entry, sort_keys=True),
         )
 
         # make sure rgw_migration_queue is populated accordingly
@@ -296,19 +314,27 @@ def test_migrate_rgw_spec(cephadm_module: CephadmOrchestrator):
         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',
-                                            }}
+        if should_migrate:
+            # 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',
+                                                }}
+        else:
+            # in a real environment, we still expect the spec to be there,
+            # just untouched by the migration. For this test specifically
+            # though, the spec will only have ended up in the spec store
+            # if it was migrated, so we can use this to test the spec
+            # was untouched
+            assert 'rgw.foo' not in cephadm_module.spec_store.all_specs