From: John Mulligan Date: Tue, 11 Feb 2025 19:37:26 +0000 (-0500) Subject: cephadm: refactor get_container_info to use new listing apis X-Git-Tag: v20.3.0~256^2~1 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=361c69d431c9c5bdeba3b70d4a6e2faac02b48bc;p=ceph.git cephadm: refactor get_container_info to use new listing apis Thoroughly refactor the get_container_info function so that it uses the new listing functions and updaters with the aim to avoid redundant operations that can waste resources. A fairly heavy rework of the tests was needed too. Signed-off-by: John Mulligan --- diff --git a/src/cephadm/cephadm.py b/src/cephadm/cephadm.py index 6085bcaaa471..86f5bbf5d73a 100755 --- a/src/cephadm/cephadm.py +++ b/src/cephadm/cephadm.py @@ -467,65 +467,79 @@ def update_default_image(ctx: CephadmContext) -> None: ctx.image = _get_default_image(ctx) -def get_container_info(ctx: CephadmContext, daemon_filter: str, by_name: bool) -> Optional[ContainerInfo]: +def get_container_info( + ctx: CephadmContext, daemon_filter: str, by_name: bool +) -> Optional[ContainerInfo]: """ :param ctx: Cephadm context :param daemon_filter: daemon name or type :param by_name: must be set to True if daemon name is provided :return: Container information or None """ - def daemon_name_or_type(daemon: Dict[str, str]) -> str: - return daemon['name'] if by_name else daemon['name'].split('.', 1)[0] - if by_name and '.' not in daemon_filter: - logger.warning(f'Trying to get container info using invalid daemon name {daemon_filter}') + logger.warning( + f'Trying to get container info using invalid daemon name {daemon_filter}' + ) return None - if by_name: - # NOTE: we are not passing detail=False to this list_daemons call - # as we want the container_image name in the case where we are - # doing this by name and this is skipped when detail=False - matching_daemons = list_daemons(ctx, daemon_name=daemon_filter) - if len(matching_daemons) > 1: - logger.warning(f'Found multiple daemons sharing same name: {daemon_filter}') - # Take the first daemon we find that is actually running, or just the - # first in the list if none are running - matched_daemon = None - for d in matching_daemons: - if 'state' in d and d['state'] == 'running': - matched_daemon = d - break - if not matched_daemon: - matched_daemon = matching_daemons[0] - matching_daemons = [matched_daemon] - else: - # NOTE: we are passing detail=False here as in this case where we are not - # doing it by_name, we really only need the names of the daemons. Additionally, - # when not doing it by_name, we are getting the info for all daemons on the - # host, and doing this with detail=True tends to be slow. - daemons = list_daemons(ctx, detail=False) - matching_daemons = [d for d in daemons if daemon_name_or_type(d) == daemon_filter and d['fsid'] == ctx.fsid] - if matching_daemons: - if ( - by_name - and 'state' in matching_daemons[0] - and matching_daemons[0]['state'] != 'running' - and 'container_image_name' in matching_daemons[0] - and matching_daemons[0]['container_image_name'] - ): - # this daemon contianer is not running so the regular `podman/docker inspect` on the - # container will not help us. If we have the image name from the list_daemons output - # we can try that. - image_name = matching_daemons[0]['container_image_name'] - cinfo = parsed_container_image_stats(ctx, image_name) - if cinfo: - return cinfo - else: - d_type, d_id = matching_daemons[0]['name'].split('.', 1) - cinfo = get_container_stats( - ctx, DaemonIdentity(ctx.fsid, d_type, d_id) - ) - if cinfo: - return cinfo + + # configure filters: fsid and (daemon name or daemon type) + kwargs = { + 'fsid': ctx.fsid, + ('daemon_name' if by_name else 'daemon_type'): daemon_filter, + } + # use keep_container_info to cache the ContainerInfo generated + # during the loop and hopefully avoid having to perform the same + # lookup right away. + _cinfo_key = '_container_info' + _updater = CoreStatusUpdater(keep_container_info=_cinfo_key) + matching_daemons = [ + _updater.expand(ctx, entry) + for entry in daemons_matching(ctx, **kwargs) + ] + + if not matching_daemons: + # no matches at all + logger.debug( + 'no daemons match: daemon_filter=%r, by_name=%r', + daemon_filter, + by_name, + ) + return None + if by_name and len(matching_daemons) > 1: + # too many matches while searching by name + logger.warning( + f'Found multiple daemons sharing same name: {daemon_filter}' + ) + # Prefer to take the first daemon we find that is actually running, or + # just the first in the list if none are running + # (key reminder: false (0) sorts before true (1)) + matching_daemons = sorted( + matching_daemons, key=lambda d: d.get('state') != 'running' + ) + + matched_deamon = matching_daemons[0] + is_running = matched_deamon.get('state') == 'running' + image_name = matched_deamon.get('container_image_name', '') + if is_running: + cinfo = matched_deamon.get(_cinfo_key) + if cinfo: + # found a cached ContainerInfo while getting daemon statuses + return cinfo + return get_container_stats( + ctx, + DaemonIdentity.from_name( + matched_deamon['fsid'], matched_deamon['name'] + ), + ) + elif image_name: + # this daemon's container is not running. the regular container inspect + # command will not work. Fall back to inspecting the container image + assert isinstance(image_name, str) + return parsed_container_image_stats(ctx, image_name) + # not running, but no image name to look up! + logger.debug( + 'bad daemon state: no image, not running: %r', matched_deamon + ) return None diff --git a/src/cephadm/tests/test_cephadm.py b/src/cephadm/tests/test_cephadm.py index 192621ab961d..0bf9f492fb83 100644 --- a/src/cephadm/tests/test_cephadm.py +++ b/src/cephadm/tests/test_cephadm.py @@ -786,10 +786,32 @@ class TestCephAdm(object): output, funkypatch, ): + import cephadmlib.listing + import cephadmlib.daemon_identity + ctx = _cephadm.CephadmContext() ctx.fsid = '00000000-0000-0000-0000-0000deadbeef' ctx.container_engine = mock_podman() - funkypatch.patch('cephadm.list_daemons').return_value = daemon_list + + def _dms(*args, **kwargs): + for val in daemon_list: + yield cephadmlib.listing.DaemonEntry( + cephadmlib.daemon_identity.DaemonIdentity.from_name( + val['fsid'], val['name'] + ), + val, + '', + ) + + funkypatch.patch('cephadmlib.listing.daemons').side_effect = _dms + funkypatch.patch('cephadmlib.systemd.check_unit').return_value = ( + True, + 'running' if container_stats else 'stopped', + '', + ) + funkypatch.patch('cephadmlib.call_wrappers.call').side_effect = ( + ValueError + ) cinfo = ( _cephadm.ContainerInfo(*container_stats) if container_stats @@ -803,9 +825,18 @@ class TestCephAdm(object): ) def test_get_container_info_daemon_down(self, funkypatch): + import cephadmlib.listing + import cephadmlib.daemon_identity + funkypatch.patch('cephadmlib.systemd.check_unit').return_value = (True, 'stopped', '') + funkypatch.patch('cephadmlib.call_wrappers.call').side_effect = ValueError _get_stats_by_name = funkypatch.patch('cephadmlib.container_engines.parsed_container_image_stats') _get_stats = funkypatch.patch('cephadmlib.container_types.get_container_stats') - _list_daemons = funkypatch.patch('cephadm.list_daemons') + _daemons = funkypatch.patch('cephadmlib.listing.daemons') + + class FakeUpdater(cephadmlib.listing.NoOpDaemonStatusUpdater): + def __init__(self, *args, **kwargs): + pass + funkypatch.patch('cephadmlib.listing_updaters.CoreStatusUpdater', dest=FakeUpdater) ctx = _cephadm.CephadmContext() ctx.fsid = '5e39c134-dfc5-11ee-a344-5254000ee071' @@ -844,7 +875,14 @@ class TestCephAdm(object): "deployed": "2024-03-11T17:37:23.520061Z", "configured": "2024-03-11T17:37:28.494075Z" } - _list_daemons.return_value = [down_osd_json] + + _current_daemons = [] + def _dms(*args, **kwargs): + for d in _current_daemons: + ident = cephadmlib.daemon_identity.DaemonIdentity.from_name(d['fsid'], d['name']) + yield cephadmlib.listing.DaemonEntry(ident, d, '') + _daemons.side_effect = _dms + _current_daemons[:] = [down_osd_json] expected_container_info = _cephadm.ContainerInfo( container_id='', @@ -867,7 +905,7 @@ class TestCephAdm(object): up_osd_json = copy.deepcopy(down_osd_json) up_osd_json['state'] = 'running' _get_stats.return_value = _cephadm.ContainerInfo('container_id', 'image_name','image_id','the_past','') - _list_daemons.return_value = [down_osd_json, up_osd_json] + _current_daemons[:] = [down_osd_json, up_osd_json] expected_container_info = _cephadm.ContainerInfo( container_id='container_id',