WIP: fcos translate.go: add warn on small or constrained root partition #378
Conversation
prestist
force-pushed
the
warn-on-fs-too-small
branch
from
df3fd2e
to
166e0d5
Compare
config/fcos/v1_5_exp/translate.go
Outdated
| r.AddOnWarn(path.New("json", "storage", "disks", i, "partitions", p, "sizeMiB", *partition.SizeMiB), common.ErrRootTooSmall) | ||
| } else { | ||
| for _, op := range disk.Partitions { | ||
| if op.Number > partition.Number { |
There was a problem hiding this comment.
We're currently performing this check for every partition after the root partition, not just the one immediately after it.
There was a problem hiding this comment.
OH can we assume that they are always in order?
There was a problem hiding this comment.
What we don't want is to warn repeatedly for every additional partition. But you're right, we can't assume they're in order, either by starting offset or by partition number. And actually, we can't make assumptions about op.Number either, since it might be 0.
So we'll need an heuristic for figuring out the earliest partition start. If the startMiB is always specified, this is easy. Otherwise, the partition order is determined by a combination of sgdisk's heuristics (see the man page for the -n option) and the additional heuristics applied by partitions.go. And unfortunately, they have some information we don't, which is the overall size of the disk, and thus the size of the largest available block.
So we'll need to figure this out. If there are cases where we're not sure, it's better to not warn.
config/fcos/v1_5_exp/translate.go
Outdated
| } else { | ||
| for _, op := range disk.Partitions { | ||
| if op.Number > partition.Number { | ||
| if op.StartMiB == nil { |
There was a problem hiding this comment.
This doesn't match the logic described in #211 (comment).
config/common/errors.go
Outdated
| @@ -37,6 +37,8 @@ var ( | |||
| ErrNodeExists = errors.New("matching filesystem node has existing contents or different type") | |||
| ErrNoFilesDir = errors.New("local file paths are relative to a files directory that must be specified with -d/--files-dir") | |||
| ErrTreeNotDirectory = errors.New("root of tree must be a directory") | |||
| ErrRootTooSmall = errors.New("root should have 8GiB of space available") | |||
| ErrRootNotLastPartition = errors.New("root should be last partition number to allow for growth") | |||
There was a problem hiding this comment.
The message is misleading, since we allow and encourage adding partitions after the root.
config/common/errors.go
Outdated
| @@ -37,6 +37,8 @@ var ( | |||
| ErrNodeExists = errors.New("matching filesystem node has existing contents or different type") | |||
| ErrNoFilesDir = errors.New("local file paths are relative to a files directory that must be specified with -d/--files-dir") | |||
| ErrTreeNotDirectory = errors.New("root of tree must be a directory") | |||
| ErrRootTooSmall = errors.New("root should have 8GiB of space available") | |||
There was a problem hiding this comment.
nit: available could be read to imply free space in the filesystem.
| if partition.Label != nil { | ||
| for p, partition := range disk.Partitions { | ||
| if partition.Label != nil { | ||
| if *partition.Label == "root" { |
config/fcos/v1_5_exp/translate.go
Outdated
| } | ||
| } | ||
| } | ||
| // Don't warn if wipeTable is set, matching later spec versions |
There was a problem hiding this comment.
Looks like we lost the original comment here. "matching later spec versions"?
| { | ||
| Kind: report.Warn, | ||
| Message: common.ErrRootTooSmall.Error(), | ||
| Context: path.New("json", "storage", "disks", 0, "partitions", 0, "sizeMiB", 500), |
There was a problem hiding this comment.
This is specifying the 501st element of the sizeMiB array.
| Partitions: []types.Partition{ | ||
| { | ||
| Label: util.StrToPtr("root"), | ||
| SizeMiB: util.IntToPtr(10000), |
There was a problem hiding this comment.
This size is sufficient, so the test should pass.
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Let's also add tests where the root partition doesn't explicitly specify a size but is constrained by another partition which does/doesn't leave enough room.
| { | ||
| Kind: report.Warn, | ||
| Message: common.ErrRootNotLastPartition.Error(), | ||
| Context: path.New("json", "storage", "disks", 0, "partitions", 0, "number", 4), |
There was a problem hiding this comment.
Likewise, this is the 5th element of the number array.
prestist
force-pushed
the
warn-on-fs-too-small
branch
6 times, most recently
from
b95add4
to
98397ca
Compare
prestist
force-pushed
the
warn-on-fs-too-small
branch
from
98397ca
to
8a4a4c3
Compare
Fixes coreos#211, the root partition needs a certain amount of space, with recent changes butane has the opportunity to warn when those expectations are not met. Add a warn when the root is too small, and additionally add warn when the root is small and cannot expand.
prestist
force-pushed
the
warn-on-fs-too-small
branch
from
8a4a4c3
to
50f80a6
Compare
#211
First naive approach, plan on refactoring before coping work to prior versions of fcos