From f0ffb26b80627931096d2e8283613a9310fbca9c Mon Sep 17 00:00:00 2001 From: John Mulligan Date: Wed, 2 Jul 2025 14:06:21 -0400 Subject: [PATCH] mgr/smb: replace cross check if-block with singledispatch The previous code relied on a cascading block of if-isintance statements that was dense and somwehat error prone as I found out during an experiment to add a new top level resource type. Refactor the cross check function to use singledispatch: https://docs.python.org/3.9/library/functools.html#functools.singledispatch Now instead of correctly adding check function(s) and updating the if-block, only new check functions using the register decorator is needed. Note that making this checking more generic is difficult as each different resource type really has different cross checking needs. Signed-off-by: John Mulligan --- src/pybind/mgr/smb/staging.py | 72 ++++++++++++++++------------------- 1 file changed, 33 insertions(+), 39 deletions(-) diff --git a/src/pybind/mgr/smb/staging.py b/src/pybind/mgr/smb/staging.py index 9c9e11d84e4fd..d92f7c9ea0c9b 100644 --- a/src/pybind/mgr/smb/staging.py +++ b/src/pybind/mgr/smb/staging.py @@ -1,13 +1,14 @@ from typing import ( + Any, Collection, Dict, Iterator, List, Optional, Set, - Union, ) +import functools import logging import operator @@ -41,9 +42,6 @@ from .resources import SMBResource from .results import ErrorResult, Result, ResultGroup from .utils import checked -ClusterRef = Union[resources.Cluster, resources.RemovedCluster] -ShareRef = Union[resources.Share, resources.RemovedShare] - log = logging.getLogger(__name__) @@ -163,6 +161,7 @@ def ug_refs(cluster: resources.Cluster) -> Collection[str]: } +@functools.singledispatch def cross_check_resource( resource: SMBResource, staging: Staging, @@ -170,31 +169,14 @@ def cross_check_resource( path_resolver: PathResolver, earmark_resolver: EarmarkResolver, ) -> None: - if isinstance(resource, (resources.Cluster, resources.RemovedCluster)): - _check_cluster(resource, staging) - elif isinstance(resource, (resources.Share, resources.RemovedShare)): - _check_share( - resource, - staging, - path_resolver, - earmark_resolver, - ) - elif isinstance(resource, resources.JoinAuth): - _check_join_auths(resource, staging) - elif isinstance(resource, resources.UsersAndGroups): - _check_users_and_groups(resource, staging) - else: - raise TypeError('not a valid smb resource') - + raise TypeError(f'not a valid smb resource: {type(resource)}') -def _check_cluster(cluster: ClusterRef, staging: Staging) -> None: - """Check that the cluster resource can be updated.""" - if cluster.intent == Intent.PRESENT: - return _check_cluster_present(cluster, staging) - return _check_cluster_removed(cluster, staging) - -def _check_cluster_removed(cluster: ClusterRef, staging: Staging) -> None: +@cross_check_resource.register +def _check_removed_cluster_resource( + cluster: resources.RemovedCluster, staging: Staging, **_: Any +) -> None: + assert cluster.intent == Intent.REMOVED share_ids = ShareEntry.ids(staging) clusters_used = {cid for cid, _ in share_ids} if cluster.cluster_id in clusters_used: @@ -211,8 +193,11 @@ def _check_cluster_removed(cluster: ClusterRef, staging: Staging) -> None: ) -def _check_cluster_present(cluster: ClusterRef, staging: Staging) -> None: - assert isinstance(cluster, resources.Cluster) +@cross_check_resource.register +def _check_cluster_resource( + cluster: resources.Cluster, staging: Staging, **_: Any +) -> None: + assert cluster.intent == Intent.PRESENT cluster.validate() if not staging.is_new(cluster): _check_cluster_modifications(cluster, staging) @@ -291,15 +276,22 @@ def _check_cluster_modifications( raise ErrorResult(cluster, msg, status={'hint': hint}) -def _check_share( - share: ShareRef, +@cross_check_resource.register +def _check_removed_share_resource( + share: resources.RemovedShare, staging: Staging, **_: Any +) -> None: + assert share.intent == Intent.REMOVED + + +@cross_check_resource.register +def _check_share_resource( + share: resources.Share, staging: Staging, - resolver: PathResolver, + path_resolver: PathResolver, earmark_resolver: EarmarkResolver, ) -> None: """Check that the share resource can be updated.""" - if share.intent == Intent.REMOVED: - return + assert share.intent == Intent.PRESENT assert isinstance(share, resources.Share) share.validate() if share.cluster_id not in ClusterEntry.ids(staging): @@ -310,7 +302,7 @@ def _check_share( ) assert share.cephfs is not None try: - volpath = resolver.resolve_exists( + volpath = path_resolver.resolve_exists( share.cephfs.volume, share.cephfs.subvolumegroup, share.cephfs.subvolume, @@ -411,8 +403,9 @@ def _share_name_in_use( return found_curr[0].get()['share_id'] -def _check_join_auths( - join_auth: resources.JoinAuth, staging: Staging +@cross_check_resource.register +def _check_join_auth_resource( + join_auth: resources.JoinAuth, staging: Staging, **_: Any ) -> None: """Check that the JoinAuth resource can be updated.""" if join_auth.intent == Intent.PRESENT: @@ -455,8 +448,9 @@ def _check_join_auths_present( ) -def _check_users_and_groups( - users_and_groups: resources.UsersAndGroups, staging: Staging +@cross_check_resource.register +def _check_users_and_groups_resource( + users_and_groups: resources.UsersAndGroups, staging: Staging, **_: Any ) -> None: """Check that the UsersAndGroups resource can be updated.""" if users_and_groups.intent == Intent.PRESENT: -- 2.39.5