Skip to content
This repository has been archived by the owner on May 7, 2021. It is now read-only.

Revert "ore: Introspect disk for size rather than hardcode 8GB" #948

Merged
merged 1 commit into from
Nov 17, 2018
Merged

Revert "ore: Introspect disk for size rather than hardcode 8GB" #948

merged 1 commit into from
Nov 17, 2018

Conversation

bgilbert
Copy link
Contributor

It breaks all cases that don't include an explicit --file argument, including --source-object, --source-snapshot, and using the latest build from the CL SDK. Revert for now.

This reverts commit 8244c5d.

cc @cgwalters

It breaks all cases that don't include an explicit --file argument,
including --source-object, --source-snapshot, and using the latest build
from the CL SDK.  Revert for now.

This reverts commit 8244c5d.
@cgwalters
Copy link
Member

LGTM, will look at fixing this in a next attempt.

@bgilbert bgilbert merged commit 73fc1d0 into coreos:master Nov 17, 2018
@bgilbert bgilbert deleted the 8gb-is-defined-to-be-enough-for-everyone branch November 17, 2018 14:10
@cgwalters
Copy link
Member

bgilbert:8gb-is-defined-to-be-enough-for-everyone

😆 I see you noticed my branch name...

OK so what was our thought on fixing this? We could only do it if --file is provided?

@bgilbert
Copy link
Contributor Author

I think it'd be cleanest for the size to be the larger of:

  • A new command-line argument specifying the desired AMI disk size, defaulting to 8 GiB to handle the historical case.
  • The disk size from the image file (from --file or autodetected), object (from --source-object), or snapshot (from --source-snapshot).

Unfortunately that requires a nontrivial amount of new code.

@cgwalters
Copy link
Member

The disk size from the image file (from --file or autodetected), object (from --source-object), or snapshot (from --source-snapshot).

One thing I don't quite understand is the use cases for the latter two. Are they used today in CL?

The flow we have for RHCOS at least today is to upload via a file to a single region, get an AMI, then use AWS' builtin region-copy API (which is what plume is doing I believe). So at least we don't need object/snapshot bits (we always pass --delete-object).

How about --image-inspect-size or so which would be used with --file?

@bgilbert
Copy link
Contributor Author

Are they used today in CL?

Remember, ore is a development tool, not a production tool. I don't think anyone actively uses them, but they might be useful for working with images in various stages of creation.

How about --image-inspect-size or so which would be used with --file?

I'm not thrilled with lack of consistency across image sources, but I can live with it. We should still have the manual option though:

  • --image-size, defaulting to 8 GiB
  • --image-inspect-size, which works with --file (or autodetected local files) and overrides --image-size

cgwalters added a commit to cgwalters/mantle that referenced this pull request Nov 29, 2018
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
cgwalters added a commit to cgwalters/mantle that referenced this pull request Nov 29, 2018
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
cgwalters added a commit to cgwalters/mantle that referenced this pull request Nov 29, 2018
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
cgwalters added a commit to cgwalters/mantle that referenced this pull request Nov 30, 2018
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants