-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libvirt: Resize specifically to 16G #2652
Conversation
/label libvirt |
@cgwalters: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/label platform/libvirt |
@cgwalters: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
I don't think we are inheriting that, we explicitly required the RHCOS to be the size by default. need to find the previous conversation with @crawford
hmm, how does that affect this, maybe some context? |
It seems we need the same fix for platform/baremetal - in recent IPI baremetal deploys we're seeing errors like this:
We can see that /sysroot is only 2.5G and it's full:
@cgwalters would you like to update this to align https://github.com/openshift/installer/blob/master/data/data/baremetal/bootstrap/main.tf with the changes here, or should we push a separate PR that copies the same fixes? |
I opened #2673 for baremetal but happy to close if you want to incorporate those changes here. |
Hmm. Perhaps it'll be simpler for RHCOS to switch to having a smaller partition internally that's always resized rather than requiring this change. |
Took a different tack on this in coreos/coreos-assembler#917 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cgwalters, darkmuggle The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is a blocker for IPI baremetal so it'd be good to either get this in coreos/coreos-assembler#917 in or this PR and #2673 |
Today for IaaS clouds, we default to an instance type, which then in turn usually provides a default size. RHCOS resizes on boot to that size, distinct from its "default" 16G size. However, libvirt installs were inheriting our default size. We'd like to shrink it because we plan to land encryption: openshift/enhancements#15 And the less data we need to encrypt, the better. (In the future I'd like to make this configurable with a variable, but let's just prepare for the encryption work now)
e92554c
to
5e46b88
Compare
Hmm, wouldn't it make more sense for the disk size requirement to live in the installer (where I presume the code to select the right disk size on various clouds also live)? |
Right, I agree with this consistency argument for merging this. That said I think we should also change the RHCOS builds with coreos/coreos-assembler#924 since that makes things more consistent on that side too. |
if the default RHCOS disk size needs to be configured by anybody trying to use it.. means it's a wrong default. So if the installer needs to re-size for the libvirt we is supposed to the bare-minimum we can do I think the resize shouldn't live here. |
We definitely support customers picking instance and disk sizes on IaaS (GCP/AWS/OpenStack etc.) Ultimately, I think we'll want this flexibility for libvirt too (exposed via env variables or install configs maybe). |
coreos/coreos-assembler#924 merged which lessens the importance of this at least. |
OK, we have another use case for this, which is allowing CodeReady Containers to expand the RHCOS root disk before doing an install. That one needs an explicit configuration option for it in some form, either an environment variable or an installconfig option. |
/test e2e-libvirt |
Not sure why? Can somebody provide context/detail. |
@cgwalters: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Likely because in CRC everything is kept on a single node.
We can pick a number that satisfies the most common use cases when building the image. But IMO it's completely reasonable to want more control than that at deployment time depending on your circumstances. How many nodes will you have? How big are the images you expect to be using? Do you need to carve out more partitions for compliance? etc... |
Yes, CRC is using a single node, so the users' workloads are going to be installed on that disk, and the default 16GB size has been too small in some cases. |
Closing due to this being open for a long time, Please feel free to reopen /close |
@abhinavdahiya: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Today for IaaS clouds, we default to an instance type, which
then in turn usually provides a default size. RHCOS resizes on
boot to that size, distinct from its "default" 16G size.
However, libvirt installs were inheriting our default size. We'd
like to shrink it because we plan to land encryption:
openshift/enhancements#15
And the less data we need to encrypt, the better.
(In the future I'd like to make this configurable with a variable,
but let's just prepare for the encryption work now)