data/libvirt: Resize to 16 GiB #413

wking · 2018-10-04T06:37:47Z

This is the size of the current RHCOS images, but resizing here will allow the OS folks to stop guessing which size we need (#126, openshift/os#228).

Explicitly closing the file before resizing ensures we've flushed the cache after the earlier copy. We probably should have been doing that before the Rename anyway. With this explict close, the deferred close call may fail, but that's fine.

Dropping to qemu-img makes me a bit jumpy, but at least most folks with a libvirtd running will have qemu-img available.

Fixes #126.

openshift-ci-robot · 2018-10-04T06:37:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abhinavdahiya · 2018-10-04T13:21:58Z

pkg/types/config/libvirt/cache.go

+		return err
+	}
+
+	err = exec.Command("qemu-img", "resize", tempPath, "16G").Run()


Is this binary always in path?

I checked on this. qemu-img is its own package, but looks like it should always be installed based on dependencies of libvirt and qemu

but actually, virt-resize may be a better choice. It's a bit smarter and knows how to deal with resizing partitions within the disk image.

http://libguestfs.org/virt-resize.1.html

but actually, virt-resize may be a better choice...

Should I try this and fall back to qemu-img if it's not found? Or can we rely on libvirtd users having it installed?

I don't think you can depend on having this one. We'd have to add "libguestfs-tools-c" as one of the dependencies to install in the libvirt howto, but I think that's the right thing to do.

No, we explicitly don't want virt-resize - RHCOS includes code to resize partitions on boot.

For similar reasons, you wouldn't want to use virt-resize on the Fedora/RHEL "classic" cloud images that include cloud-init, which has similar code.

