diff options
-rw-r--r-- | qa/tasks/thrashosds-health.yaml | 2 | ||||
-rwxr-xr-x | qa/workunits/cephtool/test.sh | 16 | ||||
-rwxr-xr-x | src/cephadm/cephadm.py | 4 | ||||
-rw-r--r-- | src/osdc/Journaler.cc | 16 | ||||
-rw-r--r-- | src/pybind/mgr/cephadm/module.py | 5 | ||||
-rw-r--r-- | src/pybind/mgr/cephadm/serve.py | 2 | ||||
-rw-r--r-- | src/pybind/mgr/cephadm/services/cephadmservice.py | 2 | ||||
-rw-r--r-- | src/pybind/mgr/cephadm/services/nfs.py | 2 | ||||
-rw-r--r-- | src/pybind/mgr/smb/fs.py | 118 | ||||
-rw-r--r-- | src/pybind/mgr/smb/handler.py | 2 | ||||
-rw-r--r-- | src/pybind/mgr/smb/module.py | 2 | ||||
-rw-r--r-- | src/pybind/mgr/smb/tests/test_fs.py | 108 | ||||
-rw-r--r-- | src/pybind/mgr/smb/tests/test_smb.py | 55 |
13 files changed, 308 insertions, 26 deletions
diff --git a/qa/tasks/thrashosds-health.yaml b/qa/tasks/thrashosds-health.yaml index f06fd68e9be..7399022abad 100644 --- a/qa/tasks/thrashosds-health.yaml +++ b/qa/tasks/thrashosds-health.yaml @@ -28,3 +28,5 @@ overrides: - mons down - mon down - out of quorum + - noscrub + - nodeep-scrub diff --git a/qa/workunits/cephtool/test.sh b/qa/workunits/cephtool/test.sh index ee8b6029a52..89fdcaf0593 100755 --- a/qa/workunits/cephtool/test.sh +++ b/qa/workunits/cephtool/test.sh @@ -888,7 +888,7 @@ function test_tell_output_file() # only one line of json expect_true sed '2q1' < /tmp/foo > /dev/null expect_true jq -e '.version | length > 0' < /tmp/foo - rm -f /tmp/foo + sudo rm -f /tmp/foo J=$(ceph tell --format=json-pretty --daemon-output-file=/tmp/foo "$name" version) expect_true jq -e '.path == "/tmp/foo"' <<<"$J" @@ -896,24 +896,24 @@ function test_tell_output_file() # more than one line of json expect_false sed '2q1' < /tmp/foo > /dev/null expect_true jq -e '.version | length > 0' < /tmp/foo - rm -f /tmp/foo + sudo rm -f /tmp/foo # Test --daemon-output-file=:tmp: J=$(ceph tell --format=json --daemon-output-file=":tmp:" "$name" version) path=$(jq -r .path <<<"$J") expect_true test -e "$path" # only one line of json - expect_true sed '2q1' < "$path" > /dev/null - expect_true jq -e '.version | length > 0' < "$path" - rm -f "$path" + expect_true sudo sh -c "sed '2q1' < \"$path\" > /dev/null" + expect_true sudo sudo sh -c "jq -e '.version | length > 0' < \"$path\"" + sudo rm -f "$path" J=$(ceph tell --format=json-pretty --daemon-output-file=":tmp:" "$name" version) path=$(jq -r .path <<<"$J") expect_true test -e "$path" # only one line of json - expect_false sed '2q1' < "$path" > /dev/null - expect_true jq -e '.version | length > 0' < "$path" - rm -f "$path" + expect_false sudo sh -c "sed '2q1' < \"$path\" > /dev/null" + expect_true sudo sh -c "jq -e '.version | length > 0' < \"$path\"" + sudo rm -f "$path" } function test_mds_tell() diff --git a/src/cephadm/cephadm.py b/src/cephadm/cephadm.py index 95169358f3a..5deaec55949 100755 --- a/src/cephadm/cephadm.py +++ b/src/cephadm/cephadm.py @@ -2892,6 +2892,10 @@ def command_bootstrap(ctx): cli(['config', 'set', 'mgr', 'mgr/cephadm/container_init', str(ctx.container_init), '--force']) + if ctx.no_cgroups_split: + logger.info('Setting mgr/cephadm/cgroups_split to false') + cli(['config', 'set', 'mgr', 'mgr/cephadm/cgroups_split', 'false', '--force']) + if not ctx.skip_dashboard: prepare_dashboard(ctx, uid, gid, cli, wait_for_mgr_restart) diff --git a/src/osdc/Journaler.cc b/src/osdc/Journaler.cc index 04b90fb5952..c1c1178bd7f 100644 --- a/src/osdc/Journaler.cc +++ b/src/osdc/Journaler.cc @@ -493,7 +493,21 @@ void Journaler::_finish_write_head(int r, Header &wrote, } ceph_assert(!readonly); ldout(cct, 10) << "_finish_write_head " << wrote << dendl; - last_committed = wrote; + if (wrote.write_pos < last_committed.write_pos || + wrote.expire_pos < last_committed.expire_pos || + wrote.trimmed_pos < last_committed.trimmed_pos) { + lderr(cct) << __func__ << ": not updating last_committed: " + << "(wrote.write_pos/last_committed.write_pos=" + << wrote.write_pos << "," << last_committed.write_pos << "), " + << "(wrote.expire_pos/last_committed.expire_pos=" + << wrote.expire_pos << "," << last_committed.expire_pos << "), " + << "(wrote.trimmed_pos/last_committed.trimmed_pos=" + << wrote.trimmed_pos << "," << last_committed.trimmed_pos << ")" + << dendl; + ceph_abort(); + } else { + last_committed = wrote; + } if (oncommit) { oncommit->complete(r); } diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py index b28332bb6e6..c34f77a740c 100644 --- a/src/pybind/mgr/cephadm/module.py +++ b/src/pybind/mgr/cephadm/module.py @@ -3321,6 +3321,9 @@ Then run the following: 'data': self._preview_osdspecs(osdspecs=[cast(DriveGroupSpec, spec)])} svc = self.cephadm_services[spec.service_type] + rank_map = None + if svc.ranked(spec): + rank_map = self.spec_store[spec.service_name()].rank_map ha = HostAssignment( spec=spec, hosts=self.cache.get_schedulable_hosts(), @@ -3329,7 +3332,7 @@ Then run the following: networks=self.cache.networks, daemons=self.cache.get_daemons_by_service(spec.service_name()), allow_colo=svc.allow_colo(), - rank_map=self.spec_store[spec.service_name()].rank_map if svc.ranked() else None + rank_map=rank_map ) ha.validate() hosts, to_add, to_remove = ha.place() diff --git a/src/pybind/mgr/cephadm/serve.py b/src/pybind/mgr/cephadm/serve.py index 59e06bbd024..eaaf4386f62 100644 --- a/src/pybind/mgr/cephadm/serve.py +++ b/src/pybind/mgr/cephadm/serve.py @@ -780,7 +780,7 @@ class CephadmServe: } rank_map = None - if svc.ranked(): + if svc.ranked(spec): rank_map = self.mgr.spec_store[spec.service_name()].rank_map or {} ha = HostAssignment( spec=spec, diff --git a/src/pybind/mgr/cephadm/services/cephadmservice.py b/src/pybind/mgr/cephadm/services/cephadmservice.py index ec9df98413a..72e6177bc1d 100644 --- a/src/pybind/mgr/cephadm/services/cephadmservice.py +++ b/src/pybind/mgr/cephadm/services/cephadmservice.py @@ -288,7 +288,7 @@ class CephadmService(metaclass=ABCMeta): """ return None - def ranked(self) -> bool: + def ranked(self, spec: ServiceSpec) -> bool: """ If True, we will assign a stable rank (0, 1, ...) and monotonically increasing generation (0, 1, ...) to each daemon we create/deploy. diff --git a/src/pybind/mgr/cephadm/services/nfs.py b/src/pybind/mgr/cephadm/services/nfs.py index f46f65b084b..a0d7da9bb7e 100644 --- a/src/pybind/mgr/cephadm/services/nfs.py +++ b/src/pybind/mgr/cephadm/services/nfs.py @@ -23,7 +23,7 @@ logger = logging.getLogger(__name__) class NFSService(CephService): TYPE = 'nfs' - def ranked(self) -> bool: + def ranked(self, spec: ServiceSpec) -> bool: return True def fence(self, daemon_id: str) -> None: diff --git a/src/pybind/mgr/smb/fs.py b/src/pybind/mgr/smb/fs.py index 8aaa8bcde06..dc9613f21fc 100644 --- a/src/pybind/mgr/smb/fs.py +++ b/src/pybind/mgr/smb/fs.py @@ -1,8 +1,9 @@ -from typing import List, Optional +from typing import Dict, List, Optional, Tuple import logging import posixpath import stat +import time import cephfs from mgr_util import CephfsClient, Module_T, open_filesystem @@ -60,16 +61,13 @@ class CephFSPathResolver: self._mgr = mgr self._cephfs_client = client or CephfsClient(mgr) - def resolve( - self, volume: str, subvolumegroup: str, subvolume: str, path: str + def resolve_subvolume_path( + self, volume: str, subvolumegroup: str, subvolume: str ) -> str: - """Given a volume, subvolumegroup, subvolume, and path, return the real - path within the file system. subvolumegroup and subvolume may be empty - strings when no subvolume is being used. + """Given a volume, subvolumegroup, and subvolume, return the real path + within the file system. subvolumegroup and subvolume may be empty strings + when no subvolume is being used. """ - path = path.lstrip('/') - if not (subvolumegroup or subvolume): - return f'/{path}' cmd = { 'prefix': 'fs subvolume getpath', 'vol_name': volume, @@ -81,8 +79,23 @@ class CephFSPathResolver: ret, data, status = self._mgr.mon_command(cmd) if ret != 0: raise CephFSSubvolumeResolutionError(status) - log.debug('Mapped subvolume to path: %r', data) - return posixpath.join(data.strip(), path) + log.info('Mapped subvolume to path: %r', data) + return data.strip() + + def resolve( + self, volume: str, subvolumegroup: str, subvolume: str, path: str + ) -> str: + """Given a volume, subvolumegroup, subvolume, and path, return the real + path within the file system. subvolumegroup and subvolume may be empty + strings when no subvolume is being used. + """ + path = path.lstrip('/') + if not (subvolumegroup or subvolume): + return f'/{path}' + subvolume_path = self.resolve_subvolume_path( + volume, subvolumegroup, subvolume + ) + return posixpath.join(subvolume_path, path) def resolve_exists( self, volume: str, subvolumegroup: str, subvolume: str, path: str @@ -108,3 +121,86 @@ class CephFSPathResolver: raise NotADirectoryError(volpath) log.debug('Verified that %r exists in %r', volpath, volume) return volpath + + +class _TTLCache: + def __init__(self, maxsize: int = 512, ttl: float = 300.0) -> None: + self.cache: Dict[Tuple[str, str, str], Tuple[str, float]] = {} + self.maxsize: int = maxsize + self.ttl: float = ttl + + def _evict(self) -> None: + """Evicts items that have expired or if cache size exceeds maxsize.""" + current_time: float = time.monotonic() + keys_to_evict: list[Tuple[str, str, str]] = [ + key + for key, (_, timestamp) in self.cache.items() + if current_time - timestamp > self.ttl + ] + for key in keys_to_evict: + del self.cache[key] + + # Further evict if cache size exceeds maxsize + if len(self.cache) > self.maxsize: + for key in list(self.cache.keys())[ + : len(self.cache) - self.maxsize + ]: + del self.cache[key] + + def get(self, key: Tuple[str, str, str]) -> Optional[str]: + """Retrieve item from cache if it exists and is not expired.""" + self._evict() # Ensure expired items are removed + if key in self.cache: + value, _ = self.cache[key] + return value + return None + + def set(self, key: Tuple[str, str, str], value: str) -> None: + """Set item in cache, evicting expired or excess items.""" + self._evict() # Ensure expired items are removed + self.cache[key] = (value, time.monotonic()) + + def clear(self) -> None: + """Clear all items in the cache.""" + self.cache.clear() + + def __len__(self) -> int: + """Return the number of items currently in the cache.""" + return len(self.cache) + + +class CachingCephFSPathResolver(CephFSPathResolver): + """ + A subclass of CephFSPathResolver that adds caching to the resolve method + to improve performance by reducing redundant path resolutions. + + This implementation uses a TTL (Time-To-Live) cache rather than an LRU (Least + Recently Used) cache. The TTL cache is preferred in this scenario because + the validity of cached paths is time-sensitive, and we want to ensure that + paths are refreshed after a certain period regardless of access frequency. + Rlock can be used to synchronize access to the cache, but that is something + not required for now & can be later tested. + """ + + def __init__( + self, mgr: Module_T, *, client: Optional[CephfsClient] = None + ) -> None: + super().__init__(mgr, client=client) + # Initialize a TTL cache. + self._cache = _TTLCache(maxsize=512, ttl=5) + + def resolve_subvolume_path( + self, volume: str, subvolumegroup: str, subvolume: str + ) -> str: + cache_key = (volume, subvolumegroup, subvolume) + cached_path = self._cache.get(cache_key) + if cached_path: + log.debug("Cache hit for key: %r", cache_key) + return cached_path + + log.debug("Cache miss for key: %r", cache_key) + resolved_path = super().resolve_subvolume_path( + volume, subvolumegroup, subvolume + ) + self._cache.set(cache_key, resolved_path) + return resolved_path diff --git a/src/pybind/mgr/smb/handler.py b/src/pybind/mgr/smb/handler.py index 84702e72f78..f230d7952d7 100644 --- a/src/pybind/mgr/smb/handler.py +++ b/src/pybind/mgr/smb/handler.py @@ -411,7 +411,7 @@ class ClusterConfigHandler: for cluster_id in self.cluster_ids(): if (resources.Cluster, cluster_id) in matcher: out.append(self._cluster_entry(cluster_id).get_cluster()) - for share_id in cluster_shares[cluster_id]: + for share_id in cluster_shares.get(cluster_id, []): if (resources.Share, cluster_id, share_id) in matcher: out.append( self._share_entry( diff --git a/src/pybind/mgr/smb/module.py b/src/pybind/mgr/smb/module.py index 70c97d1a975..43ad681769a 100644 --- a/src/pybind/mgr/smb/module.py +++ b/src/pybind/mgr/smb/module.py @@ -52,7 +52,7 @@ class Module(orchestrator.OrchestratorClientMixin, MgrModule): self._public_store = ( public_store or rados_store.RADOSConfigStore.init(self) ) - path_resolver = path_resolver or fs.CephFSPathResolver(self) + path_resolver = path_resolver or fs.CachingCephFSPathResolver(self) # Why the honk is the cast needed but path_resolver doesn't need it?? # Sometimes mypy drives me batty. authorizer = cast( diff --git a/src/pybind/mgr/smb/tests/test_fs.py b/src/pybind/mgr/smb/tests/test_fs.py index 7af20eb6265..5653ccfd081 100644 --- a/src/pybind/mgr/smb/tests/test_fs.py +++ b/src/pybind/mgr/smb/tests/test_fs.py @@ -1,8 +1,11 @@ +import time +import unittest from unittest import mock import pytest import smb.fs +from smb.fs import _TTLCache def test_mocked_fs_authorizer(): @@ -65,3 +68,108 @@ def test_mocked_fs_path_resolver(monkeypatch): ) with pytest.raises(FileNotFoundError): fspr.resolve_exists('cephfs', 'alpha', 'beta', '/zowie') + + +class TestTTLCache(unittest.TestCase): + def setUp(self): + self.cache = _TTLCache( + ttl=1, maxsize=3 + ) # Short TTL and small size for testing + + def test_cache_set_and_get(self): + self.cache.set(('key1', 'key2', 'key3'), ('value1', 'val', 'test')) + self.assertEqual( + self.cache.get(('key1', 'key2', 'key3')), + ('value1', 'val', 'test'), + ) + + def test_cache_expiry(self): + self.cache.set(('key1', 'key2', 'key3'), ('value1', 'val', 'test')) + time.sleep(1.5) # Wait for the TTL to expire + self.assertIsNone(self.cache.get(('key1', 'key2', 'key3'))) + + def test_cache_eviction(self): + # Fill the cache to maxsize + self.cache.set(('key1', 'key2', 'key3'), ('value1', 'val', 'test')) + self.cache.set(('key4', 'key5', 'key6'), ('value2', 'val', 'test')) + self.cache.set(('key7', 'key8', 'key9'), ('value3', 'val', 'test')) + + # Add another entry to trigger eviction of the oldest + self.cache.set(('key10', 'key11', 'key12'), ('value4', 'val', 'test')) + + # Ensure oldest entry is evicted + self.assertIsNone(self.cache.get(('key1', 'key2', 'key3'))) + + # Ensure other entries are present + self.assertEqual( + self.cache.get(('key4', 'key5', 'key6')), + ('value2', 'val', 'test'), + ) + self.assertEqual( + self.cache.get(('key7', 'key8', 'key9')), + ('value3', 'val', 'test'), + ) + self.assertEqual( + self.cache.get(('key10', 'key11', 'key12')), + ('value4', 'val', 'test'), + ) + + def test_cache_clear(self): + self.cache.set(('key1', 'key2', 'key3'), ('value1', 'val', 'test')) + self.cache.clear() + self.assertIsNone(self.cache.get(('key1', 'key2', 'key3'))) + + +def test_caching_fs_path_resolver(monkeypatch): + monkeypatch.setattr('cephfs.ObjectNotFound', KeyError) + + def mmcmd(cmd): + if cmd['prefix'] == 'fs subvolume getpath': + if ( + cmd['vol_name'] == 'cached_cephfs' + and cmd['sub_name'] == 'cached_beta' + ): + return 0, '/volumes/cool/path/f00d-600d', '' + return -5, '', 'cached_eek' + + m = mock.MagicMock() + m.mon_command.side_effect = mmcmd + + fspr = smb.fs.CachingCephFSPathResolver(m, client=m) + + # Resolve a path (cache miss) + path = fspr.resolve( + 'cached_cephfs', 'cached_alpha', 'cached_beta', '/zowie' + ) + assert path == '/volumes/cool/path/f00d-600d/zowie' + assert len(fspr._cache) == 1 + assert m.mon_command.call_count == 1 + + # Resolve the same path again (cache hit) + path = fspr.resolve( + 'cached_cephfs', 'cached_alpha', 'cached_beta', '/zowie' + ) + assert path == '/volumes/cool/path/f00d-600d/zowie' + + # Ensure cache size remains the same + assert len(fspr._cache) == 1 + assert m.mon_command.call_count == 1 + + path = fspr.resolve('cached_cephfs', '', '', '/zowie') + assert path == '/zowie' + + # If subvolume is empty cache size should remain the same + assert len(fspr._cache) == 1 + assert m.mon_command.call_count == 1 + + # Clear cache and validate + fspr._cache.clear() + assert len(fspr._cache) == 0 + + # Re-resolve to repopulate cache + path = fspr.resolve( + 'cached_cephfs', 'cached_alpha', 'cached_beta', '/zowie' + ) + assert path == '/volumes/cool/path/f00d-600d/zowie' + assert len(fspr._cache) == 1 + assert m.mon_command.call_count == 2 diff --git a/src/pybind/mgr/smb/tests/test_smb.py b/src/pybind/mgr/smb/tests/test_smb.py index 4ee55e0aa90..8943123f9d1 100644 --- a/src/pybind/mgr/smb/tests/test_smb.py +++ b/src/pybind/mgr/smb/tests/test_smb.py @@ -737,3 +737,58 @@ def test_show_invalid_input(tmodule): _example_cfg_1(tmodule) with pytest.raises(smb.cli.InvalidInputValue): tmodule.show(['ceph.smb.export']) + + +def test_show_cluster_without_shares(tmodule): + # this cluster will have no shares associated with it + tmodule._internal_store.overwrite( + { + 'clusters.foo': { + 'resource_type': 'ceph.smb.cluster', + 'cluster_id': 'foo', + 'auth_mode': 'active-directory', + 'intent': 'present', + 'domain_settings': { + 'realm': 'dom1.example.com', + 'join_sources': [ + { + 'source_type': 'resource', + 'ref': 'foo', + } + ], + }, + }, + 'join_auths.foo': { + 'resource_type': 'ceph.smb.join.auth', + 'auth_id': 'foo', + 'intent': 'present', + 'auth': { + 'username': 'testadmin', + 'password': 'Passw0rd', + }, + }, + } + ) + + res, body, status = tmodule.show.command(['ceph.smb.cluster.foo']) + assert res == 0 + assert ( + body.strip() + == """ +{ + "resource_type": "ceph.smb.cluster", + "cluster_id": "foo", + "auth_mode": "active-directory", + "intent": "present", + "domain_settings": { + "realm": "dom1.example.com", + "join_sources": [ + { + "source_type": "resource", + "ref": "foo" + } + ] + } +} + """.strip() + ) |