gadget: make reinstalls honor existing sizes of partitions #15086
Conversation
When performing a re-install, snapd uses "size" from the gadget when re-creating partitions that it just removed. However, in the case of using min-size there is a possibility that the deleted partition size was smaller but still compatible with the installed gadget. Use the size of the deleted partition, as we want to respect the already existing layout - otherewise we might destroy partitions that are not re-created in the re-install and that could appear after the partitions using min-size.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15086 +/- ##
==========================================
- Coverage 78.07% 77.85% -0.22%
==========================================
Files 1182 1156 -26
Lines 157743 156658 -1085
==========================================
- Hits 123154 121967 -1187
- Misses 26943 27110 +167
+ Partials 7646 7581 -65
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There is an unrelated problem if we do not respect the layout, which is that the partition probing does not seem to be working correctly and the newly created partitions are not detected and the installation stops. I have not digged yet into that, mainly because I think that the correct behavior is to respect the layout, on one side because of the possibility of overlaps, but we could also have the problem of trying to create partitions bigger than the available space. If we want to do something else we need additional checks and in general all the approach seems risky to me. But we can think about this for the future. |
alfonsosanchezbeato
added
the
Run nested
it would be good to understand the unrelated problem even if we keep the conservative strategy fix for now. |
|
Fri Feb 14 21:02:10 UTC 2025 Failures:Preparing:
Executing:
Restoring:
|
I've added comments about this now. I think that an additional consideration is that if there is an installer (say, muinstaller for UC) the installer can decide on any size in a |
| @@ -70,7 +70,7 @@ type CreateOptions struct { | |||
| // OnDiskStructure, as it is meant to be used externally (i.e. by | |||
| // muinstaller). | |||
| func CreateMissingPartitions(dv *gadget.OnDiskVolume, gv *gadget.Volume, opts *CreateOptions) ([]*gadget.OnDiskAndGadgetStructurePair, error) { | |||
| dgpairs, err := createMissingPartitions(dv, gv, opts) | |||
| dgpairs, err := createMissingPartitions(dv, gv, opts, map[int]StructOffsetSize{}) | |||
There was a problem hiding this comment.
as we don't write to it this can be nil afaict
| if opts == nil { | ||
| opts = &CreateOptions{} | ||
| } | ||
| if deletedOffsetSize == nil { | ||
| deletedOffsetSize = map[int]StructOffsetSize{} |
| deletedIndexes := make(map[int]bool, 3) | ||
| deletedOffsetSize := make(map[int]StructOffsetSize, 3) | ||
| for i, s := range dl.Structure { | ||
| if wasCreatedDuringInstall(gv, s) { | ||
| yamlIdx := indexIfCreatedDuringInstall(gv, s) | ||
| if yamlIdx >= 0 { | ||
| logger.Noticef("partition %s was created during previous install", s.Node) | ||
| sfdiskIndexes = append(sfdiskIndexes, strconv.Itoa(i+1)) | ||
| deletedOffsetSize[yamlIdx] = StructOffsetSize{ | ||
| StartOffset: s.StartOffset, | ||
| Size: s.Size, | ||
| } | ||
| deletedIndexes[i] = true | ||
| } | ||
| } | ||
| if len(sfdiskIndexes) == 0 { | ||
| return nil | ||
| return deletedOffsetSize, nil | ||
| } |
There was a problem hiding this comment.
This entire setup is very confusing. We are using 3 different indices, and there are absoultely no description as to why each one is used.
Would it make more sense for YamlIndex to just be a part of the OnDiskStructure (i.e dl.Structure members in this case) - so yamlIndex wouldn't have to be returned from indexIfCreatedDuringInstall, and we could avoid that change?
Also from a readability perspective this change to removeCreatedPartitions that it returns a seemingly unrelated map of StructOffsetSize, without any documentation as to why this is returned seems like a menace.
To me, it seems the information about which partitions that should be removed, should be encapsulated in it's own function, return one/list of structures with all information removeCreatedPartitions and whatever deletedOffsetSize has to perform the delete and then keep that function simple.
When performing a re-install, snapd uses "size" from the gadget when re-creating partitions that it just removed. However, in the case of using min-size there is a possibility that the deleted partition size was smaller but still compatible with the installed gadget. Use the size of the deleted partition, as we want to respect the already existing layout - otherewise we might destroy partitions that are not re-created in the re-install and that could appear after the partitions using min-size.