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% 79.04% +0.09%
==========================================
Files 1084 1087 +3
Lines 146638 148057 +1419
==========================================
+ Hits 115773 117034 +1261
- Misses 23667 23777 +110
- Partials 7198 7246 +48
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 |
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
| 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.
There was a problem hiding this comment.
Such use case feel very finicky and is basically asking for trouble in the long run. If we do this it would require visibility to make sure that the thing that matched by accident is what was intended.
There was a problem hiding this comment.
There are other cases I can think about, for instance:
- vendor creates a SKU with one disk, deploys it around
- vendor makes a modification of the design and adds a disk. They need then different assignments for the 2 SKUs and want to actually use the same gadget/image. The first SKU would match the 2 possible assignements, so not allowing that would produce an error.
Again, I think that the intention of the spec is to allow this sort of scenario, it is not a match by accident, it is that I do not see crazy that there is some intersection between assignements and to me what spec is saying is that because of that we need to use the order in the list as priority.
@kubiko in your experience in the field, does this make sense?
There was a problem hiding this comment.
Hmm tricky one @alfonsosanchezbeato
It is common to have multiple sources for components like eMMC. So if we have descriptive laguage which pins it down as /dev/disk/by-path/pci-0000:02:00.1-ata-5 then having multiple options woud make sense.
But.... Otherside of the coin is that /dev/disk/by-path/xxxxx in still symbolic link to something which is more predictable e.g. /dev/mmcblk0p1. And this needs to remain the same for all the different sources. Otherwise when mmc device enumeration changes, u-boot mmc device number would change as well and we would need new u-boot build per SKU. (sure one can add some abiguity to u-boot scrip, but that on it's own is wrong approach)
So I see @alfonsosanchezbeato point, but we are potentially overthinking it. IMHO in practive ppl will use /dev/mmcblk0p1 rather than /dev/disk/by-path/
I checked also device with UFS storage, and there we know it's /dev/sda, /dev/sdb,.....
Also alternatice sources would be still plugged to the same interface, so they would likely still show as the same by-path anyway.
There was a problem hiding this comment.
The spec is actually not super clear on this point, the intention is that there we pick the first matching assignments (all nodes can be resolved), that is true, but is not explicit about ambiguity. I think for now if there are many matching assignments in theory we should produce an error only if they really result in different assignments, that means really ambiguity, this assumes we can find out if different device paths actually point to the same device?
There was a problem hiding this comment.
@kubiko note that here we allow only by-path and by-id as those identifiers are the only ones that could not change across reboots if there are disks of the same type.
@pedronis that's a good point, but taking into account that we use only by-path and by-id, in practice it would happen only if there is an assignement using by-id and a different one using by-path that would point to the same device, which would be strange as you would not want to define one using by-id in that case possibly.
Anyway, I am fine with current approach if there is no traction in what I suggest, as it is more restrictive so it can be made more flexible later.
|
Discussed in standup, pedronis doesn't have any issues with merging this. |
Meulengracht
force-pushed
the
feature/gadget-volume-assignment
branch
3 times, most recently
from
ef807c5
to
4c49013
Compare
…, 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
… Improve the doc strings based on suggestions from Maciej. Error out if multiple device assigments match the current device.
…inside `volume-assignments`.
…or secondary volume
Meulengracht
force-pushed
the
feature/gadget-volume-assignment
branch
from
4c49013
to
661b8db
Compare
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