Merge pull request ceph#57293 from phlogistonjohn/jjm-smb-create-only

smb: have create cluster and create share commands only create

Reviewed-by: Adam King <[email protected]>
Reviewed-by: Avan Thakkar <[email protected]>

Author: adk3798

src/pybind/mgr/smb/handler.py

@@ -190,37 +190,44 @@ def __init__(self, store: ConfigStore) -> None:
         self.destination_store = store
         self.incoming: Dict[EntryKey, SMBResource] = {}
         self.deleted: Dict[EntryKey, SMBResource] = {}
-        self._keycache: Set[EntryKey] = set()
+        self._store_keycache: Set[EntryKey] = set()
+        self._virt_keycache: Set[EntryKey] = set()
 
     def stage(self, resource: SMBResource) -> None:
-        self._keycache = set()
+        self._virt_keycache = set()
         ekey = resource_key(resource)
         if resource.intent == Intent.REMOVED:
             self.deleted[ekey] = resource
         else:
             self.deleted.pop(ekey, None)
             self.incoming[ekey] = resource
 
-    def _virtual_keys(self) -> Iterator[EntryKey]:
-        new = set(self.incoming.keys())
-        for ekey in self.destination_store:
-            if ekey in self.deleted:
-                continue
-            yield ekey
-            new.discard(ekey)
-        for ekey in new:
-            yield ekey
+    def _virtual_keys(self) -> Collection[EntryKey]:
+        if self._virt_keycache:
+            return self._virt_keycache
+        self._virt_keycache = set(self._store_keys()) - set(
+            self.deleted
+        ) | set(self.incoming)
+        return self._virt_keycache
+
+    def _store_keys(self) -> Collection[EntryKey]:
+        if not self._store_keycache:
+            self._store_keycache = set(self.destination_store)
+        return self._store_keycache
 
     def __iter__(self) -> Iterator[EntryKey]:
-        self._keycache = set(self._virtual_keys())
-        return iter(self._keycache)
+        return iter(self._virtual_keys())
 
     def namespaces(self) -> Collection[str]:
         return {k[0] for k in self}
 
     def contents(self, ns: str) -> Collection[str]:
         return {kname for kns, kname in self if kns == ns}
 
+    def is_new(self, resource: SMBResource) -> bool:
+        ekey = resource_key(resource)
+        return ekey not in self._store_keys()
+
     def get_cluster(self, cluster_id: str) -> resources.Cluster:
         ekey = (str(ClusterEntry.namespace), cluster_id)
         if ekey in self.incoming:
@@ -335,7 +342,12 @@ def __init__(
             f' orch {self._orch!r}'
         )
 
-    def apply(self, inputs: Iterable[SMBResource]) -> ResultGroup:
+    def apply(
+        self, inputs: Iterable[SMBResource], *, create_only: bool = False
+    ) -> ResultGroup:
+        """Apply resource configuration changes.
+        Set `create_only` to disable changing existing resource values.
+        """
         log.debug('applying changes to internal data store')
         results = ResultGroup()
         staging = _Staging(self.internal_store)
@@ -344,7 +356,9 @@ def apply(self, inputs: Iterable[SMBResource]) -> ResultGroup:
             for resource in incoming:
                 staging.stage(resource)
             for resource in incoming:
-                results.append(self._check(resource, staging))
+                results.append(
+                    self._check(resource, staging, create_only=create_only)
+                )
         except ErrorResult as err:
             results.append(err)
         except Exception as err:
@@ -421,9 +435,22 @@ def _search_resources(self, matcher: _Matcher) -> List[SMBResource]:
         log.debug("search found %d resources", len(out))
         return out
 
-    def _check(self, resource: SMBResource, staging: _Staging) -> Result:
+    def _check(
+        self,
+        resource: SMBResource,
+        staging: _Staging,
+        *,
+        create_only: bool = False,
+    ) -> Result:
         """Check/validate a staged resource."""
         log.debug('staging resource: %r', resource)
