From 6c15209e9a477652be66ab89fa14b4b8d9ec8c66 Mon Sep 17 00:00:00 2001 From: Nizamudeen A Date: Mon, 24 Mar 2025 17:09:04 +0530 Subject: [PATCH] mgr/dashboard: upgrading nvmeof doesn't update configuration Happens from 19.2.0 to any of the latest upgrade During upgrade I get ``` Failed to set Dashboard config for nvmeof: dashboard nvmeof-gateway-add failed: JSON array/object not allowed {"prefix": "dashboard nvmeof-gateway-add", "name": "nvmeof.rbd", "group": null, "daemon_name": "nvmeof.rbd.ceph-node-01.irpssg"} retval: -22 ``` which is fixed by handling the group_name when its not there in spec. And the other error was ``` Failed to set Dashboard config for nvmeof: dashboard nvmeof-gateway-add failed: Traceback (most recent call last): File "/usr/share/ceph/mgr/mgr_module.py", line 1864, in _handle_command return CLICommand.COMMANDS[cmd['prefix']].call(self, cmd, inbuf) File "/usr/share/ceph/mgr/mgr_module.py", line 499, in call return self.func(mgr, **kwargs) File "/usr/share/ceph/mgr/mgr_module.py", line 535, in check return func(*args, **kwargs) File "/usr/share/ceph/mgr/dashboard/services/nvmeof_cli.py", line 28, in add_nvmeof_gateway NvmeofGatewaysConfig.add_gateway(name, service_url, group, daemon_name) File "/usr/share/ceph/mgr/dashboard/services/nvmeof_conf.py", line 61, in add_gateway gateway['daemon_name'] = daemon_name TypeError: 'str' object does not support item assignment retval: -22 ``` which is fixed by properly updating the config to the newer format that is available in newer version Fixes: https://tracker.ceph.com/issues/70629 Signed-off-by: Nizamudeen A (cherry picked from commit 6e9ac798e354aa37b41708aae099095baec0061d) Conflicts: src/pybind/mgr/dashboard/tests/test_nvmeof_cli.py - kept test file and only added the relavant test files --- src/pybind/mgr/cephadm/services/nvmeof.py | 2 +- .../mgr/dashboard/services/nvmeof_conf.py | 20 +- src/pybind/mgr/dashboard/tests/__init__.py | 4 + .../mgr/dashboard/tests/test_nvmeof_cli.py | 177 ++++++++++++++++++ 4 files changed, 195 insertions(+), 8 deletions(-) create mode 100644 src/pybind/mgr/dashboard/tests/test_nvmeof_cli.py diff --git a/src/pybind/mgr/cephadm/services/nvmeof.py b/src/pybind/mgr/cephadm/services/nvmeof.py index 78789515a4262..f3eb623d61a2d 100644 --- a/src/pybind/mgr/cephadm/services/nvmeof.py +++ b/src/pybind/mgr/cephadm/services/nvmeof.py @@ -105,7 +105,7 @@ class NvmeofService(CephService): 'prefix': 'dashboard nvmeof-gateway-add', 'inbuf': service_url, 'name': service_name, - 'group': spec.group, + 'group': spec.group if spec.group else '', 'daemon_name': dd.name() }) return cmd_dicts diff --git a/src/pybind/mgr/dashboard/services/nvmeof_conf.py b/src/pybind/mgr/dashboard/services/nvmeof_conf.py index 170f98c70d18f..6b89c586e99f3 100644 --- a/src/pybind/mgr/dashboard/services/nvmeof_conf.py +++ b/src/pybind/mgr/dashboard/services/nvmeof_conf.py @@ -55,13 +55,19 @@ class NvmeofGatewaysConfig(object): config = cls.get_gateways_config() if name in config.get('gateways', {}): - existing_gateways = config['gateways'][name] - for gateway in existing_gateways: - if 'daemon_name' not in gateway: - gateway['daemon_name'] = daemon_name - break - if gateway['service_url'] == service_url: - return + # the nvmeof dashboard config used in v19.2.0 saves the below + # to a dict. Converting that to a list so that the upgrade + # properly migrate it to the newer format, and also keep it empty. + if isinstance(config['gateways'][name], dict): + config['gateways'][name] = [] + else: + existing_gateways = config['gateways'][name] + for gateway in existing_gateways: + if 'daemon_name' not in gateway: + gateway['daemon_name'] = daemon_name + break + if gateway['service_url'] == service_url: + return new_gateway = { 'service_url': service_url, diff --git a/src/pybind/mgr/dashboard/tests/__init__.py b/src/pybind/mgr/dashboard/tests/__init__.py index 3061fe9dc9316..41dc300e9bcdc 100644 --- a/src/pybind/mgr/dashboard/tests/__init__.py +++ b/src/pybind/mgr/dashboard/tests/__init__.py @@ -73,6 +73,10 @@ class KVStoreMockMixin(object): def get_key(cls, key): return cls.CONFIG_KEY_DICT.get(key, None) + @classmethod + def set_key(cls, key, value): + cls.CONFIG_KEY_DICT[key] = value + # pylint: disable=protected-access class CLICommandTestMixin(KVStoreMockMixin): diff --git a/src/pybind/mgr/dashboard/tests/test_nvmeof_cli.py b/src/pybind/mgr/dashboard/tests/test_nvmeof_cli.py new file mode 100644 index 0000000000000..901bdbdced12c --- /dev/null +++ b/src/pybind/mgr/dashboard/tests/test_nvmeof_cli.py @@ -0,0 +1,177 @@ +import json +import unittest + +from ..tests import CLICommandTestMixin + + +class TestNVMeoFConfCLI(unittest.TestCase, CLICommandTestMixin): + def setUp(self): + self.mock_kv_store() + + def test_cli_add_gateway(self): + + self.exec_cmd( + 'nvmeof-gateway-add', + name='nvmeof.pool.group', + inbuf='http://nvmf:port', + daemon_name='nvmeof_daemon', + group='group' + ) + + config = json.loads(self.get_key('_nvmeof_config')) + self.assertEqual( + config['gateways'], { + 'nvmeof.pool.group': [{ + 'group': 'group', + 'daemon_name': 'nvmeof_daemon', + 'service_url': 'http://nvmf:port' + }] + } + ) + + def test_cli_migration_from_legacy_config(self): + legacy_config = json.dumps({ + 'gateways': { + 'nvmeof.pool': { + 'service_url': 'http://nvmf:port' + } + } + }) + self.set_key('_nvmeof_config', legacy_config) + + self.exec_cmd( + 'nvmeof-gateway-add', + name='nvmeof.pool', + inbuf='http://nvmf:port', + daemon_name='nvmeof_daemon', + group='' + ) + + config = json.loads(self.get_key('_nvmeof_config')) + self.assertEqual( + config['gateways'], { + 'nvmeof.pool': [{ + 'daemon_name': 'nvmeof_daemon', + 'group': '', + 'service_url': 'http://nvmf:port' + }] + } + ) + + def test_cli_add_gw_to_existing(self): + # add first gw + self.exec_cmd( + 'nvmeof-gateway-add', + name='nvmeof.pool', + inbuf='http://nvmf:port', + daemon_name='nvmeof_daemon', + group='' + ) + + # add another daemon to the first gateway + self.exec_cmd( + 'nvmeof-gateway-add', + name='nvmeof.pool', + inbuf='http://nvmf-2:port', + daemon_name='nvmeof_daemon_2', + group='' + ) + + config = json.loads(self.get_key('_nvmeof_config')) + + # make sure its appended to the existing gateway + self.assertEqual( + config['gateways'], { + 'nvmeof.pool': [{ + 'daemon_name': 'nvmeof_daemon', + 'group': '', + 'service_url': 'http://nvmf:port' + }, { + 'daemon_name': 'nvmeof_daemon_2', + 'group': '', + 'service_url': 'http://nvmf-2:port' + }] + } + ) + + def test_cli_add_new_gw(self): + # add first config + self.exec_cmd( + 'nvmeof-gateway-add', + name='nvmeof.pool', + inbuf='http://nvmf:port', + daemon_name='nvmeof_daemon', + group='' + ) + + # add another gateway + self.exec_cmd( + 'nvmeof-gateway-add', + name='nvmeof2.pool.group', + inbuf='http://nvmf-2:port', + daemon_name='nvmeof_daemon_2', + group='group' + ) + + config = json.loads(self.get_key('_nvmeof_config')) + + # make sure its added as a new entry + self.assertEqual( + config['gateways'], { + 'nvmeof.pool': [{ + 'daemon_name': 'nvmeof_daemon', + 'group': '', + 'service_url': 'http://nvmf:port' + }], + 'nvmeof2.pool.group': [{ + 'daemon_name': 'nvmeof_daemon_2', + 'group': 'group', + 'service_url': 'http://nvmf-2:port' + }] + } + ) + + def test_cli_rm_gateway(self): + self.test_cli_add_gateway() + self.exec_cmd('nvmeof-gateway-rm', name='nvmeof.pool.group') + + config = json.loads(self.get_key('_nvmeof_config')) + self.assertEqual( + config['gateways'], {} + ) + + def test_cli_rm_daemon_from_gateway(self): + self.test_cli_add_gw_to_existing() + self.exec_cmd( + 'nvmeof-gateway-rm', + name='nvmeof.pool', + daemon_name='nvmeof_daemon' + ) + + config = json.loads(self.get_key('_nvmeof_config')) + self.assertEqual( + config['gateways'], { + 'nvmeof.pool': [{ + 'daemon_name': 'nvmeof_daemon_2', + 'group': '', + 'service_url': 'http://nvmf-2:port' + }] + } + ) + + def test_cli_legacy_config_rm(self): + legacy_config = json.dumps({ + 'gateways': { + 'nvmeof.pool': { + 'service_url': 'http://nvmf:port' + } + } + }) + self.set_key('_nvmeof_config', legacy_config) + + self.exec_cmd('nvmeof-gateway-rm', name='nvmeof.pool') + + config = json.loads(self.get_key('_nvmeof_config')) + self.assertEqual( + config['gateways'], {} + ) -- 2.39.5