docs/api: fix system details schema - #17696
raimundo-henriques wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The documented unavailable-reason requirement is not guaranteed by the daemon implementation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Aligns system details OpenAPI schemas with snapd’s wire types.
Changes:
- Corrects encryption and optional-component schemas.
- Adds reusable gadget-volume definitions.
- Updates install volume maps and response formatting.
| File | Description |
|---|---|
SystemDetails.yaml |
Corrects system details fields. |
GadgetVolume.yaml |
Defines gadget volume structures. |
SystemActionInstall.yaml |
Uses gadget-volume maps. |
systems-label.yaml |
Standardizes response definitions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| The reason why storage encryption is unavailable or defective. When | ||
| a single availability check error is detected, unavailable-reason reports | ||
| the error message; when multiple errors are detected, it reports the number of | ||
| errors (e.g. 'availability check identified 2 errors'). Must be populated if | ||
| availability-check-errors is non-empty. |
There was a problem hiding this comment.
@ZeyadYasser Is this surfacing an issue with the API or is it indeed the spec that is currently too stringent? Thanks
There was a problem hiding this comment.
@valentindavid I don't think this is an issue since the unavailable-reason is mostly for human consumption, wdyt?
There was a problem hiding this comment.
My understanding was that UnavailableErr is about unexpected error while checking the system. And AvailabilityCheckErrors is the error it found on the system. That is if it succeeded to find errors on the system, it will have UnavailableErr being nil, but AvailabilityCheckErrors will have the system issues. If UnavailableErr is set, it means the pre install checks failed to check the system, so the system might be OK, but something else happened.
There was a problem hiding this comment.
Thanks @valentindavid . If I get this right, then, the description should probably be more specific. Would the following perhaps suffice?
| The reason why storage encryption is unavailable or defective. When | |
| a single availability check error is detected, unavailable-reason reports | |
| the error message; when multiple errors are detected, it reports the number of | |
| errors (e.g. 'availability check identified 2 errors'). Must be populated if | |
| availability-check-errors is non-empty. | |
| The reason why storage encryption is unavailable or defective. When | |
| a single availability check error is detected, unavailable-reason reports | |
| the error message; when multiple errors are detected, it reports the number of | |
| errors (e.g. 'availability check identified 2 errors'). During pre-install, it | |
| must be populated if availability-check-errors is non-empty. |
There was a problem hiding this comment.
That sounds like the same... availability-check-errors can be empty the errors are not from the hardware, but from snapd.
We should get confirmation from @ernestl that is the correct thing. Because he worked on it.
There was a problem hiding this comment.
I thought this could only happen post-install and hence that it mapped onto the pre-install post-install distinction. Thanks!
There was a problem hiding this comment.
The spec description of the intended behavior is correct:
If this list is non-empty, encryption is considered unavailable, and the “unavailable-reason” field must be populated. In the case of a single error, “unavailable-reason” should match the error message, and for multiple errors it should provide an umbrella error message.
This is also why, regardless of the model grade / storage-safety - which results in warning or error in the install.EncryptionSupportInfo, both error and warnings are translated into client.StorageEncryption.UnavailableReason.
However, note that even if there are no secboot availability check error, that does not guarantee empty client.StorageEncryption.UnavailableReason - there may e.g. still be issue with the gadget.
Copilot is correct in pointing out that in the [later post-install check] path (https://github.com/canonical/snapd/blame/master/overlord/devicestate/devicemgr.go#L2901-L2911
) is indeed inconsistent. @valentindavid, I suppose we have to discuss?
I see it is used by used by GET /v2/systems?running=true and by fix-encryption-support when no system label is given.
But the pre-install implementation do follow the spec.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17696 +/- ##
========================================
Coverage 78.38% 78.39%
========================================
Files 1418 1417 -1
Lines 200785 200586 -199
Branches 2503 2503
========================================
- Hits 157385 157240 -145
+ Misses 33968 33927 -41
+ Partials 9432 9419 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thu Sep 24 17:29:24 UTC 2026 Spread tests skipped |
92bc7cc to
0474e33
Compare
Fix avaialability-check-errors, previously described as an array of strings. The new description follows the type used by the API and refers to the secboot library. Fix the storage-encryption schema on the system details: - the field describing the encryption type was wrongly named 'type' and was now updated to the correct 'encryption-type'; - the conditional requirement of unavailable-reason (required when availability-check-errors is non-empty) was not documented before. The volumes field of the system details was wrongly described as an array of objects with three keys. In reality, it is a map of gadget volumes keyed by volume name. Since it is also used in the on-volumes field of the install action, a GadgetVolume schema was abstracted and used for both. It describes the full gadget volume mapping the API. The available-optional field of system details was also fixed. It was wrongly described as an array of strings and is now a map of snap names to lists of available component names.
0474e33 to
e40256a
Compare
| availability-check-errors is non-empty. | ||
| availability-check-errors: | ||
| type: array | ||
| description: A list of errors detected during the preinstall check, as determined by the secboot library. |
There was a problem hiding this comment.
Striving for consistency regarding the hyphenation of 'pre-install'.
| description: A list of errors detected during the preinstall check, as determined by the secboot library. | |
| description: A list of errors detected during the pre-install check, as determined by the secboot library. |
Rnfudge02
left a comment
There was a problem hiding this comment.
A few questions about the GadgetVolume struct, otherwise its looking go to me!
| description: >- | ||
| A map of the volumes to operate on, keyed by volume name. | ||
| additionalProperties: | ||
| $ref: 'GadgetVolume.yaml' |
There was a problem hiding this comment.
When dealing with the API, do we pass the whole GadgetVolume struct in the request? Or do we just pass the key to the GadgetVolume we are looking to interact with?
Also, do we only pass in one GadgetVolume at a time? If we can pass more than one this should probably still be an array where the items are of type object?
There was a problem hiding this comment.
If I understand correctly, we pass the whole GadgetVolume and more than one, though not as an array. It's a map, keyed by strings (e.g., "pc"):
OnVolumes map[string]*gadget.Volume(From here)
Mention that order by which the actions to fix pre-install errors is relevant. This is important to tie together SystemDetails and the fix-encryption-support action.
Problem
As mentioned in a Copilot comment to PR #17673 , there were three problems with system details:
encryption-typeofstorage-encryptionwas wrongly namedtype;availability-check-errorswas described as an array of strings and not an array of objects with the error details.volumeswas an array of objects with three keys instead of a map of volume names to full gadget volumes; andavailable-optionalwas an array of strings and not a map of snap names to a list of components.Solution
This PR fixes these three problems.
Problem 3. required a rather substantial change. Since gadget volumes are also used on the
on-volumesfield of action install, a GadgetVolume schema was abstracted and used for both of them.As a small improvement, this PR also includes an update to the description of
unavailable-reason, stating that it must be present ifavailability-check-errorsis non-empty. Enforcing the conditional requirement is unfortunately dependent on updating OpenAPI to v3.1.The removal of single quotation marks around status codes improves consistency and overlaps with #17673 .
SNAPDENG-36197