gadget,tests: add support for volume-assignments in gadget #14563
Conversation
github-actions
bot
added
the
Run Nested -auto-
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14563 +/- ##
==========================================
+ Coverage 78.95% 78.96% +0.01%
==========================================
Files 1084 1085 +1
Lines 146638 147263 +625
==========================================
+ Hits 115773 116283 +510
- Misses 23667 23739 +72
- Partials 7198 7241 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Meulengracht
force-pushed
the
feature/gadget-volume-assignment
branch
2 times, most recently
from
70df3d1
to
a5c5e3f
Compare
|
needs a rebase |
…, this will allow the gadget yaml to specify specific disk mappings when either installing or updating the system
…l code used to identify devices for volumes
…to account, minor code improvements
Meulengracht
force-pushed
the
feature/gadget-volume-assignment
branch
from
078585f
to
a61b6a3
Compare
| switch { | ||
| case len(common) != len(newVolumes) && len(common) != len(oldVolumes): | ||
| // there are both volumes removed from old and volumes added to new | ||
| return fmt.Errorf("cannot update gadget assets: volumes were both added and removed") | ||
| case len(common) != len(newVolumes): | ||
| // then there are volumes in old that are not in new, i.e. a volume | ||
| // was removed | ||
| return fmt.Errorf("cannot update gadget assets: volumes were removed") | ||
| case len(common) != len(oldVolumes): | ||
| // then there are volumes in new that are not in old, i.e. a volume | ||
| // was added | ||
| return fmt.Errorf("cannot update gadget assets: volumes were added") | ||
| } |
There was a problem hiding this comment.
maybe we could use it as an opportunity to simplify, but since it's just moving preexeisting code it's ok to leave it as it is:
| switch { | |
| case len(common) != len(newVolumes) && len(common) != len(oldVolumes): | |
| // there are both volumes removed from old and volumes added to new | |
| return fmt.Errorf("cannot update gadget assets: volumes were both added and removed") | |
| case len(common) != len(newVolumes): | |
| // then there are volumes in old that are not in new, i.e. a volume | |
| // was removed | |
| return fmt.Errorf("cannot update gadget assets: volumes were removed") | |
| case len(common) != len(oldVolumes): | |
| // then there are volumes in new that are not in old, i.e. a volume | |
| // was added | |
| return fmt.Errorf("cannot update gadget assets: volumes were added") | |
| } | |
| if len(common) != len(newVolumes) || len(common) != len(oldVolumes) { | |
| // there are both volumes removed from old and volumes added to new | |
| return fmt.Errorf("cannot update gadget assets: differing volumes in new and old gadget") | |
| } |
There was a problem hiding this comment.
i left this as an exercise for a separate PR
… Improve the doc strings based on suggestions from Maciej. Error out if multiple device assigments match the current device.
…inside `volume-assignments`.
| @@ -343,10 +376,15 @@ func Run(model gadget.Model, gadgetRoot string, kernelSnapInfo *KernelSnapInfo, | |||
| return nil, err | |||
| } | |||
|
|
|||
| volumes, err := determineDeviceVolumes(info) | |||
| if err != nil { | |||
| return nil, err | |||
gadget/install/install.go
Outdated
| @@ -324,6 +339,24 @@ func onDiskStructsSortedIdx(vss map[int]*gadget.OnDiskStructure) []int { | |||
| return yamlIdxSl | |||
| } | |||
|
|
|||
| func determineDeviceVolumes(gi *gadget.Info) (map[string]*gadget.Volume, error) { | |||
| if len(gi.VolumeAssignments) != 0 { | |||
| devVols, err := gadget.VolumesForCurrentDeviceAssignment(gi) | |||
There was a problem hiding this comment.
what if we changed the signature of the function a little such that we could have:
| devVols, err := gadget.VolumesForCurrentDeviceAssignment(gi) | |
| vols, explicitlyAssigned, err := gadget.VolumesForCurrentDevice(gi) | |
| if explicitlyAssigned { | |
| // log | |
| } |
were (vols, explicitlyAssigned) is (assigned volumes, true) if there was a match, otherwise it's simply (gi.Volumes, false) ?
There was a problem hiding this comment.
I did something similar to this
…or secondary volume
| default: | ||
| return nil, fmt.Errorf("multiple matching volume-assignment for current device") |
There was a problem hiding this comment.
We should not error in this case, again, according to the spec we return the first match, but it does not say to error out if there is more than one match. I can see cases where allowing multiple matches would be useful, for instance a first assignment A that uses by-id and an assignment B that uses by-path, because the user has an special assignment A for a concrete unit but the rest use B, and in that case the special unit would match both assignments, so the order matters. And if we add more fields to decide on assignments this will start to make much more sense. In summary, I foresee vendors wanting to use this flexibility in the future.
This will allow the gadget yaml to specify specific disk mappings when either installing or updating the system.
The specification relevant to this feature is here: docs
JIRA: SNAPDENG-32507