NAS-143717 / 27.0.0-BETA.1 / Convert the zfs.resource services to the typesafe pattern - #19698
Merged
Merged
Conversation
bugclerk
changed the title
Contributor
yocalebo
requested changes
Sep 14, 2026
themylogin
reviewed
Sep 14, 2026
yocalebo
approved these changes
Sep 14, 2026
## Context
`zfs.resource`, `zfs.resource.snapshot` and `zfs.resource.pool` were already half-converted: public methods carried `@api_method(check_annotations=True)` against models in `api/v27_0_0/`, and most in-process consumers already went through `call2`. What was missing was the structural half - the service classes still held the implementation. `resource_crud.py` and `snapshot_crud.py` were 800+ lines each, mixing a 117-line create pipeline, destroy validation, path helpers and per-method exception translation in with the API surface.
## Solution
**Lean shims delegating to module functions.** The three services keep only their `Config`, decorators and docstrings, and hand off to plain functions taking a `ServiceContext` - the same shape `ports`, `hardware` and `boot` already use. The logic lands in `resource_{query,create,destroy,ops}.py`, `snapshot_ops.py` and `prefetch_ops.py`. `create_rules.py`'s three pool-inspecting helpers now take a `ServiceContext` rather than an untyped `service`, and while touching that file it picks up `from __future__ import annotations` so its annotations stop being quoted strings.
**Each service sits in a module named after its namespace** - `resource.py`, `snapshot.py`, `prefetch.py`, `tier.py` - and `plugins/zfs/__init__.py` stays a docstring. That is deliberate and worth keeping: around 30 modules elsewhere import cheap leaves of this package (`exceptions`, `zvol_utils`, `utils`, `encryption`), and `test/integration/assets/pool.py` - which 173 test files pull in, on the client side - is one of them. With the service in `__init__.py`, `from middlewared.plugins.zfs.exceptions import ZFSPathNotFoundException` went from a `typing` import to 1438 modules including `middlewared.service` and the `truenas_pylibzfs` C extension, which `tests/requirements.txt` does not install. It is now 145 and pulls neither. `plugins/zpool/` already splits this way for the same reason.
**`zfs.resource.pool` becomes a sub-service** of `zfs.resource` instead of only being picked up by the plugin loader, which gives it a `self.s` path. That was the last thing forcing untyped string calls into this namespace - `pool.dedup`, the failover event handler and the `pool.post_import` hook all use `call2` now, and no string call to `zfs.resource*` survives outside the over-the-wire tests.
**`special_vdev_thresholds` moves to `utils/zfs/tier.py`.** It is pure arithmetic over two config fields and never belonged behind a plugin; `alert.source.zfs_tier` was importing it from `plugins.zfs.tier`, and alert sources are imported while `middlewared.service` is still initialising, so that pulled a plugin package in before `Service` was bound. Worth knowing that nothing static catches this class of breakage - mypy, lint, ruff and the whole unit suite were green on a tree that could not start.
`GenericCRUDService` is not an option here - its metaclass forces `filters`/`options` onto `query`, and `zfs.resource.query` takes a single request model. The `*_impl` methods also still return dicts, so nothing outside `plugins/zfs/` changes how it reads a result. Public wire shapes are identical, checked by replaying the full API surface on a test VM and diffing against the same run before the change.
Qubad786
force-pushed
the
mrehan/typesafe-resource
branch
from
September 15, 2026 02:24
c57e711 to
a3b4ae4
Compare
Contributor
|
This PR has been merged and conversations have been locked. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
zfs.resource,zfs.resource.snapshotandzfs.resource.poolwere already half-converted: public methods carried@api_method(check_annotations=True)against models inapi/v27_0_0/, and most in-process consumers already went throughcall2. What was missing was the structural half - the service classes still held the implementation.resource_crud.pyandsnapshot_crud.pywere 800+ lines each, mixing a 117-line create pipeline, destroy validation, path helpers and per-method exception translation in with the API surface.Solution
Lean shims delegating to module functions. The three services keep only their
Config, decorators and docstrings, and hand off to plain functions taking aServiceContext- the same shapeports,hardwareandbootalready use. The logic lands inresource_{query,create,destroy,ops}.py,snapshot_ops.pyandprefetch_ops.py.create_rules.py's three pool-inspecting helpers now take aServiceContextrather than an untypedservice, and while touching that file it picks upfrom __future__ import annotationsso its annotations stop being quoted strings.Each service sits in a module named after its namespace -
resource.py,snapshot.py,prefetch.py,tier.py- andplugins/zfs/__init__.pystays a docstring. That is deliberate and worth keeping: around 30 modules elsewhere import cheap leaves of this package (exceptions,zvol_utils,utils,encryption), andtest/integration/assets/pool.py- which 173 test files pull in, on the client side - is one of them. With the service in__init__.py,from middlewared.plugins.zfs.exceptions import ZFSPathNotFoundExceptionwent from atypingimport to 1438 modules includingmiddlewared.serviceand thetruenas_pylibzfsC extension, whichtests/requirements.txtdoes not install. It is now 145 and pulls neither.plugins/zpool/already splits this way for the same reason.zfs.resource.poolbecomes a sub-service ofzfs.resourceinstead of only being picked up by the plugin loader, which gives it aself.spath. That was the last thing forcing untyped string calls into this namespace -pool.dedup, the failover event handler and thepool.post_importhook all usecall2now, and no string call tozfs.resource*survives outside the over-the-wire tests.