gadget, cmd/snap-bootstrap: MBR schema support #8487
Conversation
We won't have by-partlabel on MBR disks, so we need to fallback to using by-label here instead. This is probably a bit of a hack, but unclear what the right thing to do here is... Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
This isn't ideal but gets ups a wee bit closer to successful partitioning. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
…ture name This happens for example on an MBR disk, where we don't have partition names it seems, and only have partition labels. In these situations, we should use the partition label as the name. Also upgrade type "c" as reported by sfdisk to "0C" which is what is expected by the gadget. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
|
This is stacked on top of #8483, which is of course not of questionably quality, but is necessary all the same :-) |
| // sometimes sfdisk reports what is "0C" in gadget.yaml as "c", so fix | ||
| // that up now | ||
| // TODO:UC20: what's a better way? | ||
| if structure[i].Type == "c" { |
There was a problem hiding this comment.
If this needs to be normalized it would be better to normalize any MBR type to the %02X format instead of just 0C.
There was a problem hiding this comment.
IIRC the parsers for gadget.yaml in snapd/gadget and in ubuntu-image expect [0-9A-F]{2}, so normalizing sounds like a good idea.
| structure[i].Type = "0C" | ||
| } | ||
|
|
||
| // sometimes sfdisk reports no "Name" for partitions, so in this case |
There was a problem hiding this comment.
Unfortunately MBR partition entries have no name field so the label is all we have.
|
Also MBR doesn't have user-defined partition attributes so it will be harder to mark partitions created at install time (maybe a bit in the partition status byte? custom partition type?) |
| // sometimes sfdisk reports what is "0C" in gadget.yaml as "c", so fix | ||
| // that up now | ||
| // TODO:UC20: what's a better way? | ||
| if structure[i].Type == "c" { |
There was a problem hiding this comment.
IIRC the parsers for gadget.yaml in snapd/gadget and in ubuntu-image expect [0-9A-F]{2}, so normalizing sounds like a good idea.
gadget/device_linux.go
Outdated
| if ps.EffectiveSchema() == GPT { | ||
| byPartlabel = filepath.Join(dirs.GlobalRootDir, "/dev/disk/by-partlabel/", encodeLabel(ps.Name)) | ||
| } else { | ||
| byPartlabel = filepath.Join(dirs.GlobalRootDir, "/dev/disk/by-label/", encodeLabel(ps.Name)) |
There was a problem hiding this comment.
I think this is misplaced here. Looked at ubuntu-image again. The logic there uses partition name as a fallback when no filesystem label is provided regardless of GPT/MBR. Perhaps we should have the following:
if ps.Name != "" {
byPartlabel := filepath.Join(dirs.GlobalRootDir, "/dev/disk/by-partlabel/", encodeLabel(ps.Name))
candidates = append(candidates, byPartlabel)
}
if ps.HasFilesystem() {
fsLabel := ps.EffectiveFilesystemLabel()
if fsLabel == "" && ps.Name != "" {
fsLabel = ps.Name
}
byFsLabel := filepath.Join(dirs.GlobalRootDir, "/dev/disk/by-label/", encodeLabel(fsLabel))
candidates = append(candidates, byFsLabel)
}
and there's no longer a need to have VolumeStructure.EffectiveSchema() anymore
There was a problem hiding this comment.
yes this sounds like the proper way to do this
…-better-plz-kthxbai
| if ptable.LastLBA != 0 { | ||
| numSectors = gadget.Size(ptable.LastLBA + 1) | ||
| } else { | ||
| out, err := exec.Command("blockdev", "--getsz", ptable.Device).CombinedOutput() |
There was a problem hiding this comment.
Replaced with reading the sector size directly from /sys/block/$(basename dev)/size
There was a problem hiding this comment.
Actually rolled that back, and just wrapped the call to blockdev nicely.
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
…name When a structure has a filesystem, but no label, the structure's name will be used as a label by default. Encode this property in device lookup. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
| // sometimes sfdisk reports no "Name" for partitions, so in this case | ||
| // if we also have a label for the partition from lsblk, we should | ||
| // fallback to that instead | ||
| if structure[i].Name == "" && structure[i].Label != "" { | ||
| structure[i].Name = structure[i].Label | ||
| } |
There was a problem hiding this comment.
@bboozzoo this part is still necessary, as-is with the branch we don't have compatible layouts for the disk + gadget because the ensureLayoutCompatibility check only compares dv.Name == gv.Name, and without this snippet, the dv will not have a Name.
There was a problem hiding this comment.
I see. Hm that should be unnecessary though. The check in bootstrap.go needs to be fixed as it's based on wrong assuptions clearly. You can't have a name in MBR. At the same time, the gadget is free to specify a different and name and fs label regardless of schema, so promoting the label to be the name does not ensure success.
There was a problem hiding this comment.
well I adapted the check slightly like this:
Author: Ian Johnson <ian.johnson@canonical.com>
Date: Fri Apr 17 14:06:03 2020 -0500
cmd/snap-bootstrap/bootstrap: also use name == label when ensuring compatibility
This is needed since we no longer will set the Name of the device structure from
the device structure's label, so we also need to check the possibility that the
device structure's label matches the gadget structure's name.
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
diff --git a/cmd/snap-bootstrap/bootstrap/bootstrap.go b/cmd/snap-bootstrap/bootstrap/bootstrap.go
index 4bec579865..2d43c21f6e 100644
--- a/cmd/snap-bootstrap/bootstrap/bootstrap.go
+++ b/cmd/snap-bootstrap/bootstrap/bootstrap.go
@@ -257,8 +257,14 @@ func ensureLayoutCompatibility(gadgetLayout *gadget.LaidOutVolume, diskLayout *p
eq := func(ds partition.DeviceStructure, gs gadget.LaidOutStructure) bool {
dv := ds.VolumeStructure
gv := gs.VolumeStructure
- // Previous installation may have failed before filesystem creation or partition may be encrypted
- check := dv.Name == gv.Name && ds.StartOffset == gs.StartOffset && (ds.Created || dv.Filesystem == gv.Filesystem)
+
+ // the name may not exist on the device structure if this is an MBR
+ // disk, in which case we have to compare the gadget structure name
+ // with the disk structure label
+ check := (dv.Name == gv.Name || dv.Label == gv.Name) &&
+ ds.StartOffset == gs.StartOffset &&
+ // Previous installation may have failed before filesystem creation or partition may be encrypted
+ (ds.Created || dv.Filesystem == gv.Filesystem)
if gv.Role == gadget.SystemData {
// system-data may have been expandedThere was a problem hiding this comment.
As discussed on IRC, e862927 contains the fix.
When using a MBR schema, there is no way to store the partition name, thus the structure names present in the gadget will not appear when inspecting a device layout, rendering the name check void. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
bboozzoo
changed the title
| // reasonable assumption that partitions carrying a filesystem | ||
| // labeled "ubuntu-seed" are part of the factory image while | ||
| // everything else was created at install time | ||
| if fs.Label != ubuntuSeedLabel { |
There was a problem hiding this comment.
I wonder if we should be slightly more explicit here and realy encode ["ubuntu-data", "ubuntu-boot", "ubuntu-save"] because the gadget may have partitions for firmware or in the LK kernel case for storing raw kernels etc?
There was a problem hiding this comment.
Fair point. I'll push it.
There was a problem hiding this comment.
yes, we definitely need to be explicit here (that's what I suggested originally I think, though I forgot that mbr entries themselves carry no names :/ )
There was a problem hiding this comment.
This will cover the factory reset use case, but if we crash during install we may still need to reflash the image (because it could crash before the filesystem is created). But this is not a very likely scenario and reflashing is easy.
There was a problem hiding this comment.
maybe we should add a TODO about considering a mapping from start offset to label (via the gadget info) ?
…ted during install Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
| if gadgetLayout.Schema == "mbr" { | ||
| // partitions have no names in MBR | ||
| nameMatch = true | ||
| } |
| } | ||
| } | ||
|
|
||
| // if we don't have an LBA, we are probably on an MBR disk and need to |
There was a problem hiding this comment.
why is this bit of code not using ptable.Label as condition as there rest?
There was a problem hiding this comment.
I isn't strictly necessary, last-lba is set only for GPT
There was a problem hiding this comment.
can you expand the TODO then: // TODO:UC20: how/where/why/who/when to best do this ???
is not clear to me what it refers to/what we need to improve here
There was a problem hiding this comment.
Right, that TODO can be dropped now.
| ds[i] = DeviceStructure{ | ||
| LaidOutStructure: gadget.LaidOutStructure{ | ||
| VolumeStructure: &structure[i], | ||
| StartOffset: gadget.Size(p.Start) * sectorSize, | ||
| Index: i + 1, | ||
| }, | ||
| Node: p.Node, | ||
| Created: p.isCreated(), | ||
| Created: isCreatedDuringInstall(&p, &bd, ptable.Label), |
There was a problem hiding this comment.
I missed that we had again a Created attr, can we rename it to CreatedByBoostrap or CreatedDuringInstall? all partitions are created somehow
| // reasonable assumption that partitions carrying a filesystem | ||
| // labeled "ubuntu-seed" are part of the factory image while | ||
| // everything else was created at install time | ||
| if fs.Label != ubuntuSeedLabel { |
There was a problem hiding this comment.
yes, we definitely need to be explicit here (that's what I suggested originally I think, though I forgot that mbr entries themselves carry no names :/ )
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
| // sfdisk reports the last usable LBA for GPT disks only | ||
| numSectors = gadget.Size(ptable.LastLBA + 1) | ||
| } else { | ||
| // sfdisk does not report any information about the side of a |
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
There was a problem hiding this comment.
This looks much better than what I originally proposed, thanks 😄
One note though about checking an additional error condition that currently looks like it would result in a bad error message.
Also I have not yet reviewed the tests, will do so this afternoon, but also wanted to give my +1 because I tested this branch on the pi and it worked great 🎉
| v, err := strconv.ParseUint(st, 16, 8) | ||
| if err != nil { | ||
| return "", fmt.Errorf("cannot convert MBR partition type %q", st) | ||
| } | ||
| return fmt.Sprintf("%02X", v), nil |
There was a problem hiding this comment.
this looks good, thanks!
| byFsLabel := filepath.Join(dirs.GlobalRootDir, "/dev/disk/by-label/", encodeLabel(ps.Label)) | ||
| if ps.HasFilesystem() { | ||
| fsLabel := ps.EffectiveFilesystemLabel() | ||
| if fsLabel == "" && ps.Name != "" { |
There was a problem hiding this comment.
what if both fsLabel and ps.Name are "" ? if that's the case then candidates will contain /dev/disk/by-label/ which will result in the loop below failing with candidate /dev/disk/by-label/ is not a symlink, which is a bad user experience IMO, and instead we should just not add anything to candidates in this case in which case if we then have no candidates then the user will see ErrDeviceNotFound
There was a problem hiding this comment.
Good catch. I'll push a patch
Tweak the lookup of device structures to provide better error messages. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
| // reasonable assumption that only partitions carrying | ||
| // ubuntu-boot and ubuntu-data labels are created during | ||
| // install, everything else was part of factory image | ||
| createdDuringInstall := []string{ubuntuBootLabel, ubuntuDataLabel} |
Add support for MBR schema. We can't store extra information in the MBR (unlike GPT) so we hardcode what partitions need to be created/wiped on install/reinstall.