From df0028f0fe62af004c04a113d5da1bfea7a403b3 Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Mon, 2 Mar 2026 08:50:05 -0500 Subject: [PATCH] Revert "Merge pull request #67167 from Hezko/nvme-module-two" This PR broke the ceph-mgr [1] and was not QA'd more broadly. [1] https://github.com/ceph/ceph/pull/67167#issuecomment-3984274659 This reverts commit 884bbcfd38d88b08213c3106dc89cd6c956d4b95, reversing changes made to 472e267f03705145ee306d15f3d566ec4ac3f726. Signed-off-by: Patrick Donnelly --- PendingReleaseNotes | 4 - ceph.spec.in | 1 - debian/ceph-mgr-modules-core.install | 1 - src/common/options/mgr.yaml.in | 2 +- src/pybind/mgr/CMakeLists.txt | 1 - src/pybind/mgr/nvmeof/__init__.py | 2 - src/pybind/mgr/nvmeof/module.py | 57 ------------- src/pybind/mgr/nvmeof/tests/__init__.py | 0 .../mgr/nvmeof/tests/test_nvmeof_module.py | 85 ------------------- src/pybind/mgr/orchestrator/module.py | 26 +----- .../orchestrator/tests/test_orchestrator.py | 55 ------------ src/pybind/mgr/tox.ini | 2 - 12 files changed, 4 insertions(+), 232 deletions(-) delete mode 100644 src/pybind/mgr/nvmeof/__init__.py delete mode 100644 src/pybind/mgr/nvmeof/module.py delete mode 100644 src/pybind/mgr/nvmeof/tests/__init__.py delete mode 100644 src/pybind/mgr/nvmeof/tests/test_nvmeof_module.py diff --git a/PendingReleaseNotes b/PendingReleaseNotes index 5c37cd7001f..e14ae6e48bd 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -263,10 +263,6 @@ to ensure compatibility during upgrades, but can be disabled once old usage logs are no longer present to avoid performance overhead. -* NVMe-oF: A new Ceph Manager module, `nvmeof`, is now available. When enabled, it provisions the - dedicated `.nvmeof` RADOS Block Device (RBD) pool required for NVMe-oF integration, - simplifying initial setup and ensuring the pool is created consistently across deployments. - * MGR: A new command, `ceph osd ok-to-upgrade`, has been added that allows users and orchestration tools to determine a safe set of OSDs within a CRUSH bucket to upgrade simultaneously without impacting data availability. To help diff --git a/ceph.spec.in b/ceph.spec.in index f0f6a50c56d..18cd57b2683 100644 --- a/ceph.spec.in +++ b/ceph.spec.in @@ -2015,7 +2015,6 @@ fi %{_datadir}/ceph/mgr/telemetry %{_datadir}/ceph/mgr/test_orchestrator %{_datadir}/ceph/mgr/volumes -%{_datadir}/ceph/mgr/nvmeof %files mgr-rook %{_datadir}/ceph/mgr/rook diff --git a/debian/ceph-mgr-modules-core.install b/debian/ceph-mgr-modules-core.install index 17ed984b02f..90359a8e3e7 100644 --- a/debian/ceph-mgr-modules-core.install +++ b/debian/ceph-mgr-modules-core.install @@ -25,4 +25,3 @@ usr/share/ceph/mgr/telegraf usr/share/ceph/mgr/telemetry usr/share/ceph/mgr/test_orchestrator usr/share/ceph/mgr/volumes -usr/share/ceph/mgr/nvmeof diff --git a/src/common/options/mgr.yaml.in b/src/common/options/mgr.yaml.in index 7ee2d3c8b6e..658a0160987 100644 --- a/src/common/options/mgr.yaml.in +++ b/src/common/options/mgr.yaml.in @@ -153,7 +153,7 @@ options: first started after installation, to populate the list of enabled manager modules. Subsequent updates are done using the 'mgr module [enable|disable]' commands. List may be comma or space separated. - default: iostat nfs nvmeof + default: iostat nfs services: - mon - common diff --git a/src/pybind/mgr/CMakeLists.txt b/src/pybind/mgr/CMakeLists.txt index d4032eb1dd7..9e900f859d7 100644 --- a/src/pybind/mgr/CMakeLists.txt +++ b/src/pybind/mgr/CMakeLists.txt @@ -27,7 +27,6 @@ set(mgr_modules devicehealth diskprediction_local # hello is an example for developers, not for user - nvmeof influx insights iostat diff --git a/src/pybind/mgr/nvmeof/__init__.py b/src/pybind/mgr/nvmeof/__init__.py deleted file mode 100644 index 33b8e63a31c..00000000000 --- a/src/pybind/mgr/nvmeof/__init__.py +++ /dev/null @@ -1,2 +0,0 @@ -# flake8: noqa -from .module import NVMeoF diff --git a/src/pybind/mgr/nvmeof/module.py b/src/pybind/mgr/nvmeof/module.py deleted file mode 100644 index df780f0bdc8..00000000000 --- a/src/pybind/mgr/nvmeof/module.py +++ /dev/null @@ -1,57 +0,0 @@ -import logging -from typing import Any - -from mgr_module import MgrModule -import rbd - -logger = logging.getLogger(__name__) - -POOL_NAME = ".nvmeof" - - -class NVMeoF(MgrModule): - def __init__(self, *args: Any, **kwargs: Any) -> None: - super(NVMeoF, self).__init__(*args, **kwargs) - - def _pool_exists(self, pool_name: str) -> bool: - logger.info(f"checking if pool {pool_name} exists") - pool_exists = self.pool_exists(pool_name) - if pool_exists: - logger.info(f"pool {pool_name} already exists") - else: - logger.info(f"pool {pool_name} doesn't exist") - return pool_exists - - def _create_pool(self, pool_name: str) -> None: - try: - self.create_pool(pool_name) - logger.info(f"Pool '{pool_name}' created.") - except Exception: - logger.error(f"Error creating pool '{pool_name}", exc_info=True) - raise - - def _enable_rbd_application(self, pool_name: str) -> None: - try: - self.appify_pool(pool_name, 'rbd') - logger.info(f"'rbd' application enabled on pool '{pool_name}'.") - except Exception: - logger.error( - f"Failed to enable 'rbd' application on '{pool_name}'", - exc_info=True - ) - raise - - def _rbd_pool_init(self, pool_name: str) -> None: - try: - with self.rados.open_ioctx(pool_name) as ioctx: - rbd.RBD().pool_init(ioctx, False) - logger.info(f"RBD pool_init completed on '{pool_name}'.") - except Exception: - logger.error(f"Failed to initialize RBD pool '{pool_name}'", exc_info=True) - raise - - def create_pool_if_not_exists(self) -> None: - if not self._pool_exists(POOL_NAME): - self._create_pool(POOL_NAME) - self._enable_rbd_application(POOL_NAME) - self._rbd_pool_init(POOL_NAME) diff --git a/src/pybind/mgr/nvmeof/tests/__init__.py b/src/pybind/mgr/nvmeof/tests/__init__.py deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/src/pybind/mgr/nvmeof/tests/test_nvmeof_module.py b/src/pybind/mgr/nvmeof/tests/test_nvmeof_module.py deleted file mode 100644 index 44741a38129..00000000000 --- a/src/pybind/mgr/nvmeof/tests/test_nvmeof_module.py +++ /dev/null @@ -1,85 +0,0 @@ - -from contextlib import contextmanager -from unittest.mock import MagicMock - -import nvmeof.module as nvmeof_mod - - -class FakeRados: - def __init__(self, exists: bool): - self._exists = exists - self.opened_pools = [] - - def pool_exists(self, pool_name: str) -> bool: - return self._exists - - @contextmanager - def open_ioctx(self, pool_name: str): - self.opened_pools.append(pool_name) - yield object() - - -def patch_rbd_pool_init(monkeypatch): - rbd_instance = MagicMock() - monkeypatch.setattr(nvmeof_mod.rbd, "RBD", lambda: rbd_instance) - return rbd_instance - - -def make_mgr(mon_handler, exists: bool, monkeypatch): - mgr = nvmeof_mod.NVMeoF.__new__(nvmeof_mod.NVMeoF) - mgr.mon_command = mon_handler - mgr._print_log = lambda *args, **kwargs: None - mgr.run = False - - mgr._fake_rados = FakeRados(exists) - mgr.remote = MagicMock() - - def _pool_exists(self, pool_name: str) -> bool: - return self._fake_rados.pool_exists(pool_name) - - def _rbd_pool_init(self, pool_name: str): - with self._fake_rados.open_ioctx(pool_name) as ioctx: - nvmeof_mod.rbd.RBD().pool_init(ioctx, False) - - monkeypatch.setattr(nvmeof_mod.NVMeoF, "_pool_exists", _pool_exists, raising=True) - monkeypatch.setattr(nvmeof_mod.NVMeoF, "_rbd_pool_init", _rbd_pool_init, raising=True) - - return mgr - - -def test_pool_exists_skips_create_calls_enable_and_pool_init(monkeypatch): - calls = [] - - def mon_command(cmd, inbuf): - calls.append(cmd) - return 0, "", "" - - rbd_instance = patch_rbd_pool_init(monkeypatch) - mgr = make_mgr(mon_command, exists=True, monkeypatch=monkeypatch) - - mgr.create_pool_if_not_exists() - - assert not any(c.get("prefix") == "osd pool create" for c in calls) - assert any(c.get("prefix") == "osd pool application enable" for c in calls) - - assert mgr._fake_rados.opened_pools == [".nvmeof"] - rbd_instance.pool_init.assert_called_once() - - -def test_pool_missing_creates_then_enables_then_pool_init(monkeypatch): - calls = [] - - def mon_command(cmd, inbuf): - calls.append(cmd) - return 0, "", "" - - rbd_instance = patch_rbd_pool_init(monkeypatch) - mgr = make_mgr(mon_command, exists=False, monkeypatch=monkeypatch) - - mgr.create_pool_if_not_exists() - - assert any(c.get("prefix") == "osd pool create" for c in calls) - assert any(c.get("prefix") == "osd pool application enable" for c in calls) - - assert mgr._fake_rados.opened_pools == [".nvmeof"] - rbd_instance.pool_init.assert_called_once() diff --git a/src/pybind/mgr/orchestrator/module.py b/src/pybind/mgr/orchestrator/module.py index 876f1c536d6..3a7d453f799 100644 --- a/src/pybind/mgr/orchestrator/module.py +++ b/src/pybind/mgr/orchestrator/module.py @@ -2117,23 +2117,10 @@ Usage: return self._apply_misc([spec], dry_run, format, no_overwrite) - def _is_module_enabled(self, module: str) -> bool: - mgr_map = self.get('mgr_map') - return ( - module in mgr_map.get('modules', []) - or module in mgr_map.get('always_on_modules', []).get(self.release_name, []) - ) - - def _create_nvmeof_metadata_pool_if_needed(self) -> None: - if not self._is_module_enabled('nvmeof'): - raise OrchestratorError('nvmeof module must be enabled to use .nvmeof pool') - self.remote('nvmeof', 'create_pool_if_not_exists') - @OrchestratorCLICommand.Write('orch apply nvmeof') def _apply_nvmeof(self, - _end_positional_: int = 0, - pool: str = ".nvmeof", - group: str = '', + pool: str, + group: str, placement: Optional[str] = None, unmanaged: bool = False, dry_run: bool = False, @@ -2141,18 +2128,11 @@ Usage: no_overwrite: bool = False, inbuf: Optional[str] = None) -> HandleCommandResult: """Scale an nvmeof service""" - if group == '': - raise OrchestratorValidationError('The --group argument is required') - if inbuf: raise OrchestratorValidationError('unrecognized command -i; -h or --help for usage') - if pool == ".nvmeof": - self._create_nvmeof_metadata_pool_if_needed() - - cleanpool = pool.lstrip('.') spec = NvmeofServiceSpec( - service_id=f'{cleanpool}.{group}' if group else cleanpool, + service_id=f'{pool}.{group}' if group else pool, pool=pool, group=group, placement=PlacementSpec.from_string(placement), diff --git a/src/pybind/mgr/orchestrator/tests/test_orchestrator.py b/src/pybind/mgr/orchestrator/tests/test_orchestrator.py index 39306485083..53c47ea8bfe 100644 --- a/src/pybind/mgr/orchestrator/tests/test_orchestrator.py +++ b/src/pybind/mgr/orchestrator/tests/test_orchestrator.py @@ -293,58 +293,3 @@ def test_preview_table_osd_smoke(): } ] preview_table_osd(data) - - -@mock.patch("orchestrator.module.OrchestratorCli.release_name", new_callable=mock.PropertyMock) -@mock.patch("orchestrator.module.OrchestratorCli._apply_misc") -@mock.patch("orchestrator.module.OrchestratorCli.remote") -@mock.patch("orchestrator.module.OrchestratorCli.get") -class TestApplyNvmeof: - - def setup_method(self): - self.m = OrchestratorCli('orchestrator', 0, 0) - - def test_missing_group_raises_validation_error(self, mock_get, mock_remote, mock_apply_misc, mock_release_name): - res = self.m._apply_nvmeof(pool="mypool", group="") - - assert res.retval != 0 - assert "The --group argument is required" in res.stderr - mock_apply_misc.assert_not_called() - - def test_inbuf_raises_validation_error(self, mock_get, mock_remote, mock_apply_misc, mock_release_name): - res = self.m._apply_nvmeof(pool="mypool", group="mygroup", inbuf="some_yaml_content") - - assert res.retval != 0 - assert "unrecognized command -i; -h or --help for usage" in res.stderr - mock_apply_misc.assert_not_called() - - def test_custom_pool_skips_metadata_pool_creation(self, mock_get, mock_remote, mock_apply_misc, mock_release_name): - mock_apply_misc.return_value = HandleCommandResult(retval=0, stdout="Success") - - res = self.m._apply_nvmeof(pool="custompool", group="mygroup") - - mock_remote.assert_not_called() - mock_apply_misc.assert_called_once() - assert res.retval == 0 - - def test_default_pool_fails_if_module_disabled(self, mock_get, mock_remote, mock_apply_misc, mock_release_name): - mock_release_name.return_value = "squid" - mock_get.return_value = {'modules': [], 'always_on_modules': {}} - - res = self.m._apply_nvmeof(pool=".nvmeof", group="mygroup") - - assert res.retval != 0 - assert "nvmeof module must be enabled to use .nvmeof pool" in res.stderr - mock_remote.assert_not_called() - mock_apply_misc.assert_not_called() - - def test_default_pool_creates_metadata_pool_if_module_enabled(self, mock_get, mock_remote, mock_apply_misc, mock_release_name): - mock_release_name.return_value = "squid" - mock_get.return_value = {'modules': ['nvmeof'], 'always_on_modules': {}} - mock_apply_misc.return_value = HandleCommandResult(retval=0, stdout="Success") - - res = self.m._apply_nvmeof(pool=".nvmeof", group="mygroup") - - mock_remote.assert_called_once_with('nvmeof', 'create_pool_if_not_exists') - mock_apply_misc.assert_called_once() - assert res.retval == 0 diff --git a/src/pybind/mgr/tox.ini b/src/pybind/mgr/tox.ini index 35d7f972d00..c2deb627261 100644 --- a/src/pybind/mgr/tox.ini +++ b/src/pybind/mgr/tox.ini @@ -93,7 +93,6 @@ commands = -m devicehealth \ -m diskprediction_local \ -m hello \ - -m nvmeof \ -m influx \ -m iostat \ -m localpool \ @@ -147,7 +146,6 @@ modules = devicehealth \ diskprediction_local \ hello \ - nvmeof \ iostat \ localpool \ mgr_module.py \ -- 2.47.3