To elaborate on this further, the reason I prefer this is it maintains parity with how e.g. OpenStack and AWS work - they just resize the raw disk image and rely on code inside the OS to react to that (historically again, that's cloud-init on many distributions, but not CoreOS).

ACK, fair enough.

crawford · 2018-10-04T15:22:18Z

The RHCOS team needs to know how large the image needs to be though. Otherwise, the installer will need to pre-process images for every platform.

cgwalters · 2018-10-04T16:47:17Z

I'm confused;

# qemu-img info /srv/libvirt/images-gold/rhcos-4.0.6469-qemu.qcow2 |grep -i virtu
virtual size: 16G (17179869184 bytes)

So why does this talk about resizing to 16GB?

wking · 2018-10-04T17:23:18Z

The RHCOS team needs to know how large the image needs to be though. Otherwise, the installer will need to pre-process images for every platform.

Is pre-processing images for every platform a problem? I think the size constraint is mostly "we need this much space for our installer stuff" and less about how much space the OS itself needs. See also #126, which I guess you'd close as wontfix if you feel that this really is something RHCOS should be handling for us.

I'm confused;
# qemu-img info /srv/libvirt/images-gold/rhcos-4.0.6469-qemu.qcow2 |grep -i virtu
virtual size: 16G (17179869184 bytes)
So why does this talk about resizing to 16GB?

If/when this lands, you can revert openshift/os#228.

rphillips · 2018-10-04T21:26:32Z

I understand cloud-init is not in RHCOS; however, terraform-provider-libvirt just merged a PR to add the wanted size to an image. dmacvicar/terraform-provider-libvirt#416

wking · 2018-10-04T23:45:48Z

/retest

Exercising openshift/release#1817.

crawford · 2018-10-15T16:54:27Z

I would prefer it if we made use of dmacvicar/terraform-provider-libvirt#416 instead of invoking an external command. I would prefer it even more if we didn't bother with it at all and just assume that the base image will always be big enough.

wking · 2018-10-15T17:23:03Z

I would prefer it if we made use of dmacvicar/terraform-provider-libvirt#416 instead of invoking an external command.

I can reroll to do this.

I would prefer it even more if we didn't bother with it at all and just assume that the base image will always be big enough.

I'm fine with this too; it's really up to the RHCOS folks. If you can talk them into closing #126 (or put your foot down and close it wontfix), then we can close this PR.

This is the size of the current RHCOS images: $ qemu-img info ~/.cache/openshift-install/libvirt/image/2454fcdc0a61a9005fbf916b54fbe332 image: /home/trking/.cache/openshift-install/libvirt/image/2454fcdc0a61a9005fbf916b54fbe332 file format: qcow2 virtual size: 16G (17179869184 bytes) disk size: 1.6G cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false Resizing here will allow the OS folks to stop guessing which size we need [1,2]. And when we need future resizes, we can update here instead of bugging the OS team. Also inline the volume in the libvirt Terraform module. The libvirt/volume module indirection was unnecessary indirection. [1]: openshift#126 [2]: openshift/os#228

wking · 2018-10-15T17:52:57Z

I've rebased this onto master and moved to the Terraform size approach with 7e98d1e -> 7385b35.

abhinavdahiya · 2018-10-17T20:29:59Z

@crawford is this good to go?

crawford · 2018-10-17T21:36:51Z

@abhinavdahiya I'd rather not enforce the size requirement in the installer. RHCOS is specifically designed to run OpenShift workloads (and only OpenShift), so it seems reasonable to me that it is sized correctly. On top of that, it isn't required that the installer is used to create infrastructure, so doing the size check there seems analogous to relying on front-end validation.

crawford · 2018-11-02T16:39:31Z

I'm going to close this out. It is not the installer's job to make sure that the OS is the correct size; that's the OS team's responsibility.

/close

openshift-ci-robot · 2018-11-02T16:39:33Z

@crawford: Closing this PR.

In response to this:

I'm going to close this out. It is not the installer's job to make sure that the OS is the correct size; that's the OS team's responsibility.

/close

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.

wking · 2018-11-02T18:03:55Z

I'm going to close this out. It is not the installer's job...

So close #126 as wontfix?

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 4, 2018

openshift-ci-robot requested review from aaronlevy and staebler October 4, 2018 06:37

wking mentioned this pull request Oct 4, 2018

Image resize support for libvirt/development use cases #126

Closed

abhinavdahiya reviewed Oct 4, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2018

wking mentioned this pull request Oct 15, 2018

pkg/tfvars/libvirt/cache: Close before renaming #464

Merged

wking force-pushed the resize-qcow branch from 7e98d1e to 7385b35 Compare October 15, 2018 17:51

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2018

wking changed the title ~~pkg/types/config/libvirt/cache: Resize to 16 GB~~ data/libvirt: Resize to 16 GiB Oct 15, 2018

openshift-ci-robot closed this Nov 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data/libvirt: Resize to 16 GiB #413

data/libvirt: Resize to 16 GiB #413

wking commented Oct 4, 2018

openshift-ci-robot commented Oct 4, 2018

abhinavdahiya Oct 4, 2018

russellb Oct 4, 2018

russellb Oct 4, 2018

wking Oct 4, 2018

russellb Oct 4, 2018 •

edited

Loading

cgwalters Oct 4, 2018

cgwalters Oct 4, 2018

russellb Oct 4, 2018

crawford commented Oct 4, 2018

cgwalters commented Oct 4, 2018

wking commented Oct 4, 2018

rphillips commented Oct 4, 2018

wking commented Oct 4, 2018

crawford commented Oct 15, 2018

wking commented Oct 15, 2018

wking commented Oct 15, 2018

abhinavdahiya commented Oct 17, 2018

crawford commented Oct 17, 2018

crawford commented Nov 2, 2018

openshift-ci-robot commented Nov 2, 2018

wking commented Nov 2, 2018

data/libvirt: Resize to 16 GiB #413

data/libvirt: Resize to 16 GiB #413

Conversation

wking commented Oct 4, 2018

openshift-ci-robot commented Oct 4, 2018

abhinavdahiya Oct 4, 2018

Choose a reason for hiding this comment

russellb Oct 4, 2018

Choose a reason for hiding this comment

russellb Oct 4, 2018

Choose a reason for hiding this comment

wking Oct 4, 2018

Choose a reason for hiding this comment

russellb Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

cgwalters Oct 4, 2018

Choose a reason for hiding this comment

cgwalters Oct 4, 2018

Choose a reason for hiding this comment

russellb Oct 4, 2018

Choose a reason for hiding this comment

crawford commented Oct 4, 2018

cgwalters commented Oct 4, 2018

wking commented Oct 4, 2018

rphillips commented Oct 4, 2018

wking commented Oct 4, 2018

crawford commented Oct 15, 2018

wking commented Oct 15, 2018

wking commented Oct 15, 2018

abhinavdahiya commented Oct 17, 2018

crawford commented Oct 17, 2018

crawford commented Nov 2, 2018

openshift-ci-robot commented Nov 2, 2018

wking commented Nov 2, 2018

russellb Oct 4, 2018 •

edited

Loading