ore: Introspect disk for size rather than hardcode 8GB #944
Conversation
cmd/ore/aws/upload.go
Outdated
| if err != nil { | ||
| return 0, err | ||
| } | ||
| return uint32(info.VirtualSize / (uint64(1024) * 1024 * 1024)), nil |
There was a problem hiding this comment.
This will round down, is that intended?
There was a problem hiding this comment.
I thought about that, but right now the disk images we're providing are exact multiples of GiB. We could make it a fatal error to have it not be exact?
I am not clear what the semantics are here - presumably if it wasn't a GiB multiple one would need to round up?
Although now that I actually look at the docs I notice, yeah we'd need to round up but also:
Default: If you're creating the volume from a snapshot and don't specify a volume size, the default is the snapshot size.
Which makes sense...so why are we specifying it in the first place? A little git blame later:
"The default if nothing is set is 5GB" is confusing - were CL disk images 5GB at that time and he wanted AWS to be larger?
There was a problem hiding this comment.
CL images are 4.5 GB right now, so this is (a) broken, and (b) a behavior change.
There was a problem hiding this comment.
Ah. Well that explains the "5GB" bit probably. But then where did 8GB come from?
There was a problem hiding this comment.
We create 8 GB images in AWS, so that the root FS has a reasonable amount of free space.
For compatibility, maybe use max(8 GB, actual image size)?
There was a problem hiding this comment.
We create 8 GB images in AWS, so that the root FS has a reasonable amount of free space.
Only for AWS? Like Azure and other clouds are still 5GB?
For compatibility, maybe use max(8 GB, actual image size)?
Hmm, if I do that unconditionally then this entire PR would be pointless since I want to grow RHCOS beyond 8GB, right?
It seems like we'd need a new --im-not-cl flag? Or maybe we only clamp if amiName.contains("Container-Linux") ?
There was a problem hiding this comment.
Sorry, I was confused...yeah, we can do max.
There was a problem hiding this comment.
TFW one finds golang/go#20775
Oh well, these are small integers, we can be confident we're not going to hit FP accuracy issues.
There was a problem hiding this comment.
Azure images emitted by our build system are 30 GB and GCE images are 8.5 GB. We're not consistent.
There was a problem hiding this comment.
OK another updated version, still not tested beyond compilation though ⬇️
cmd/ore/aws/upload.go
Outdated
| if err != nil { | ||
| return 0, err | ||
| } | ||
| return uint32(info.VirtualSize / (uint64(1024) * 1024 * 1024)), nil |
There was a problem hiding this comment.
CL images are 4.5 GB right now, so this is (a) broken, and (b) a behavior change.
cgwalters
force-pushed
the
8gb-isnt-enough-for-everyone
branch
from
2e2164d
to
3d36f17
Compare
|
Updated for comments, also tried to consistently use I didn't test it though beyond compilation yet. |
cgwalters
force-pushed
the
8gb-isnt-enough-for-everyone
branch
2 times, most recently
from
8861d98
to
0fc6af8
Compare
|
FWIW we're currently carrying this patch for RHCOS which also implies we have a forked coreos-assembler which is going to be annoying to maintain; I can retest it, any other code comments? |
platform/api/aws/images.go
Outdated
| @@ -36,6 +36,13 @@ var ( | |||
|
|
|||
| type EC2ImageType string | |||
|
|
|||
| // In GiB. This used to be hardcoded in ore. See discussion in | |||
There was a problem hiding this comment.
It was hardcoded in this file, actually. I'm still in favor of dropping that sentence, but 🤷♂️
There was a problem hiding this comment.
Updated to tweak the comment ⬇️
We want to expand the disk size of Red Hat CoreOS. Use the JSON output of `qemu-img info` to get the size of the VMDK, and pass that on to AWS. The hardcoded 8GB moves to Plume. I think to support larger disks there then we'd need to query the snapshot or so?
cgwalters
force-pushed
the
8gb-isnt-enough-for-everyone
branch
from
0fc6af8
to
8244c5d
Compare
|
Also I did test this latest PR, but only with RHCOS, not CL yet. I think it'll work - if not we'll find out on the next nightly job right? |
|
We'll find out on the plume side if we check the uploaded image by hand. We don't run ore automatically. You could download an AWS image for testing ore. |
I did that and uploaded it and it failed, and I didn't have time to debug it in depth. Turns out the problem is it's a |
|
Ah, yeah, for historical reasons there's a separate VMDK image, Good to merge? |
|
good to merge |
We want to expand the disk size of Red Hat CoreOS; previously there was a hardcoded 8GiB size for Container Linux which (I believe entirely coincidentally) matched RHCOS. See coreos#948 and coreos#944
We want to expand the disk size of Red Hat CoreOS; previously there was a hardcoded 8GiB size for Container Linux which (I believe entirely coincidentally) matched RHCOS. See coreos#948 and coreos#944
We want to expand the disk size of Red Hat CoreOS; previously there was a hardcoded 8GiB size for Container Linux which (I believe entirely coincidentally) matched RHCOS. See coreos#948 and coreos#944
We want to expand the disk size of Red Hat CoreOS; previously there was a hardcoded 8GiB size for Container Linux which (I believe entirely coincidentally) matched RHCOS. See coreos#948 and coreos#944
|
This was reverted in #948 |
We want to expand the disk size of Red Hat CoreOS. Use the JSON
output of
qemu-img infoto get the size of the VMDK, and passthat on to AWS.
The hardcoded 8GB moves to Plume. I think to support larger disks
there then we'd need to query the snapshot or so?