Skip to content
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.

Sign up for GitHub

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

Disk resize #1607

Merged
merged 3 commits into from
Oct 30, 2020
Merged

Disk resize #1607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1f4558
start: Add support for disk resizing
cfergeau Aug 10, 2020
bb1efee
libvirt: Add storage pool support
cfergeau Sep 30, 2020
fe354ab
machine: Display warning when UpdateConfigRaw returns ErrNotImplemented
cfergeau Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/crc/cmd/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const (
Bundle = "bundle"
CPUs = "cpus"
Memory = "memory"
DiskSize = "disk-size"
NameServer = "nameserver"
PullSecretFile = "pull-secret-file"
DisableUpdateCheck = "disable-update-check"
Expand All @@ -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, constants.DefaultDiskSize, config.ValidateDiskSize, config.RequiresRestartMsg)
cfg.AddSetting(NameServer, "", config.ValidateIPAddress, config.SuccessfullyApplied)
cfg.AddSetting(PullSecretFile, "", config.ValidatePath, config.SuccessfullyApplied)
cfg.AddSetting(DisableUpdateCheck, false, config.ValidateBool, config.SuccessfullyApplied)
Expand Down
5 changes: 5 additions & 0 deletions cmd/crc/cmd/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ func init() {
flagSet.StringP(cmdConfig.PullSecretFile, "p", "", fmt.Sprintf("File path of image pull secret (download from %s)", constants.CrcLandingPageURL))
flagSet.IntP(cmdConfig.CPUs, "c", constants.DefaultCPUs, "Number of CPU cores to allocate to the OpenShift cluster")
flagSet.IntP(cmdConfig.Memory, "m", constants.DefaultMemory, "MiB of memory to allocate to the OpenShift cluster")
flagSet.UintP(cmdConfig.DiskSize, "d", constants.DefaultDiskSize, "Total size in GiB of the disk used by the OpenShift cluster")
flagSet.StringP(cmdConfig.NameServer, "n", "", "IPv4 address of nameserver to use for the OpenShift cluster")
flagSet.Bool(cmdConfig.DisableUpdateCheck, false, "Don't check for update")

Expand Down Expand Up @@ -65,6 +66,7 @@ func runStart(arguments []string) (*machine.StartResult, error) {
startConfig := machine.StartConfig{
BundlePath: config.Get(cmdConfig.Bundle).AsString(),
Memory: config.Get(cmdConfig.Memory).AsInt(),
DiskSize: config.Get(cmdConfig.DiskSize).AsInt(),
CPUs: config.Get(cmdConfig.CPUs).AsInt(),
NameServer: config.Get(cmdConfig.NameServer).AsString(),
PullSecret: &cluster.PullSecret{
Expand Down Expand Up @@ -161,6 +163,9 @@ func validateStartFlags() error {
if err := validation.ValidateCPUs(config.Get(cmdConfig.CPUs).AsInt()); err != nil {
return err
}
if err := validation.ValidateDiskSize(config.Get(cmdConfig.DiskSize).AsInt()); err != nil {
return err
}
if err := validation.ValidateBundle(config.Get(cmdConfig.Bundle).AsString()); err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ require (
github.com/cavaliercoder/grab v2.0.0+incompatible
github.com/cheggaaa/pb/v3 v3.0.4
github.com/code-ready/clicumber v0.0.0-20200728062640-1203dda97f67
github.com/code-ready/machine v0.0.0-20201008083613-1420a5d7f1e2
github.com/code-ready/machine v0.0.0-20201022100236-4405fef4d0cc
github.com/cucumber/godog v0.9.0
github.com/cucumber/messages-go/v10 v10.0.3
github.com/docker/docker v1.13.1 // indirect
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ github.com/cockroachdb/cockroach-go v0.0.0-20181001143604-e0a95dfd547c/go.mod h1
github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa/go.mod h1:zn76sxSg3SzpJ0PPJaLDCu+Bu0Lg3sKTORVIj19EIF8=
github.com/code-ready/clicumber v0.0.0-20200728062640-1203dda97f67 h1:C+EVPWw8sjASF/529T8BMddoACTb+ruHB8yrZoYscek=
github.com/code-ready/clicumber v0.0.0-20200728062640-1203dda97f67/go.mod h1:ZPZUcYGpv/FQGnwakUuhiHP9x/IIcU2SKbn8xPe4ROc=
github.com/code-ready/machine v0.0.0-20201008083613-1420a5d7f1e2 h1:qfXZn0/voilXKONlr3dGv6Ctk3YtrelK3r2v9plfO1o=
github.com/code-ready/machine v0.0.0-20201008083613-1420a5d7f1e2/go.mod h1:g30jKsf0FV9yJjsqZFAuFzVxwJE2h5cqDXSuMUV1G/U=
github.com/code-ready/machine v0.0.0-20201022100236-4405fef4d0cc h1:9Tp3sGd10JVbS2OhRmsxtsRytRbZoS6akW7DR8hk6Jw=
github.com/code-ready/machine v0.0.0-20201022100236-4405fef4d0cc/go.mod h1:g30jKsf0FV9yJjsqZFAuFzVxwJE2h5cqDXSuMUV1G/U=
github.com/codegangsta/negroni v1.0.0/go.mod h1:v0y3T5G7Y1UlFfyxFn/QLRU4a2EuNau2iZY63YTKWo0=
github.com/container-storage-interface/spec v1.2.0/go.mod h1:6URME8mwIBbpVyZV93Ce5St17xBiQJQY67NDsuohiy4=
github.com/containerd/cgroups v0.0.0-20190919134610-bf292b21730f/go.mod h1:OApqhQ4XNSNC13gXIwDjhOQxjWa/NxkwZXJ1EvqT0ko=
Expand Down
13 changes: 13 additions & 0 deletions pkg/crc/config/validations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@ func ValidateBool(value interface{}) (bool, string) {
return false, "must be true or false"
}

// ValidateDiskSize checks if provided disk size is valid in the config
func ValidateDiskSize(value interface{}) (bool, string) {
diskSize, err := cast.ToIntE(value)
if err != nil {
return false, fmt.Sprintf("could not convert '%s' to integer", value)
}
if err := validation.ValidateDiskSize(diskSize); err != nil {
return false, err.Error()
}

return true, ""
}

// ValidateCPUs checks if provided cpus count is valid in the config
func ValidateCPUs(value interface{}) (bool, string) {
v, err := cast.ToIntE(value)
Expand Down
7 changes: 4 additions & 3 deletions pkg/crc/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import (
)

const (
DefaultName = "crc"
DefaultCPUs = 4
DefaultMemory = 9216
DefaultName = "crc"
DefaultCPUs = 4
DefaultMemory = 9216
DefaultDiskSize = 31

DefaultSSHUser = "core"
DefaultSSHPort = 22
Expand Down
12 changes: 12 additions & 0 deletions pkg/crc/machine/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,15 @@ func setVcpus(host *host.Host, vcpus int) error {

return updateDriverValue(host, vcpuSetter)
}

func convertGiBToBytes(gib int) uint64 {
return uint64(gib) * 1024 * 1024 * 1024
}

func setDiskSize(host *host.Host, diskSizeGiB int) error {
diskSizeSetter := func(driver *libmachine.VMDriver) {
driver.DiskCapacity = convertGiBToBytes(diskSizeGiB)
}

return updateDriverValue(host, diskSizeSetter)
}
3 changes: 2 additions & 1 deletion pkg/crc/machine/libvirt/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import "fmt"

const (
// Defaults
DefaultNetwork = "crc"
DefaultNetwork = "crc"
DefaultStoragePool = "crc"

// Static addresses
MACAddress = "52:fd:fc:07:21:82"
Expand Down
1 change: 1 addition & 0 deletions pkg/crc/machine/libvirt/driver_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ func CreateHost(machineConfig config.MachineConfig) *libvirt.Driver {
config.InitVMDriverFromMachineConfig(machineConfig, libvirtDriver.VMDriver)

libvirtDriver.Network = DefaultNetwork
libvirtDriver.StoragePool = DefaultStoragePool

return libvirtDriver
}
28 changes: 26 additions & 2 deletions pkg/crc/machine/machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,20 +114,39 @@ func (client *client) updateVMConfig(startConfig StartConfig, api libmachine.API
logging.Debugf("Updating CRC VM configuration")
if err := setMemory(host, startConfig.Memory); err != nil {
logging.Debugf("Failed to update CRC VM configuration: %v", err)
if err != drivers.ErrNotImplemented {
if err == drivers.ErrNotImplemented {
logging.Warn("Memory configuration change has been ignored as the machine driver does not support it")
} else {
return err
}
}
if err := setVcpus(host, startConfig.CPUs); err != nil {
logging.Debugf("Failed to update CRC VM configuration: %v", err)
if err != drivers.ErrNotImplemented {
if err == drivers.ErrNotImplemented {
logging.Warn("CPU configuration change has been ignored as the machine driver does not support it")
} else {
return err
}
}
if err := api.Save(host); err != nil {
return err
}

/* Disk size */
if startConfig.DiskSize != constants.DefaultDiskSize {
if err := setDiskSize(host, startConfig.DiskSize); err != nil {
logging.Debugf("Failed to update CRC disk configuration: %v", err)
if err == drivers.ErrNotImplemented {
logging.Warn("Disk size configuration change has been ignored as the machine driver does not support it")
} else {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is where macOS user will currently fall ?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}
if err := api.Save(host); err != nil {
return err
}
}

return nil
}

Expand Down Expand Up @@ -262,6 +281,11 @@ func (client *client) Start(startConfig StartConfig) (*StartResult, error) {
return nil, errors.Wrap(err, "Error updating public key")
}

// Trigger disk resize, this will be a no-op if no disk size change is needed
if _, err = sshRunner.Run("sudo xfs_growfs / >/dev/null"); err != nil {
return nil, errors.Wrap(err, "Error updating filesystem size")
}

// Start network time synchronization if `CRC_DEBUG_ENABLE_STOP_NTP` is not set
if stopNtp, _ := strconv.ParseBool(os.Getenv("CRC_DEBUG_ENABLE_STOP_NTP")); !stopNtp {
logging.Info("Starting network time synchronization in CodeReady Containers VM")
Expand Down
5 changes: 3 additions & 2 deletions pkg/crc/machine/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ type StartConfig struct {
BundlePath string

// Hypervisor
Memory int
CPUs int
Memory int // Memory size in MiB
CPUs int
DiskSize int // Disk size in GiB

// Nameserver
NameServer string
Expand Down
7 changes: 7 additions & 0 deletions pkg/crc/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@ func ValidateMemory(value int) error {
return ValidateEnoughMemory(value)
}

func ValidateDiskSize(value int) error {
if value <= constants.DefaultDiskSize {
return fmt.Errorf("requires disk size in GiB >= %d", constants.DefaultDiskSize)
}
return nil
}

// ValidateEnoughMemory checks if enough memory is installed on the host
func ValidateEnoughMemory(value int) error {
totalMemory := memory.TotalMemory()
Expand Down
1 change: 1 addition & 0 deletions vendor/github.com/code-ready/machine/drivers/hyperkit/driver_darwin.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 18 additions & 3 deletions vendor/github.com/code-ready/machine/drivers/hyperv/hyperv.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 5 additions & 4 deletions vendor/github.com/code-ready/machine/drivers/libvirt/driver_linux.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions vendor/github.com/code-ready/machine/libmachine/drivers/base.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ github.com/cheggaaa/pb/v3/termutil
# github.com/code-ready/clicumber v0.0.0-20200728062640-1203dda97f67
github.com/code-ready/clicumber/testsuite
github.com/code-ready/clicumber/util
# github.com/code-ready/machine v0.0.0-20201008083613-1420a5d7f1e2
# github.com/code-ready/machine v0.0.0-20201022100236-4405fef4d0cc
github.com/code-ready/machine/drivers/errdriver
github.com/code-ready/machine/drivers/hyperkit
github.com/code-ready/machine/drivers/hyperv
Expand Down