+        if create_only:
+            if not staging.is_new(resource):
+                return Result(
+                    resource,
+                    success=False,
+                    msg='a resource with the same ID already exists',
+                )
         try:
             if isinstance(
                 resource, (resources.Cluster, resources.RemovedCluster)

src/pybind/mgr/smb/module.py

@@ -178,7 +178,7 @@ def cluster_create(
             placement=pspec,
         )
         to_apply.append(cluster)
-        return self._handler.apply(to_apply).squash(cluster)
+        return self._handler.apply(to_apply, create_only=True).squash(cluster)
 
     @cli.SMBCommand('cluster rm', perm='rw')
     def cluster_rm(self, cluster_id: str) -> handler.Result:
@@ -220,7 +220,7 @@ def share_create(
                 subvolume=subvolume,
             ),
         )
-        return self._handler.apply([share]).one()
+        return self._handler.apply([share], create_only=True).one()
 
     @cli.SMBCommand('share rm', perm='rw')
     def share_rm(self, cluster_id: str, share_id: str) -> handler.Result:

src/pybind/mgr/smb/tests/test_handler.py

@@ -1116,3 +1116,91 @@ def test_rand_name():
     assert len(name) == 18
     name = smb.handler.rand_name('')
     assert len(name) == 8
+
+
+def test_apply_with_create_only(thandler):
+    test_apply_full_cluster_create(thandler)
+
+    to_apply = [
+        smb.resources.Cluster(
+            cluster_id='mycluster1',
+            auth_mode=smb.enums.AuthMode.ACTIVE_DIRECTORY,
+            domain_settings=smb.resources.DomainSettings(
+                realm='MYDOMAIN.EXAMPLE.ORG',
+                join_sources=[
+                    smb.resources.JoinSource(
+                        source_type=smb.enums.JoinSourceType.RESOURCE,
+                        ref='join1',
+                    ),
+                ],
+            ),
+            custom_dns=['192.168.76.204'],
+        ),
+        smb.resources.Share(
+            cluster_id='mycluster1',
+            share_id='homedirs',
+            name='Altered Home Directries',
+            cephfs=smb.resources.CephFSStorage(
+                volume='cephfs',
+                subvolume='homedirs',
+                path='/',
+            ),
+        ),
+        smb.resources.Share(
+            cluster_id='mycluster1',
+            share_id='foodirs',
+            name='Foo Directries',
+            cephfs=smb.resources.CephFSStorage(
+                volume='cephfs',
+                subvolume='homedirs',
+                path='/foo',
+            ),
+        ),
+    ]
+    results = thandler.apply(to_apply, create_only=True)
+    assert not results.success
+    rs = results.to_simplified()
+    assert len(rs['results']) == 3
+    assert (
+        rs['results'][0]['msg']
+        == 'a resource with the same ID already exists'
+    )
+    assert (
+        rs['results'][1]['msg']
+        == 'a resource with the same ID already exists'
+    )
+
+    # no changes to the store
+    assert (
+        'shares',
+        'mycluster1.foodirs',
+    ) not in thandler.internal_store.data
+    assert ('shares', 'mycluster1.homedirs') in thandler.internal_store.data
+    assert (
+        thandler.internal_store.data[('shares', 'mycluster1.homedirs')][
+            'name'
+        ]
+        == 'Home Directries'
+    )
+
+    # foodirs share is new, it can be applied separately
+    to_apply = [
+        smb.resources.Share(
+            cluster_id='mycluster1',
+            share_id='foodirs',
+            name='Foo Directries',
+            cephfs=smb.resources.CephFSStorage(
+                volume='cephfs',
+                subvolume='homedirs',
+                path='/foo',
+            ),
+        ),
+    ]
+    results = thandler.apply(to_apply, create_only=True)
+    assert results.success
+    rs = results.to_simplified()
+    assert len(rs['results']) == 1
+    assert (
+        'shares',
+        'mycluster1.foodirs',
+    ) in thandler.internal_store.data