cmd-buildextend-metal: dont use anaconda on x86_64 #563
Conversation
jlebon
left a comment
jlebon
left a comment
There was a problem hiding this comment.
Looks good from a quick scan!
Haven't had a chance to test it yet.
src/cmd-buildextend-metal
Outdated
|
|
||
| Build raw metal images for bios or metal. If neither switches are provided, | ||
| both targets are built. | ||
| Build raw metal images. |
There was a problem hiding this comment.
I guess we only have one metal image now, right? So raw metal image ?
There was a problem hiding this comment.
Build a bare metal image? raw seems redundant.
src/cmd-buildextend-metal
Outdated
| itype=metal | ||
| img=${name}-${build}-${itype}.raw | ||
| if [ -f "${builddir}/${img}" ]; then | ||
| echo "Image $itype already exists" |
There was a problem hiding this comment.
Maybe just Bare metal image already exists
|
Pushed the suggested changes |
| /usr/lib/coreos-assembler/estimate-commit-disk-size --repo "$ostree_repo" "$ref" > "$PWD/tmp/ostree-size.json" | ||
| fi | ||
| size="$(jq '."estimate-mb".final' "$PWD/tmp/ostree-size.json")" | ||
| # 384M is the non-ostree partitions |
There was a problem hiding this comment.
Random thought related to this: I remember we recently discussed the fact that because we don't actually expand until the real root, it technically limits how much Ignition can write to the disk. Or I forgot, did we say we want to switch that auto-expand logic to a base config so ignition-disks does it?
There was a problem hiding this comment.
I don't remember where we stand on that either, but it's somewhat disconnected from this PR. Ignition disks notably can't extend filesystems, only partitions (fixing that is... complicated). We might think about doing something between disks and files though (effectively moving coreos-growpart or something like it to the initramfs).
This currently matches the existing behavior, lets separate any changes there to another PR?
jlebon
left a comment
jlebon
left a comment
There was a problem hiding this comment.
Nice, built and installed locally with no issues.
Just a couple more comments.
| rm -f "${path}".tmp | ||
| else | ||
| if [ ! -e "$PWD/tmp/ostree-size.json" ]; then | ||
| /usr/lib/coreos-assembler/estimate-commit-disk-size --repo "$ostree_repo" "$ref" > "$PWD/tmp/ostree-size.json" |
There was a problem hiding this comment.
An echo "Estimating disk size..." here would be useful, since it does take some time.
| /usr/lib/coreos-assembler/estimate-commit-disk-size --repo "$ostree_repo" "$ref" > "$PWD/tmp/ostree-size.json" | ||
| fi | ||
| size="$(jq '."estimate-mb".final' "$PWD/tmp/ostree-size.json")" | ||
| # 384M is the non-ostree partitions |
src/cmd-buildextend-metal
Outdated
| "x86_64"|"aarch64") | ||
| ## fall through to the rest of the file | ||
| ;; | ||
| "ppc64le"|"s390x") |
There was a problem hiding this comment.
Minor: wouldn't just * here be safer?
| path=${PWD}/${img} | ||
| run_virtinstall "${ostree_repo}" "${ref}" "${path}.tmp" --variant="${itype}" | ||
| if [ "$arch" == "aarch64" ]; then | ||
| run_virtinstall "${ostree_repo}" "${ref}" "${path}.tmp" --variant="${itype}-uefi" |
There was a problem hiding this comment.
Hmm, how come we don't have to estimate the disk size in this path?
There was a problem hiding this comment.
We do, it's in run_virtinstall
|
Fixed! |
jlebon
left a comment
jlebon
left a comment
There was a problem hiding this comment.
LGTM! Optional tweak but feel free to merge as is.
src/cmd-buildextend-metal
Outdated
| /usr/lib/coreos-assembler/estimate-commit-disk-size --repo "$ostree_repo" "$ref" > "$PWD/tmp/ostree-size.json" | ||
| fi | ||
| size="$(jq '."estimate-mb".final' "$PWD/tmp/ostree-size.json")" | ||
| echo "Disk size estimated to $size" |
There was a problem hiding this comment.
Minor/optional: this has no units. Maybe just move it one line down so we print with the M.
There was a problem hiding this comment.
Oh right. fixed now. That would have also left off the 384M.
Also fail out on unsupported arches and continue using anaconda on aarch64.
2 participants
Also fail out on unsupported arches and continue using anaconda on
aarch64.
Tested on packet (bios) and that worked with the current fcos installer, ran through a cosa build on arm64 as well to confirm this doesn't break that.
Fixes #334