NAS-143371 / 27.0.0-BETA.1 / Return snapshot user properties from pool.snapshot.query

[eschultz]

NAS-143371

pool.snapshot.query cannot return a snapshot's user properties. Naming one in extra.properties returns nothing and reports no error, so a caller cannot tell "that property is not set" from "this endpoint will never give it to you".

pool.dataset.query returns dataset user properties. pool.snapshot.create accepts them by name. The query side of the snapshot API was the gap.

Cause

query() puts everything from extra.properties into query_args['properties']. That reaches __build_snapshot_cache, which resolves each name with ZFSProperty[i.upper()] inside a try whose except KeyError is continue. A user property name is not an enum member, so it is dropped there, silently.

extra.retention is not a way round it — it sets get_user_properties internally, but zettarepl.annotate_snapshots pops the whole dict and keeps one removal-date key.

Fix

A ZFS user property is exactly a name containing a colon, the same rule check_user_property_names uses in plugins/zfs/create_rules.py. query() splits those out, asks for them with get_user_properties, and _transform_snapshot_entry merges the ones the caller named into the properties map the entry already carries. No API model change — PoolSnapshotEntry.properties is dict[str, PoolSnapshotEntryPropertyFields], so the keys are open.

ZFS reports no source for a user property, so the merged entry says LOCAL. That is the same compromise normalize_user_properties() already makes for pool.dataset.query, and its docstring says so. Anything else would make the two endpoints disagree.

Verified on 26.0.0-BETA.2

• a user property on its own comes back with source LOCAL and the right value
• mixed with creation, both come back; creation still source NONE
• a user property that is not set is simply absent, not an error
• a native-only request is byte for byte what it was
• extra.retention and extra.holds in the same call still work

Three tests added to tests/api2/test_snapshot_query.py.

One thing deliberately not patched

The same silent drop also swallows real ZFS properties that are not valid on a snapshot — __build_snapshot_cache resolves the name then applies if prop in valid_props with no else. So extra.properties: ["mountpoint", "quota", "creation"] returns only creation, and about fifty names behave that way.

I left it alone: it is a separate decision, and unlike the user-property case there is no correct value to return, so the answer is either an error or nothing. The same except KeyError: continue shape is also in pool.dataset.query's extra.properties and extra.snapshots_properties, so whatever is decided should probably cover all of them. Happy to open a second ticket.

Note on shape

I first wrote this as a validation error rejecting any name that is not a ZFS property. Review talked me out of it. Rejecting would be a breaking change to a method with no internal callers, so the whole cost lands on external clients, with no API version to pin against because extra is untyped; it would also take down the response entirely for a caller who asked for holds and retention in the same call and previously got both.

Applies to stable/26 unchanged. stable/goldeye has a different query implementation and would need its own patch.

[anodos325]

The asymmetry is due to differences in cost between snapshot retrieval and dataset retrieval N is much higher here potentially by several orders of magnitude. I don't see a reason to add this honestly unless you have some part of our UI that is broken by it.

[eschultz]

You're right that N is much larger for snapshots, and that's the correct reason for the asymmetry. But the patch is opt-in, and I measured the cost rather than assuming it, because I'd made the same guess you did.

get_user_properties is set only when the caller explicitly names a property containing a colon in extra.properties. A caller who doesn't ask pays nothing — no change to any existing query.

On 26.0.0-BETA.3, 5000 snapshots under one dataset, each carrying 2 user properties, median of 5 runs:

zfs.resource.snapshot.query
  no properties                        357 ms
  properties=[creation]               1232 ms
  get_user_properties=true            1039 ms
  creation + get_user_properties      1255 ms

Two things there surprised me. Fetching user properties is cheaper than asking for a single native property (1039 vs 1232 ms), and once any property is already being fetched, adding user properties costs about 2% — 1255 vs 1232 ms across 5000 snapshots.

The other half is that this path already ships:

pool.snapshot.query (unpatched)
  plain                                374 ms
  extra.properties=[creation]         1087 ms
  extra.retention=true                1046 ms

extra.retention already sets get_user_properties=True for the entire result set — it's how the removal date is read — and then zettarepl.annotate_snapshots pops the dict and keeps one key. So middleware is already paying this exact cost on a path any caller can hit today; the patch reuses that fetch instead of adding a new one.

On your actual question: no, I don't have a broken UI. I should have led with that. The driver was that the failure is silent — extra.properties: ["com.example:one"] returns properties: {} with no error, so a caller can't tell an unset property from one this endpoint will never return. That's what I'd like fixed; returning the value was just the option that seemed most useful.

If the cost is still not worth it to you, I'd rather have the smaller change: reject a name that can't be returned, or document it, so the drop stops being silent. Happy to redo the PR that way, or to close it if you'd prefer to leave the endpoint as it is. Either is fine by me.