snap-recovery: create filesystems as defined in the gadget #7629
Conversation
This adds filesystem creation to the snap-recovery tool.
There was a problem hiding this comment.
First pass. Please forgive me if my questions are silly, I'm new to the state-of-the-art of this area.
I'm mainly -1 about packaging snap-recover outside of core systems (exception is ubuntu 16.04 but even there I would argue, it is better to have a new package for all the core tooling nobody ever needs on classic). This avoids confusion in packaging for other systems where people sometimes look at what the ubuntu packaging does.
cmd/snap-recovery/partition/mkfs.go
Outdated
| func MakeFilesystems(created map[string]gadget.LaidOutStructure) error { | ||
| for node, part := range created { | ||
| if part.VolumeStructure.Filesystem != "" { | ||
| if err := mkfs(node, part.VolumeStructure.Label, part.VolumeStructure.Filesystem); err != nil { |
There was a problem hiding this comment.
I don't like the fact that this creates filesystems in non-deterministic order. Could you please consider sorting them by node name so that this function is entirely deterministic.
There was a problem hiding this comment.
There will be a followup that will use the gpt partition attributes to mark partitions as ready or in progress. I assume your concern is idempotency? This will be addressed. Or is there another reason why this should be deterministic?
There was a problem hiding this comment.
In this specific case my concern is determinism. If this is used on millions of devices it should not result in sometimes doing partition A ahead of B and sometimes the other way around.
There was a problem hiding this comment.
Yeah, I agree with @zyga but a doing that in the next PR sounds good too.
| // details are already tested in the gadget package | ||
| c.Assert(mockMkfs.Calls(), HasLen, 1) | ||
| c.Assert(s.mockMkfsExt4.Calls(), HasLen, 1) | ||
| c.Assert(s.mockMkfsVfat.Calls(), HasLen, 1) |
There was a problem hiding this comment.
If you follow my recommendation please ensure that the order is tested here.
cmd/snap-recovery/recover/recover.go
Outdated
| // will contain encryption later | ||
| } | ||
|
|
||
| type Recover struct { |
There was a problem hiding this comment.
Package as well as type-level descriptions would be appreciated.
There was a problem hiding this comment.
Yeah, this is all very early though, given the current structure of the Recover struct this may just become a single function and the package may actually be removed and just folded into the cmd/snap-recovery/ code. Sorry that its not quite so clean as usually, this is merging bit of Claudio and my own ideas which makes this a bit less straightforward than usual.
|
Thanks for the feedback! I addressed a lot of this in #7613 and once that is in will get back to this one here and address the remaining pieces. |
We don't actually need the snap-recovery binary on classic. So we can skip building/shipping it everywhere except on ubuntu 16.04+ where we build the core/snapd snap.
packaging/snapd.mk
Outdated
| @@ -50,14 +50,14 @@ snap_mount_dir = /snap | |||
| endif | |||
|
|
|||
| # The list of go binaries we are expected to build. | |||
| go_binaries = snap snapctl snap-seccomp snap-update-ns snap-exec snapd snap-recovery | |||
There was a problem hiding this comment.
Note, you can still keep this part but expand the part below that removes non-core binaries.
| @@ -216,10 +217,10 @@ func buildPartitionList(ptable *sfdiskPartitionTable, pv *gadget.LaidOutVolume) | |||
| s.Size/sectorSize, partitionType(ptable.Label, p.Type), s.Name) | |||
|
|
|||
| // Are roles unique so we can use it to map nodes? Should we use labels instead? | |||
| deviceMap[s.Role] = node | |||
| toBeCreated[node] = p | |||
There was a problem hiding this comment.
Wish we had more reliable name to identify device nodes rather than making up the name ourselves. In the context of the followup PR, perhaps we should create, list the partitions and match then with structures, and then operate on the device nodes.
| buf := &bytes.Buffer{} | ||
| deviceMap := map[string]string{} | ||
| toBeCreated = make(map[string]gadget.LaidOutStructure, len(pv.LaidOutStructure)) |
There was a problem hiding this comment.
Probably a followup material, but we could do:
type deviceStructure {
gadget.LaidOutStructure
Node string
}
There was a problem hiding this comment.
Good suggestion, I implemented this now as []deviceStructure sorted by layout as defined in the gadget.yaml
cmd/snap-recovery/partition/mkfs.go
Outdated
| func MakeFilesystems(created map[string]gadget.LaidOutStructure) error { | ||
| for node, part := range created { | ||
| if part.VolumeStructure.Filesystem != "" { | ||
| if err := mkfs(node, part.VolumeStructure.Label, part.VolumeStructure.Filesystem); err != nil { |
There was a problem hiding this comment.
Yeah, I agree with @zyga but a doing that in the next PR sounds good too.
…e with udev apparently)
This adds filesystem creation to the snap-recovery tool.
Based on #7613