Disk resize #1607

cfergeau · 2020-10-20T16:02:33Z

Fixes: Issue #127

Solution/Idea

This uses the machine driver's "UpdateConfigRaw" API introduced in the previous release in order to implement disk resizing support. This needs machine driver support as well.

Proposed changes

List main as well as consequential changes you introduced or had to introduce.

Added --disk-size argument to the start command.
Added 'disk-size' config key

disk size is always in GiB (102410241024 bytes) and can't be less than 31 GiB as this is the size used by our bundles, and disks can't be shrunk.

Testing

What is the bottom-line functionality that needs testing? Describe in pseudo-code or in English. Use verifiable statements that tie your changes to existing functionality.

start without any argument/special configuration succeeds
crc start --disk-size xxx succeeds and resizes the disk
crc start succeeds after crc config set disk-size xxx
in both cases, disk is resized (can be checked with ssh ... df -h, or on linux through virsh vol-info crc.qcow2 crc)

For now this is not supported on macos
6. start fails after start succeeded or after status says CRC is Running
7. ...

pkg/crc/machine/machine.go

guillaumerose · 2020-10-21T06:15:10Z

pkg/crc/machine/machine.go

+		if err := setDiskSize(host, startConfig.DiskSize); err != nil {
+			logging.Debugf("Failed to update CRC disk configuration: %v", err)
+			if err != drivers.ErrNotImplemented {
+				return err


I guess this is where macOS user will currently fall ?

Same my concern, startConfig.DiskSize will have the default DiskSize, shouldn't we compare startConfig.DiskSize != constants.DefaultDiskSize ?

This is checking for UpdateConfigRaw failures, at the moment the hyperkit driver only ignores disk sizes, so it won't return an error, it needs an update in order to return an error. This is one part where UpdateConfigRaw could be improved, return an error if it was called but did not modify any fields.

Same my concern, startConfig.DiskSize will have the default DiskSize, shouldn't we compare startConfig.DiskSize != constants.DefaultDiskSize ?

I don't understand what you are asking :( However, I did not want the 'disk-size' config to default to constants.DefaultDiskSize as this would cause unexpected behaviour when doing
crc start --disk-size 50; crc stop; crc start.
It would try to resize the disk back to constants.DefaultDiskSize, but the libvirt driver errors out when trying to do that. So it's better to keep the old size, and 0 is used as a means to indicate "no value was set"

This is one part where UpdateConfigRaw could be improved, return an error if it was called but did not modify any fields.

The machine-driver-hyperkit implementation may be something like this: (untested)

From c83e1a0c18f5df18940e686dd51737bd93fe67d1 Mon Sep 17 00:00:00 2001 From: Christophe Fergeau <cfergeau@redhat.com> Date: Wed, 21 Oct 2020 20:36:26 +0200 Subject: [PATCH] Report an error when trying to change unsupported field Only memory size and number of CPUS should be changed with `UpdateConfigRaw`. If neither of these changed, the call to UpdateConfigRaw is unsupported. --- pkg/hyperkit/driver.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/hyperkit/driver.go b/pkg/hyperkit/driver.go index 084b54e..c8bbd90 100644 --- a/pkg/hyperkit/driver.go +++ b/pkg/hyperkit/driver.go @@ -416,6 +416,13 @@ func (d *Driver) UpdateConfigRaw(rawConfig []byte) error { return err } + if newDriver.Memory == d.Memory && newDriver.CPU == d.CPU { + /* For now only changing memory and CPU is supported/tested. + * If none of these changed, we might be trying to change another + * value, which is may or may not work, return ErrNotImplemented for now + */ + return drivers.ErrNotImplemented + } *d = newDriver return nil -- 2.28.0

I tried on macOS and the flag is just discarded. I guess it's fine for a first release.

crc-org/machine-driver-hyperkit#21 should improve this, together with the commit I added to this PR to show a warning when ErrNotImplemented is returned.

crc-org/machine-driver-hyperkit#21 is not for disk size. Should we add it ?

cmd/crc/cmd/start.go

gbraad · 2020-10-21T06:39:59Z

pkg/crc/validation/validation.go

+	if value == 0 {
+		return nil
+	}
+	if value < constants.DefaultDiskSize {


praveenkumar · 2020-10-21T08:48:55Z

go.mod

@@ -75,4 +75,7 @@ replace (
 	k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.18.2
 )

-replace vbom.ml/util => github.com/fvbommel/util v0.0.2
+replace (
+	github.com/code-ready/machine => ../machine


I think this need to be removed once you have changes in machine side.

pkg/crc/machine/machine.go

praveenkumar · 2020-10-21T08:54:20Z

pkg/crc/machine/machine.go

+		if err := setDiskSize(host, startConfig.DiskSize); err != nil {
+			logging.Debugf("Failed to update CRC disk configuration: %v", err)
+			if err != drivers.ErrNotImplemented {
+				return err


Same my concern, startConfig.DiskSize will have the default DiskSize, shouldn't we compare startConfig.DiskSize != constants.DefaultDiskSize ?

gbraad · 2020-10-21T09:53:19Z

@cfergeau can you describe clearly for @kowen what is supported and what not? This needs to be part of the release note,s. But let's also consider to actually document this. Can this be a follow PR?

cfergeau · 2020-10-21T10:57:24Z

@cfergeau can you describe clearly for @kowen what is supported and what not? This needs to be part of the release note,s. But let's also consider to actually document this. Can this be a follow PR?

The machine PR crc-org/machine#48 adds support for disk resize on Windows (though I haven't retested it recently). The machine-driver-libvirt PR adds support on linux crc-org/machine-driver-libvirt#68
hyperkit won't be supported for now, the hyperkit machine driver needs to be updated to use the latest version of https://github.com/moby/hyperkit/tree/master/go to get resizing support moby/hyperkit@7e0fa50

This PR description #1607 (comment) has a user-level explanation regarding how to use it. Disk size is specified in GiB, though this can easily be changed to GB if more user friendly.

cfergeau · 2020-10-26T13:50:22Z

/retest

guillaumerose · 2020-10-27T10:10:09Z

cmd/crc/cmd/config/config.go

@@ -28,6 +29,7 @@ func RegisterSettings(cfg *config.Config) {
 	cfg.AddSetting(Bundle, constants.DefaultBundlePath, config.ValidateBundle, config.SuccessfullyApplied)
 	cfg.AddSetting(CPUs, constants.DefaultCPUs, config.ValidateCPUs, config.RequiresRestartMsg)
 	cfg.AddSetting(Memory, constants.DefaultMemory, config.ValidateMemory, config.RequiresRestartMsg)
+	cfg.AddSetting(DiskSize, 0, config.ValidateDiskSize, config.RequiresRestartMsg)


Why 0 and not DefaultDiskSize ?

Initial concern was to be able to differentiate between "no value was specified on the command line" and "user specified a value which happens to be the default one". Without this, initial iterations of the code would end up calling if err := setDiskSize(host, cfg.DefaultDiskSize), which would trigger an error if the disk had been resized before.
However, now the setDiskSize call is wrapped in a if diskSize != 0 test. If we change this test to if diskSize != cfg.DefaultDiskSize then everything should work. I'll test this now.

@cfergeau doesn't our validation run initially and error out if the provided disk size is not greater than the default one and we only run the disk resize part when the disk size is provided greater than default?

The "only run the disk resize part when the disk size is provided greater than default" is the part which is not done yet, we only have if startConfig.diskSize != 0, I think this could be replaced with if startConfig.diskSize != constants.DefaultDiskSize, but this needs testing

I've force-pushed an updated series, only difference is a fixup commit which replaces the 0 default value with a constants.DefaultDiskSize default value, we can go with this one I think, I'll squash it.

guillaumerose · 2020-10-27T10:15:46Z

pkg/crc/validation/validation.go

@@ -31,6 +31,17 @@ func ValidateMemory(value int) error {
 	return ValidateEnoughMemory(value)
 }

+func ValidateDiskSize(value int) error {
+	/* If no disk size was set on the command line or in the preferences, we'll validate '0' */
+	if value == 0 {


I guess this check is related to 0 being the default value.

openshift-merge-robot · 2020-10-29T16:15:09Z

@cfergeau: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-crc	`5ab0673`	link	`/test e2e-crc`

Full PR test history. Your PR dashboard.

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.

This adds a --disk-size command line argument to `crc start`, as well as a 'disk-size' config option. The size is specified in GiB. Upon next crc start, the size of the VM disk will be automatically resized. This relies on machine driver support, the UpdateConfigRaw implementation must be able to resize the disk image. Once that is done, `xfs_growfs` is called on VM startup in order to grow the size of the VM partition.

In order to be able to support disk image resizing in the libvirt machine driver (through virStorageVolResize), we need to use libvirt storage pools. This commit sets the name of the storage pool that the machine driver should be using.

When this error is returned, this means the user tried to change the VM settings, but they were not applied. We currently only print a debug log about such errors, but this deserves a user-visible warning as it's unexpected that the configuration change did not happen.

openshift-ci-robot · 2020-10-30T09:06:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, gbraad, guillaumerose

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~~ [cfergeau,gbraad,guillaumerose]

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

openshift-ci-robot requested review from anjannath and praveenkumar October 20, 2020 16:02

openshift-ci-robot added the approved label Oct 20, 2020

guillaumerose reviewed Oct 20, 2020

View reviewed changes

pkg/crc/machine/machine.go Show resolved Hide resolved

anjannath mentioned this pull request Oct 21, 2020

running out of disk quickly with default 32GB #1608

Closed

guillaumerose reviewed Oct 21, 2020

View reviewed changes

gbraad reviewed Oct 21, 2020

View reviewed changes

cmd/crc/cmd/start.go Outdated Show resolved Hide resolved

gbraad reviewed Oct 21, 2020

View reviewed changes

pkg/crc/validation/validation.go Outdated

if value == 0 {

return nil

}

if value < constants.DefaultDiskSize {

Copy link

Contributor

gbraad Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:+1

cfergeau force-pushed the disk-resize branch from dd0642d to 77da319 Compare October 21, 2020 07:56

praveenkumar reviewed Oct 21, 2020

View reviewed changes

gbraad approved these changes Oct 21, 2020

View reviewed changes

openshift-ci-robot assigned gbraad Oct 21, 2020

openshift-ci-robot added the lgtm label Oct 21, 2020

cfergeau force-pushed the disk-resize branch from 77da319 to 8433ebe Compare October 21, 2020 17:38

openshift-ci-robot removed the lgtm label Oct 21, 2020

cfergeau force-pushed the disk-resize branch 2 times, most recently from 278603c to 03feab6 Compare October 26, 2020 13:28

guillaumerose reviewed Oct 27, 2020

View reviewed changes

cfergeau force-pushed the disk-resize branch 2 times, most recently from 1cd5c47 to bc621e3 Compare October 29, 2020 15:29

cfergeau added 3 commits October 30, 2020 10:05

libvirt: Add storage pool support

bb1efee

In order to be able to support disk image resizing in the libvirt machine driver (through virStorageVolResize), we need to use libvirt storage pools. This commit sets the name of the storage pool that the machine driver should be using.

cfergeau force-pushed the disk-resize branch from 5ab0673 to fe354ab Compare October 30, 2020 09:05

guillaumerose approved these changes Oct 30, 2020

View reviewed changes

openshift-ci-robot assigned guillaumerose Oct 30, 2020

openshift-ci-robot added the lgtm label Oct 30, 2020

guillaumerose merged commit b4a6188 into crc-org:master Oct 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disk resize #1607

Disk resize #1607

cfergeau commented Oct 20, 2020 •

edited

Loading

guillaumerose Oct 21, 2020

praveenkumar Oct 21, 2020

cfergeau Oct 21, 2020

cfergeau Oct 21, 2020

cfergeau Oct 21, 2020

guillaumerose Oct 22, 2020

cfergeau Oct 26, 2020

guillaumerose Oct 27, 2020

gbraad Oct 21, 2020

praveenkumar Oct 21, 2020

praveenkumar Oct 21, 2020

gbraad commented Oct 21, 2020

cfergeau commented Oct 21, 2020

cfergeau commented Oct 26, 2020

guillaumerose Oct 27, 2020

cfergeau Oct 28, 2020

praveenkumar Oct 29, 2020

cfergeau Oct 29, 2020

cfergeau Oct 29, 2020

guillaumerose Oct 27, 2020

openshift-merge-robot commented Oct 29, 2020

openshift-ci-robot commented Oct 30, 2020

Disk resize #1607

Disk resize #1607

Conversation

cfergeau commented Oct 20, 2020 • edited Loading

Solution/Idea

Proposed changes

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad commented Oct 21, 2020

cfergeau commented Oct 21, 2020

cfergeau commented Oct 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Oct 29, 2020

openshift-ci-robot commented Oct 30, 2020

cfergeau commented Oct 20, 2020 •

edited

Loading