NAS-137223 / 26.04 / remove most internal calls to zfs.snapshot.query (#17022)

This removes most internal calls to `zfs.snapshot.query` since we can
now gather this information with our newly minted
`zfs.resource.query_impl` endpoint. The performance/efficiency
differences are extreme (in the positive).

Author: yocalebo

src/middlewared/middlewared/plugins/apps/rollback.py

@@ -72,9 +72,8 @@ def rollback(self, job, app_name, options):
                 app_volume_ds := self.middleware.call_sync('app.get_app_volume_ds', app_name)
             ):
                 snap_name = f'{app_volume_ds}@{options["app_version"]}'
-                if self.middleware.call_sync('zfs.snapshot.query', [['id', '=', snap_name]]):
+                if self.middleware.call_sync('zfs.resource.snapshot_exists', snap_name):
                     job.set_progress(40, f'Rolling back {app_name!r} app to {options["app_version"]!r} version')
-
                     self.middleware.call_sync(
                         'zfs.snapshot.rollback', snap_name, {
                             'force': True,

src/middlewared/middlewared/plugins/apps/upgrade.py

@@ -60,7 +60,7 @@ def take_snapshot_of_hostpath_and_stop_app(self, app, snapshot_hostpath):
                 continue
 
             snap_name = f'{dataset}@{get_upgrade_snap_name(app_info["name"], app_info["version"])}'
-            if self.middleware.call_sync('zfs.snapshot.query', [['id', '=', snap_name]]):
+            if self.middleware.call_sync('zfs.resource.snapshot_exists', snap_name):
                 logger.debug('Snapshot %r already exists for %r app', snap_name, app_info['name'])
                 continue
 
@@ -139,7 +139,7 @@ def upgrade(self, job, app_name, options):
 
             if app_volume_ds := self.middleware.call_sync('app.get_app_volume_ds', app_name):
                 snap_name = f'{app_volume_ds}@{app["version"]}'
-                if self.middleware.call_sync('zfs.snapshot.query', [['id', '=', snap_name]]):
+                if self.middleware.call_sync('zfs.resource.snapshot_exists', snap_name):
                     self.middleware.call_sync('zfs.snapshot.delete', snap_name, {'recursive': True})
 
                 self.middleware.call_sync(

src/middlewared/middlewared/plugins/docker/backup.py

@@ -1,9 +1,10 @@
+import datetime
 import errno
 import logging
 import os
 import shutil
+
 import yaml
-from datetime import datetime
 
 from middlewared.api import api_method
 from middlewared.api.current import (
@@ -42,14 +43,14 @@ def backup(self, job, backup_name):
         """
         self.middleware.call_sync('docker.state.validate')
         docker_config = self.middleware.call_sync('docker.config')
-        name = backup_name or datetime.now().strftime('%F_%T')
+        name = backup_name or datetime.datetime.now().strftime('%F_%T')
         if not validate_snapshot_name(f'a@{name}'):
             # The a@ added is just cosmetic as the function requires a complete snapshot name
             # with the dataset name included in it
             raise CallError(f'{name!r} is not a valid snapshot name. It should be a valid ZFS snapshot name')
 
         snap_name = BACKUP_NAME_PREFIX + name
-        if self.middleware.call_sync('zfs.snapshot.query', [['id', '=', f'{docker_config["dataset"]}@{snap_name}']]):
+        if self.middleware.call_sync('zfs.resource.snapshot_exists', snap_name):
             raise CallError(f'{snap_name!r} snapshot already exists', errno=errno.EEXIST)
 
         if name in self.list_backups():
@@ -95,13 +96,21 @@ def list_backups(self):
 
         backups_base_dir = backup_ds_path()
         backups = {}
-        snapshots = self.middleware.call_sync(
-            'zfs.snapshot.query', [
-                ['name', '^', f'{docker_config["dataset"]}@{BACKUP_NAME_PREFIX}']
-            ], {'select': ['name']}
+        ds = self.middleware.call_sync(
+            'zfs.resource.query_impl',
+            {'paths': [docker_config['dataset']], 'properties': None, 'get_snapshots': True}
         )
-        for snapshot in snapshots:
-            backup_name = snapshot['name'].split('@', 1)[-1].split(BACKUP_NAME_PREFIX, 1)[-1]
+        if not ds:
+            return backups
+        elif not ds[0]["snapshots"]:
+            return backups
+
+        prefix = f'{docker_config["dataset"]}@{BACKUP_NAME_PREFIX}'
+        for snap_name, snap_info in ds[0]['snapshots'].items():
+            if not snap_name.startswith(prefix):
+                continue
+
+            backup_name = snap_name.split('@', 1)[-1].split(BACKUP_NAME_PREFIX, 1)[-1]
             backup_path = os.path.join(backups_base_dir, backup_name)
             if not os.path.exists(backup_path):
                 continue
@@ -115,10 +124,13 @@ def list_backups(self):
             backups[backup_name] = {
                 'name': backup_name,
                 'apps': [{k: app[k] for k in ('id', 'name', 'state')} for app in apps.values()],
-                'snapshot_name': snapshot['name'],
-                'created_on': str(self.middleware.call_sync(
-                    'zfs.snapshot.get_instance', snapshot['name']
-                )['properties']['creation']['parsed']),
+                'snapshot_name': snap_name,
+                'created_on': str(
+                    datetime.datetime.fromtimestamp(
+                        snap_info["properties"]["creation"]["value"],
+                        datetime.UTC
+                    )
+                ),
                 'backup_path': backup_path,
             }
 
@@ -168,7 +180,7 @@ async def post_system_update_hook(middleware):
                 break
 
     backup_job = await middleware.call(
-        'docker.backup', f'{UPDATE_BACKUP_PREFIX}-{datetime.now().strftime("%F_%T")}'
+        'docker.backup', f'{UPDATE_BACKUP_PREFIX}-{datetime.datetime.now().strftime("%F_%T")}'
     )
     await backup_job.wait()
     if backup_job.error:

src/middlewared/middlewared/plugins/docker/migrate.py

@@ -84,5 +84,5 @@ async def replicate_apps_dataset(self, new_pool, old_pool, migration_options):
         finally:
             await self.middleware.call('zfs.snapshot.delete', snap_details['id'], {'recursive': True})
             snap_name = f'{applications_ds_name(new_pool)}@{snap_details["snapshot_name"]}'
-            if await self.middleware.call('zfs.snapshot.query', [['id', '=', snap_name]]):
+            if await self.middleware.call('zfs.resource.snapshot_exists', snap_name):
                 await self.middleware.call('zfs.snapshot.delete', snap_name, {'recursive': True})

src/middlewared/middlewared/plugins/vm/clone.py

@@ -31,16 +31,18 @@ async def __next_clone_name(self, name):
         return clone_name
 
     async def __clone_zvol(self, name, zvol, created_snaps, created_clones):
-        if not await self.middleware.call(
-            'zfs.resource.query_impl', {'paths': [zvol], 'properties': None}
-        ):
+        zz = await self.middleware.call(
+            'zfs.resource.query_impl',
+            {'paths': [zvol], 'properties': None, 'get_snapshots': True}
+        )
+        if not zz:
             raise CallError(f'zvol {zvol} does not exist.', errno.ENOENT)
 
         snapshot_name = name
         i = 0
         while True:
             zvol_snapshot = f'{zvol}@{snapshot_name}'
-            if await self.middleware.call('zfs.snapshot.query', [('id', '=', zvol_snapshot)]):
+            if zvol_snapshot in zz[0]['snapshots']:
                 if ZVOL_CLONE_RE.search(snapshot_name):
                     snapshot_name = ZVOL_CLONE_RE.sub(rf'\1{ZVOL_CLONE_SUFFIX}{i}', snapshot_name)
                 else:

src/middlewared/middlewared/plugins/zfs/resource_crud.py

@@ -129,6 +129,18 @@ def query_impl(self, tls, data: dict | None = None):
         else:
             return results
 
+    @private
+    def snapshot_exists(self, snap_name: str):
+        """Check to see if a given snapshot exists.
+        NOTE: internal method so lots of assumptions
+        are made by the passed in `snap_name` arg."""
+        ds, snap_name = snap_name.split("@")
+        rv = self.middleware.call_sync(
+            "zfs.resource.query_impl",
+            {"paths": [ds], "properties": None, "get_snapshots": True}
+        )
+        return rv and snap_name in rv[0]["snapshots"]
+
     @api_method(
         ZFSResourceQueryArgs,
         ZFSResourceQueryResult,

tests/api2/test_account_privilege_role.py

@@ -17,7 +17,11 @@ def test_can_read_with_read_or_write_role(role):
     with dataset("test_snapshot_read") as ds:
         with snapshot(ds, "test"):
             with unprivileged_user_client([role]) as c:
-                assert len(c.call("pool.snapshot.query", [["dataset", "=", ds]])) == 1
+                snaps = c.call(
+                    "zfs.resource.query",
+                    {"paths": [ds], "properties": None, "get_snapshots": True}
+                )[0]["snapshots"]
+                assert len(snaps) == 1
 
 
 def test_can_not_write_with_read_role():

tests/api2/test_cloud_backup.py

@@ -451,7 +451,11 @@ def test_snapshot(s3_credential):
                 ]) == ["blob", "blob2"]
 
         time.sleep(1)
-        assert call("pool.snapshot.query", [["dataset", "=", ds]]) == []
+        snaps = call(
+            "zfs.resource.query",
+            {"paths": [ds], "properties": None, "get_snapshots": True}
+        )[0]["snapshots"]
+        assert len(snaps) == 0
 
 
 @pytest.mark.parametrize("cloud_backup_task, expected", [(

tests/api2/test_cloud_sync.py

@@ -120,7 +120,11 @@ def test_snapshot(has_zvol_sibling):
 
             time.sleep(1)
 
-            assert call("pool.snapshot.query", [["dataset", "=", ds]]) == []
+            snaps = call(
+                "zfs.resource.query",
+                {"paths": [ds], "properties": None, "get_snapshots": True}
+            )[0]["snapshots"]
+            assert len(snaps) == 0
         finally:
             if has_zvol_sibling:
                 ssh(f"zfs destroy -r {pool}/zvol")