From 85f2a11d1bf6fa9dff8ef503fe2ed445bad1872d Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Wed, 6 Nov 2024 12:21:06 +0530 Subject: [PATCH 1/5] Add VirtualMachine interface to abstract out crc virtual machine interaction Add a VirtualMachine interface and make the CRC `machine` package client use the VirtualMachine interface instead of a concrete implementation. This way we can inject a dummy test FakeVirtualMachine implementation into client tests that can ease writing tests for this package. - Add some additional methods in VirtualMachine interface so that we can replace direct usage of struct fields with interface methods - `Bundle()` - `Driver()` - `API()` - `Host()` - `Kill()` --- pkg/crc/machine/console.go | 2 +- pkg/crc/machine/generate_bundle.go | 4 +-- pkg/crc/machine/ip.go | 2 +- pkg/crc/machine/start.go | 46 ++++++++++++------------- pkg/crc/machine/status.go | 18 +++++----- pkg/crc/machine/stop.go | 2 +- pkg/crc/machine/virtualmachine.go | 54 ++++++++++++++++++++++++++---- 7 files changed, 84 insertions(+), 44 deletions(-) diff --git a/pkg/crc/machine/console.go b/pkg/crc/machine/console.go index e372e4c9ba..bc6dc2c268 100644 --- a/pkg/crc/machine/console.go +++ b/pkg/crc/machine/console.go @@ -21,7 +21,7 @@ func (client *client) GetConsoleURL() (*types.ConsoleResult, error) { return nil, errors.Wrap(err, "Error getting the state for virtual machine") } - clusterConfig, err := getClusterConfig(vm.bundle) + clusterConfig, err := getClusterConfig(vm.Bundle()) if err != nil { return nil, errors.Wrap(err, "Error loading cluster configuration") } diff --git a/pkg/crc/machine/generate_bundle.go b/pkg/crc/machine/generate_bundle.go index 4cc05baf8d..c3e112ef4b 100644 --- a/pkg/crc/machine/generate_bundle.go +++ b/pkg/crc/machine/generate_bundle.go @@ -111,7 +111,7 @@ func loadVM(client *client) (*bundle.CrcBundleInfo, *crcssh.Runner, error) { } defer vm.Close() - currentState, err := vm.Driver.GetState() + currentState, err := vm.Driver().GetState() if err != nil { return nil, nil, errors.Wrap(err, "Cannot get machine state") } @@ -124,5 +124,5 @@ func loadVM(client *client) (*bundle.CrcBundleInfo, *crcssh.Runner, error) { return nil, nil, errors.Wrap(err, "Error creating the ssh client") } - return vm.bundle, sshRunner, nil + return vm.Bundle(), sshRunner, nil } diff --git a/pkg/crc/machine/ip.go b/pkg/crc/machine/ip.go index f03b2a8d15..088bdd76fa 100644 --- a/pkg/crc/machine/ip.go +++ b/pkg/crc/machine/ip.go @@ -21,6 +21,6 @@ func (client *client) ConnectionDetails() (*types.ConnectionDetails, error) { IP: ip, SSHPort: vm.SSHPort(), SSHUsername: constants.DefaultSSHUser, - SSHKeys: []string{constants.GetPrivateKeyPath(), constants.GetECDSAPrivateKeyPath(), vm.bundle.GetSSHKeyPath()}, + SSHKeys: []string{constants.GetPrivateKeyPath(), constants.GetECDSAPrivateKeyPath(), vm.Bundle().GetSSHKeyPath()}, }, nil } diff --git a/pkg/crc/machine/start.go b/pkg/crc/machine/start.go index cad83b8aea..eb1ba080a3 100644 --- a/pkg/crc/machine/start.go +++ b/pkg/crc/machine/start.go @@ -60,10 +60,10 @@ func getCrcBundleInfo(preset crcPreset.Preset, bundleName, bundlePath string, en return bundle.Use(bundleName) } -func (client *client) updateVMConfig(startConfig types.StartConfig, vm *virtualMachine) error { +func (client *client) updateVMConfig(startConfig types.StartConfig, vm VirtualMachine) error { /* Memory */ logging.Debugf("Updating CRC VM configuration") - if err := setMemory(vm.Host, startConfig.Memory); err != nil { + if err := setMemory(vm.Host(), startConfig.Memory); err != nil { logging.Debugf("Failed to update CRC VM configuration: %v", err) if err == drivers.ErrNotImplemented { logging.Warn("Memory configuration change has been ignored as the machine driver does not support it") @@ -71,7 +71,7 @@ func (client *client) updateVMConfig(startConfig types.StartConfig, vm *virtualM return err } } - if err := setVcpus(vm.Host, startConfig.CPUs); err != nil { + if err := setVcpus(vm.Host(), startConfig.CPUs); err != nil { logging.Debugf("Failed to update CRC VM configuration: %v", err) if err == drivers.ErrNotImplemented { logging.Warn("CPU configuration change has been ignored as the machine driver does not support it") @@ -79,13 +79,13 @@ func (client *client) updateVMConfig(startConfig types.StartConfig, vm *virtualM return err } } - if err := vm.api.Save(vm.Host); err != nil { + if err := vm.API().Save(vm.Host()); err != nil { return err } /* Disk size */ if startConfig.DiskSize != constants.DefaultDiskSize { - if err := setDiskSize(vm.Host, startConfig.DiskSize); err != nil { + if err := setDiskSize(vm.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") @@ -93,7 +93,7 @@ func (client *client) updateVMConfig(startConfig types.StartConfig, vm *virtualM return err } } - if err := vm.api.Save(vm.Host); err != nil { + if err := vm.API().Save(vm.Host()); err != nil { return err } } @@ -101,7 +101,7 @@ func (client *client) updateVMConfig(startConfig types.StartConfig, vm *virtualM // we want to set the shared dir password on-the-fly to be used // we do not want this value to be persisted to disk if startConfig.SharedDirPassword != "" { - if err := setSharedDirPassword(vm.Host, startConfig.SharedDirPassword); err != nil { + if err := setSharedDirPassword(vm.Host(), startConfig.SharedDirPassword); err != nil { return fmt.Errorf("Failed to set shared dir password: %w", err) } } @@ -206,9 +206,9 @@ func growLVForMicroshift(sshRunner *crcssh.Runner, lvFullName string, rootPart s return nil } -func configureSharedDirs(vm *virtualMachine, sshRunner *crcssh.Runner) error { +func configureSharedDirs(vm VirtualMachine, sshRunner *crcssh.Runner) error { logging.Debugf("Configuring shared directories") - sharedDirs, err := vm.Driver.GetSharedDirs() + sharedDirs, err := vm.Driver().GetSharedDirs() if err != nil { // the libvirt machine driver uses net/rpc, which wraps errors // in rpc.ServerError, but without using golang 1.13 error @@ -334,7 +334,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) } defer vm.Close() - currentBundleName := vm.bundle.GetBundleName() + currentBundleName := vm.Bundle().GetBundleName() if currentBundleName != bundleName { logging.Debugf("Bundle '%s' was requested, but the existing VM is using '%s'", bundleName, currentBundleName) @@ -347,8 +347,8 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) return nil, errors.Wrap(err, "Error getting the machine state") } if vmState == state.Running { - logging.Infof("A CRC VM for %s %s is already running", startConfig.Preset.ForDisplay(), vm.bundle.GetVersion()) - clusterConfig, err := getClusterConfig(vm.bundle) + logging.Infof("A CRC VM for %s %s is already running", startConfig.Preset.ForDisplay(), vm.Bundle().GetVersion()) + clusterConfig, err := getClusterConfig(vm.Bundle()) if err != nil { return nil, errors.Wrap(err, "Cannot create cluster configuration") } @@ -365,7 +365,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) return nil, err } - logging.Infof("Starting CRC VM for %s %s...", startConfig.Preset, vm.bundle.GetVersion()) + logging.Infof("Starting CRC VM for %s %s...", startConfig.Preset, vm.Bundle().GetVersion()) if client.useVSock() { if err := exposePorts(startConfig.Preset, startConfig.IngressHTTPPort, startConfig.IngressHTTPSPort); err != nil { @@ -457,7 +457,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) return nil, errors.Wrap(err, "Failed to change permissions to root podman socket") } - proxyConfig, err := getProxyConfig(vm.bundle) + proxyConfig, err := getProxyConfig(vm.Bundle()) if err != nil { return nil, errors.Wrap(err, "Error getting proxy configuration") } @@ -472,7 +472,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) SSHRunner: sshRunner, IP: instanceIP, // TODO: should be more finegrained - BundleMetadata: *vm.bundle, + BundleMetadata: *vm.Bundle(), NetworkMode: client.networkMode(), } @@ -502,7 +502,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) logging.Warn(fmt.Sprintf("Failed to query DNS from host: %v", err)) } - if vm.bundle.IsMicroshift() { + if vm.Bundle().IsMicroshift() { // ************************** // END OF MICROSHIFT START CODE // ************************** @@ -547,7 +547,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) ocConfig := oc.UseOCWithSSH(sshRunner) if err := cluster.ApproveCSRAndWaitForCertsRenewal(ctx, sshRunner, ocConfig, certsExpired[cluster.KubeletClientCert], certsExpired[cluster.KubeletServerCert], certsExpired[cluster.AggregatorClientCert]); err != nil { - logBundleDate(vm.bundle) + logBundleDate(vm.Bundle()) return nil, errors.Wrap(err, "Failed to renew TLS certificates: please check if a newer CRC release is available") } @@ -596,7 +596,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) } } - if err := updateKubeconfig(ctx, ocConfig, sshRunner, vm.bundle.GetKubeConfigPath()); err != nil { + if err := updateKubeconfig(ctx, ocConfig, sshRunner, vm.Bundle().GetKubeConfigPath()); err != nil { return nil, errors.Wrap(err, "Failed to update kubeconfig file") } @@ -607,7 +607,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) waitForProxyPropagation(ctx, ocConfig, proxyConfig) - clusterConfig, err := getClusterConfig(vm.bundle) + clusterConfig, err := getClusterConfig(vm.Bundle()) if err != nil { return nil, errors.Wrap(err, "Cannot get cluster configuration") } @@ -694,17 +694,17 @@ func createHost(machineConfig config.MachineConfig, preset crcPreset.Preset) err return nil } -func startHost(ctx context.Context, vm *virtualMachine) error { - if err := vm.Driver.Start(); err != nil { +func startHost(ctx context.Context, vm VirtualMachine) error { + if err := vm.Driver().Start(); err != nil { return fmt.Errorf("Error in driver during machine start: %s", err) } - if err := vm.api.Save(vm.Host); err != nil { + if err := vm.API().Save(vm.Host()); err != nil { return fmt.Errorf("Error saving virtual machine to store after attempting creation: %s", err) } logging.Debug("Waiting for machine to be running, this may take a few minutes...") - if err := crcerrors.Retry(ctx, 3*time.Minute, host.MachineInState(vm.Driver, libmachinestate.Running), 3*time.Second); err != nil { + if err := crcerrors.Retry(ctx, 3*time.Minute, host.MachineInState(vm.Driver(), libmachinestate.Running), 3*time.Second); err != nil { return fmt.Errorf("Error waiting for machine to be running: %s", err) } diff --git a/pkg/crc/machine/status.go b/pkg/crc/machine/status.go index 7dbad231a1..37aed4123f 100644 --- a/pkg/crc/machine/status.go +++ b/pkg/crc/machine/status.go @@ -37,13 +37,13 @@ func (client *client) Status() (*types.ClusterStatusResult, error) { CrcStatus: vmStatus, } switch { - case vm.bundle.IsMicroshift(): + case vm.Bundle().IsMicroshift(): clusterStatusResult.OpenshiftStatus = types.OpenshiftStopped - clusterStatusResult.OpenshiftVersion = vm.bundle.GetVersion() + clusterStatusResult.OpenshiftVersion = vm.Bundle().GetVersion() clusterStatusResult.Preset = preset.Microshift default: clusterStatusResult.OpenshiftStatus = types.OpenshiftStopped - clusterStatusResult.OpenshiftVersion = vm.bundle.GetVersion() + clusterStatusResult.OpenshiftVersion = vm.Bundle().GetVersion() clusterStatusResult.Preset = preset.OpenShift } @@ -62,10 +62,10 @@ func (client *client) Status() (*types.ClusterStatusResult, error) { clusterStatusResult.DiskSize = diskSize switch { - case vm.bundle.IsMicroshift(): + case vm.Bundle().IsMicroshift(): clusterStatusResult.OpenshiftStatus = getMicroShiftStatus(context.Background(), ip) clusterStatusResult.PersistentVolumeUse, clusterStatusResult.PersistentVolumeSize = client.getPVCSize(vm) - case vm.bundle.IsOpenShift(): + case vm.Bundle().IsOpenShift(): clusterStatusResult.OpenshiftStatus = getOpenShiftStatus(context.Background(), ip) } @@ -112,7 +112,7 @@ func (client *client) GetClusterLoad() (*types.ClusterLoadResult, error) { }, nil } -func (client *client) getDiskDetails(vm *virtualMachine) (int64, int64) { +func (client *client) getDiskDetails(vm VirtualMachine) (int64, int64) { disk, err, _ := client.diskDetails.Memoize("disks", func() (interface{}, error) { sshRunner, err := vm.SSHRunner() if err != nil { @@ -162,7 +162,7 @@ func getStatus(status *cluster.Status) types.OpenshiftStatus { return types.OpenshiftStopped } -func (client *client) getRAMStatus(vm *virtualMachine) (int64, int64) { +func (client *client) getRAMStatus(vm VirtualMachine) (int64, int64) { ram, err, _ := client.ramDetails.Memoize("ram", func() (interface{}, error) { sshRunner, err := vm.SSHRunner() if err != nil { @@ -184,7 +184,7 @@ func (client *client) getRAMStatus(vm *virtualMachine) (int64, int64) { return ram.([]int64)[0], ram.([]int64)[1] } -func (client *client) getCPUStatus(vm *virtualMachine) []int64 { +func (client *client) getCPUStatus(vm VirtualMachine) []int64 { sshRunner, err := vm.SSHRunner() if err != nil { logging.Debugf("Cannot get SSH runner: %v", err) @@ -202,7 +202,7 @@ func (client *client) getCPUStatus(vm *virtualMachine) []int64 { } -func (client *client) getPVCSize(vm *virtualMachine) (int, int) { +func (client *client) getPVCSize(vm VirtualMachine) (int, int) { sshRunner, err := vm.SSHRunner() if err != nil { logging.Debugf("Cannot get SSH runner: %v", err) diff --git a/pkg/crc/machine/stop.go b/pkg/crc/machine/stop.go index 549188e241..ece7519232 100644 --- a/pkg/crc/machine/stop.go +++ b/pkg/crc/machine/stop.go @@ -54,7 +54,7 @@ func (client *client) Stop() (state.State, error) { // is fixed. We should also ignore the openshift specific errors because stop // operation shouldn't depend on the openshift side. Without this graceful shutdown // takes around 6-7 mins. -func stopAllContainers(vm *virtualMachine) error { +func stopAllContainers(vm VirtualMachine) error { logging.Info("Stopping kubelet and all containers...") sshRunner, err := vm.SSHRunner() if err != nil { diff --git a/pkg/crc/machine/virtualmachine.go b/pkg/crc/machine/virtualmachine.go index ff9c89eae2..fa664d8f17 100644 --- a/pkg/crc/machine/virtualmachine.go +++ b/pkg/crc/machine/virtualmachine.go @@ -9,12 +9,28 @@ import ( "github.com/crc-org/crc/v2/pkg/crc/ssh" "github.com/crc-org/crc/v2/pkg/libmachine" libmachinehost "github.com/crc-org/crc/v2/pkg/libmachine/host" + "github.com/crc-org/machine/libmachine/drivers" "github.com/pkg/errors" ) +type VirtualMachine interface { + Close() error + Remove() error + State() (state.State, error) + IP() (string, error) + SSHPort() int + SSHRunner() (*ssh.Runner, error) + Kill() error + Stop() error + Bundle() *bundle.CrcBundleInfo + Driver() drivers.Driver + API() libmachine.API + Host() *libmachinehost.Host +} + type virtualMachine struct { - name string - *libmachinehost.Host + name string + host *libmachinehost.Host bundle *bundle.CrcBundleInfo api libmachine.API vsock bool @@ -34,7 +50,7 @@ func (err *MissingHostError) Error() string { var errInvalidBundleMetadata = errors.New("Error loading bundle metadata") -func loadVirtualMachine(name string, useVSock bool) (*virtualMachine, error) { +func loadVirtualMachine(name string, useVSock bool) (VirtualMachine, error) { apiClient := libmachine.NewClient(constants.MachineBaseDir) exists, err := apiClient.Exists(name) if err != nil { @@ -56,7 +72,7 @@ func loadVirtualMachine(name string, useVSock bool) (*virtualMachine, error) { return &virtualMachine{ name: name, - Host: libmachineHost, + host: libmachineHost, bundle: crcBundleMetadata, api: apiClient, vsock: useVSock, @@ -68,7 +84,7 @@ func (vm *virtualMachine) Close() error { } func (vm *virtualMachine) Remove() error { - if err := vm.Driver.Remove(); err != nil { + if err := vm.Driver().Remove(); err != nil { return errors.Wrap(err, "Driver cannot remove machine") } @@ -80,7 +96,7 @@ func (vm *virtualMachine) Remove() error { } func (vm *virtualMachine) State() (state.State, error) { - vmStatus, err := vm.Driver.GetState() + vmStatus, err := vm.Driver().GetState() if err != nil { return state.Error, err } @@ -91,7 +107,7 @@ func (vm *virtualMachine) IP() (string, error) { if vm.vsock { return "127.0.0.1", nil } - return vm.Driver.GetIP() + return vm.Driver().GetIP() } func (vm *virtualMachine) SSHPort() int { @@ -108,3 +124,27 @@ func (vm *virtualMachine) SSHRunner() (*ssh.Runner, error) { } return ssh.CreateRunner(ip, vm.SSHPort(), constants.GetPrivateKeyPath(), constants.GetECDSAPrivateKeyPath(), vm.bundle.GetSSHKeyPath()) } + +func (vm *virtualMachine) Bundle() *bundle.CrcBundleInfo { + return vm.bundle +} + +func (vm *virtualMachine) Driver() drivers.Driver { + return vm.host.Driver +} + +func (vm *virtualMachine) API() libmachine.API { + return vm.api +} + +func (vm *virtualMachine) Host() *libmachinehost.Host { + return vm.host +} + +func (vm *virtualMachine) Kill() error { + return vm.host.Kill() +} + +func (vm *virtualMachine) Stop() error { + return vm.host.Stop() +} From 743a97e6996e95cc8872a9861b18d17b8393b931 Mon Sep 17 00:00:00 2001 From: Christophe Fergeau Date: Wed, 6 Nov 2024 16:51:02 +0100 Subject: [PATCH 2/5] allow to provide fake vm implementation Introduce a new constructor method `newClientWithVirtualMachine` in machine client that would have an additional VirtualMachine argument, this would be kept package private so that it's used only by tests in the same package. --- pkg/crc/machine/client.go | 25 +++++++++++++++++-------- pkg/crc/machine/start.go | 2 +- pkg/crc/machine/stop.go | 2 +- pkg/crc/machine/virtualmachine.go | 7 +++++++ 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/pkg/crc/machine/client.go b/pkg/crc/machine/client.go index 5c7ba59bb8..093295febd 100644 --- a/pkg/crc/machine/client.go +++ b/pkg/crc/machine/client.go @@ -30,21 +30,30 @@ type Client interface { } type client struct { - name string - debug bool - config crcConfig.Storage + name string + debug bool + config crcConfig.Storage + virtualMachine VirtualMachine diskDetails *memoize.Memoizer ramDetails *memoize.Memoizer } func NewClient(name string, debug bool, config crcConfig.Storage) Client { + return newClientWithVirtualMachine(name, debug, config, nil) +} + +// newClientWithVirtualMachine creates a Client instance with an overridden VirtualMachine implementation. +// It would not create a new VirtualMachine object. This method is primarily created for usage in tests so +// that we can pass a fake VirtualMachine implementation. +func newClientWithVirtualMachine(name string, debug bool, config crcConfig.Storage, vm VirtualMachine) Client { return &client{ - name: name, - debug: debug, - config: config, - diskDetails: memoize.NewMemoizer(time.Minute, 5*time.Minute), - ramDetails: memoize.NewMemoizer(30*time.Second, 2*time.Minute), + name: name, + debug: debug, + config: config, + diskDetails: memoize.NewMemoizer(time.Minute, 5*time.Minute), + ramDetails: memoize.NewMemoizer(30*time.Second, 2*time.Minute), + virtualMachine: vm, } } diff --git a/pkg/crc/machine/start.go b/pkg/crc/machine/start.go index eb1ba080a3..e573784487 100644 --- a/pkg/crc/machine/start.go +++ b/pkg/crc/machine/start.go @@ -625,7 +625,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) } func (client *client) IsRunning() (bool, error) { - vm, err := loadVirtualMachine(client.name, client.useVSock()) + vm, err := loadVirtualMachineLazily(client.virtualMachine, client.name, client.useVSock()) if err != nil { return false, errors.Wrap(err, "Cannot load machine") } diff --git a/pkg/crc/machine/stop.go b/pkg/crc/machine/stop.go index ece7519232..f25a2e18f8 100644 --- a/pkg/crc/machine/stop.go +++ b/pkg/crc/machine/stop.go @@ -20,7 +20,7 @@ func (client *client) Stop() (state.State, error) { if running, _ := client.IsRunning(); !running { return state.Error, errors.New("Instance is already stopped") } - vm, err := loadVirtualMachine(client.name, client.useVSock()) + vm, err := loadVirtualMachineLazily(client.virtualMachine, client.name, client.useVSock()) if err != nil { return state.Error, errors.Wrap(err, "Cannot load machine") } diff --git a/pkg/crc/machine/virtualmachine.go b/pkg/crc/machine/virtualmachine.go index fa664d8f17..03a6b0e60f 100644 --- a/pkg/crc/machine/virtualmachine.go +++ b/pkg/crc/machine/virtualmachine.go @@ -50,6 +50,13 @@ func (err *MissingHostError) Error() string { var errInvalidBundleMetadata = errors.New("Error loading bundle metadata") +func loadVirtualMachineLazily(vm VirtualMachine, name string, useVSock bool) (VirtualMachine, error) { + if vm != nil { + return vm, nil + } + return loadVirtualMachine(name, useVSock) +} + func loadVirtualMachine(name string, useVSock bool) (VirtualMachine, error) { apiClient := libmachine.NewClient(constants.MachineBaseDir) exists, err := apiClient.Exists(name) From 58b7f9fb0c6f4a6d7d73287bb0f52767d460cbf9 Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Wed, 6 Nov 2024 12:22:18 +0530 Subject: [PATCH 3/5] test : Add a dummy VirtualMachine implementation for use in tests Add FakeVirtualMachine sturct in `fakemachine` for adding dummy implementation for `VirtualMachine` interface. Currently, I've only completed methods used by `stop_test.go`, I'll add more in small increments as we implement more unit tests using this implementation. Signed-off-by: Rohan Kumar --- pkg/crc/machine/fakemachine/virtualmachine.go | 114 ++++++++++++++++++ pkg/crc/ssh/ssh.go | 6 + 2 files changed, 120 insertions(+) create mode 100644 pkg/crc/machine/fakemachine/virtualmachine.go diff --git a/pkg/crc/machine/fakemachine/virtualmachine.go b/pkg/crc/machine/fakemachine/virtualmachine.go new file mode 100644 index 0000000000..d19b521c37 --- /dev/null +++ b/pkg/crc/machine/fakemachine/virtualmachine.go @@ -0,0 +1,114 @@ +package fakemachine + +import ( + "errors" + + "github.com/crc-org/crc/v2/pkg/crc/machine/bundle" + "github.com/crc-org/crc/v2/pkg/crc/machine/state" + "github.com/crc-org/crc/v2/pkg/crc/ssh" + "github.com/crc-org/crc/v2/pkg/libmachine" + libmachinehost "github.com/crc-org/crc/v2/pkg/libmachine/host" + "github.com/crc-org/machine/libmachine/drivers" +) + +type FakeSSHClient struct { + LastExecutedCommand string + IsSSHClientClosed bool +} + +func (client *FakeSSHClient) Run(command string) ([]byte, []byte, error) { + client.LastExecutedCommand = command + return []byte("test"), []byte("test"), nil +} + +func (client *FakeSSHClient) Close() { + client.IsSSHClientClosed = true +} + +func NewFakeSSHClient() *FakeSSHClient { + return &FakeSSHClient{ + LastExecutedCommand: "", + IsSSHClientClosed: false, + } +} + +type FakeVirtualMachine struct { + IsClosed bool + IsRemoved bool + IsStopped bool + FailingStop bool + FailingState bool + FakeSSHClient *FakeSSHClient +} + +func (vm *FakeVirtualMachine) Close() error { + vm.IsClosed = true + return nil +} + +func (vm *FakeVirtualMachine) Remove() error { + vm.IsRemoved = true + return nil +} + +func (vm *FakeVirtualMachine) Stop() error { + if vm.FailingStop { + return errors.New("stopping failed") + } + vm.IsStopped = true + return nil +} + +func (vm *FakeVirtualMachine) State() (state.State, error) { + if vm.FailingState { + return state.Error, errors.New("unable to get virtual machine state") + } + if vm.IsClosed || vm.IsStopped { + return state.Stopped, nil + } + return state.Running, nil +} + +func (vm *FakeVirtualMachine) IP() (string, error) { + panic("not implemented") +} + +func (vm *FakeVirtualMachine) SSHPort() int { + panic("not implemented") +} + +func (vm *FakeVirtualMachine) SSHRunner() (*ssh.Runner, error) { + runner, err := ssh.CreateRunnerWithClient(vm.FakeSSHClient) + return runner, err +} + +func (vm *FakeVirtualMachine) Bundle() *bundle.CrcBundleInfo { + panic("not implemented") +} + +func (vm *FakeVirtualMachine) Driver() drivers.Driver { + panic("not implemented") +} + +func (vm *FakeVirtualMachine) API() libmachine.API { + panic("not implemented") +} + +func (vm *FakeVirtualMachine) Host() *libmachinehost.Host { + panic("not implemented") +} + +func (vm *FakeVirtualMachine) Kill() error { + panic("not implemented") +} + +func NewFakeVirtualMachine(failingStop bool, failingState bool) *FakeVirtualMachine { + return &FakeVirtualMachine{ + FakeSSHClient: NewFakeSSHClient(), + IsStopped: false, + IsRemoved: false, + IsClosed: false, + FailingStop: failingStop, + FailingState: failingState, + } +} diff --git a/pkg/crc/ssh/ssh.go b/pkg/crc/ssh/ssh.go index a40a45296e..2100ce8fbf 100644 --- a/pkg/crc/ssh/ssh.go +++ b/pkg/crc/ssh/ssh.go @@ -27,6 +27,12 @@ func CreateRunner(ip string, port int, privateKeys ...string) (*Runner, error) { }, nil } +func CreateRunnerWithClient(client Client) (*Runner, error) { + return &Runner{ + client: client, + }, nil +} + func (runner *Runner) Close() { runner.client.Close() } From 325b81b93a5d624aca6e19495eae739f77f8258b Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Wed, 6 Nov 2024 12:29:35 +0530 Subject: [PATCH 4/5] test (machine) : Add additional tests in stop_test.go to use dummy VirtualMachine implementation Add additional tests in `stop_test.go` to verify that client.Stop() updates the state of virtual machine and unexposes exposed ports as expected. Use the fake vm implementation added previously to test vm state modification behavior on stop. Signed-off-by: Rohan Kumar --- pkg/crc/machine/stop_test.go | 60 ++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/pkg/crc/machine/stop_test.go b/pkg/crc/machine/stop_test.go index 3cee1d4207..a4ae6a1907 100644 --- a/pkg/crc/machine/stop_test.go +++ b/pkg/crc/machine/stop_test.go @@ -6,11 +6,71 @@ import ( "testing" crcConfig "github.com/crc-org/crc/v2/pkg/crc/config" + "github.com/crc-org/crc/v2/pkg/crc/machine/fakemachine" "github.com/crc-org/crc/v2/pkg/crc/machine/state" crcOs "github.com/crc-org/crc/v2/pkg/os" "github.com/stretchr/testify/assert" ) +func TestStop_WhenVMRunning_ThenShouldStopVirtualMachine(t *testing.T) { + // Given + crcConfigStorage := crcConfig.New(crcConfig.NewEmptyInMemoryStorage(), crcConfig.NewEmptyInMemorySecretStorage()) + crcConfigStorage.AddSetting(crcConfig.NetworkMode, "user", crcConfig.ValidateBool, crcConfig.SuccessfullyApplied, + "network-mode") + _, err := crcConfigStorage.Set(crcConfig.NetworkMode, "true") + assert.NoError(t, err) + virtualMachine := fakemachine.NewFakeVirtualMachine(false, false) + client := newClientWithVirtualMachine("fake-virtual-machine", false, crcConfigStorage, virtualMachine) + + // When + clusterState, stopErr := client.Stop() + + // Then + assert.NoError(t, stopErr) + assert.Equal(t, clusterState, state.Stopped) + assert.Equal(t, virtualMachine.IsStopped, true) + assert.Equal(t, virtualMachine.FakeSSHClient.LastExecutedCommand, "sudo -- sh -c 'crictl stop $(crictl ps -q)'") + assert.Equal(t, virtualMachine.FakeSSHClient.IsSSHClientClosed, true) +} + +func TestStop_WhenStopVmFailed_ThenErrorThrown(t *testing.T) { + // Given + crcConfigStorage := crcConfig.New(crcConfig.NewEmptyInMemoryStorage(), crcConfig.NewEmptyInMemorySecretStorage()) + crcConfigStorage.AddSetting(crcConfig.NetworkMode, "user", crcConfig.ValidateBool, crcConfig.SuccessfullyApplied, + "network-mode") + _, err := crcConfigStorage.Set(crcConfig.NetworkMode, "true") + assert.NoError(t, err) + virtualMachine := fakemachine.NewFakeVirtualMachine(true, false) + client := newClientWithVirtualMachine("fake-virtual-machine", false, crcConfigStorage, virtualMachine) + + // When + _, stopErr := client.Stop() + + // Then + assert.ErrorContains(t, stopErr, "Cannot stop machine: stopping failed") +} + +func TestStop_WhenVMAlreadyStopped_ThenThrowError(t *testing.T) { + // Given + crcConfigStorage := crcConfig.New(crcConfig.NewEmptyInMemoryStorage(), crcConfig.NewEmptyInMemorySecretStorage()) + crcConfigStorage.AddSetting(crcConfig.NetworkMode, "user", crcConfig.ValidateBool, crcConfig.SuccessfullyApplied, + "network-mode") + _, err := crcConfigStorage.Set(crcConfig.NetworkMode, "true") + assert.NoError(t, err) + virtualMachine := fakemachine.NewFakeVirtualMachine(false, false) + err = virtualMachine.Stop() + assert.NoError(t, err) + client := newClientWithVirtualMachine("fake-virtual-machine", false, crcConfigStorage, virtualMachine) + + // When + clusterState, stopErr := client.Stop() + + // Then + assert.EqualError(t, stopErr, "Instance is already stopped") + assert.Equal(t, clusterState, state.Error) + assert.Equal(t, virtualMachine.IsStopped, true) +} + func TestClient_WhenStopInvokedWithNonExistentVM_ThenThrowError(t *testing.T) { // Given dir := t.TempDir() From 7cc767264e4bda27602aa4d4895ae9b59fb26579 Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Tue, 5 Nov 2024 16:24:01 +0530 Subject: [PATCH 5/5] test (machine) : add vsock interaction methods to VirtualMachine interface Make VirtualMachine implement vsock methods so that vsock interaction is also done via VirtualMachine interface. This would help in writing tests, we can override the behavior of expose/unexpose in fake vm implementation. Signed-off-by: Rohan Kumar --- pkg/crc/machine/delete.go | 2 +- pkg/crc/machine/fakemachine/virtualmachine.go | 12 ++++++++++ pkg/crc/machine/start.go | 2 +- pkg/crc/machine/stop.go | 2 +- pkg/crc/machine/stop_test.go | 22 +++++++++---------- pkg/crc/machine/virtualmachine.go | 4 ++++ pkg/crc/machine/vsock.go | 4 ++-- 7 files changed, 32 insertions(+), 16 deletions(-) diff --git a/pkg/crc/machine/delete.go b/pkg/crc/machine/delete.go index 2983e8231f..66f3651fb2 100644 --- a/pkg/crc/machine/delete.go +++ b/pkg/crc/machine/delete.go @@ -22,7 +22,7 @@ func (client *client) Delete() error { // In case usermode networking make sure all the port bind on host should be released if client.useVSock() { - if err := unexposePorts(); err != nil { + if err := vm.UnExposePorts(); err != nil { return err } } diff --git a/pkg/crc/machine/fakemachine/virtualmachine.go b/pkg/crc/machine/fakemachine/virtualmachine.go index d19b521c37..2b36d8895d 100644 --- a/pkg/crc/machine/fakemachine/virtualmachine.go +++ b/pkg/crc/machine/fakemachine/virtualmachine.go @@ -5,6 +5,7 @@ import ( "github.com/crc-org/crc/v2/pkg/crc/machine/bundle" "github.com/crc-org/crc/v2/pkg/crc/machine/state" + crcPreset "github.com/crc-org/crc/v2/pkg/crc/preset" "github.com/crc-org/crc/v2/pkg/crc/ssh" "github.com/crc-org/crc/v2/pkg/libmachine" libmachinehost "github.com/crc-org/crc/v2/pkg/libmachine/host" @@ -39,6 +40,7 @@ type FakeVirtualMachine struct { FailingStop bool FailingState bool FakeSSHClient *FakeSSHClient + PortsExposed bool } func (vm *FakeVirtualMachine) Close() error { @@ -102,6 +104,16 @@ func (vm *FakeVirtualMachine) Kill() error { panic("not implemented") } +func (vm *FakeVirtualMachine) ExposePorts(_ crcPreset.Preset, _, _ uint) error { + vm.PortsExposed = true + return nil +} + +func (vm *FakeVirtualMachine) UnExposePorts() error { + vm.PortsExposed = false + return nil +} + func NewFakeVirtualMachine(failingStop bool, failingState bool) *FakeVirtualMachine { return &FakeVirtualMachine{ FakeSSHClient: NewFakeSSHClient(), diff --git a/pkg/crc/machine/start.go b/pkg/crc/machine/start.go index e573784487..8d7f1b98ec 100644 --- a/pkg/crc/machine/start.go +++ b/pkg/crc/machine/start.go @@ -368,7 +368,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) logging.Infof("Starting CRC VM for %s %s...", startConfig.Preset, vm.Bundle().GetVersion()) if client.useVSock() { - if err := exposePorts(startConfig.Preset, startConfig.IngressHTTPPort, startConfig.IngressHTTPSPort); err != nil { + if err := vm.ExposePorts(startConfig.Preset, startConfig.IngressHTTPPort, startConfig.IngressHTTPSPort); err != nil { return nil, err } } diff --git a/pkg/crc/machine/stop.go b/pkg/crc/machine/stop.go index f25a2e18f8..8d6fb80da1 100644 --- a/pkg/crc/machine/stop.go +++ b/pkg/crc/machine/stop.go @@ -45,7 +45,7 @@ func (client *client) Stop() (state.State, error) { } // In case usermode networking make sure all the port bind on host should be released if client.useVSock() { - return status, unexposePorts() + return status, vm.UnExposePorts() } return status, nil } diff --git a/pkg/crc/machine/stop_test.go b/pkg/crc/machine/stop_test.go index a4ae6a1907..9882c79be7 100644 --- a/pkg/crc/machine/stop_test.go +++ b/pkg/crc/machine/stop_test.go @@ -19,8 +19,8 @@ func TestStop_WhenVMRunning_ThenShouldStopVirtualMachine(t *testing.T) { "network-mode") _, err := crcConfigStorage.Set(crcConfig.NetworkMode, "true") assert.NoError(t, err) - virtualMachine := fakemachine.NewFakeVirtualMachine(false, false) - client := newClientWithVirtualMachine("fake-virtual-machine", false, crcConfigStorage, virtualMachine) + fakeVirtualMachine := fakemachine.NewFakeVirtualMachine(false, false) + client := newClientWithVirtualMachine("fake-virtual-machine", false, crcConfigStorage, fakeVirtualMachine) // When clusterState, stopErr := client.Stop() @@ -28,9 +28,9 @@ func TestStop_WhenVMRunning_ThenShouldStopVirtualMachine(t *testing.T) { // Then assert.NoError(t, stopErr) assert.Equal(t, clusterState, state.Stopped) - assert.Equal(t, virtualMachine.IsStopped, true) - assert.Equal(t, virtualMachine.FakeSSHClient.LastExecutedCommand, "sudo -- sh -c 'crictl stop $(crictl ps -q)'") - assert.Equal(t, virtualMachine.FakeSSHClient.IsSSHClientClosed, true) + assert.Equal(t, fakeVirtualMachine.IsStopped, true) + assert.Equal(t, fakeVirtualMachine.FakeSSHClient.IsSSHClientClosed, true) + assert.Equal(t, fakeVirtualMachine.PortsExposed, false) } func TestStop_WhenStopVmFailed_ThenErrorThrown(t *testing.T) { @@ -40,8 +40,8 @@ func TestStop_WhenStopVmFailed_ThenErrorThrown(t *testing.T) { "network-mode") _, err := crcConfigStorage.Set(crcConfig.NetworkMode, "true") assert.NoError(t, err) - virtualMachine := fakemachine.NewFakeVirtualMachine(true, false) - client := newClientWithVirtualMachine("fake-virtual-machine", false, crcConfigStorage, virtualMachine) + fakeVirtualMachine := fakemachine.NewFakeVirtualMachine(true, false) + client := newClientWithVirtualMachine("fake-virtual-machine", false, crcConfigStorage, fakeVirtualMachine) // When _, stopErr := client.Stop() @@ -57,10 +57,10 @@ func TestStop_WhenVMAlreadyStopped_ThenThrowError(t *testing.T) { "network-mode") _, err := crcConfigStorage.Set(crcConfig.NetworkMode, "true") assert.NoError(t, err) - virtualMachine := fakemachine.NewFakeVirtualMachine(false, false) - err = virtualMachine.Stop() + fakeVirtualMachine := fakemachine.NewFakeVirtualMachine(false, false) + err = fakeVirtualMachine.Stop() assert.NoError(t, err) - client := newClientWithVirtualMachine("fake-virtual-machine", false, crcConfigStorage, virtualMachine) + client := newClientWithVirtualMachine("fake-virtual-machine", false, crcConfigStorage, fakeVirtualMachine) // When clusterState, stopErr := client.Stop() @@ -68,7 +68,7 @@ func TestStop_WhenVMAlreadyStopped_ThenThrowError(t *testing.T) { // Then assert.EqualError(t, stopErr, "Instance is already stopped") assert.Equal(t, clusterState, state.Error) - assert.Equal(t, virtualMachine.IsStopped, true) + assert.Equal(t, fakeVirtualMachine.IsStopped, true) } func TestClient_WhenStopInvokedWithNonExistentVM_ThenThrowError(t *testing.T) { diff --git a/pkg/crc/machine/virtualmachine.go b/pkg/crc/machine/virtualmachine.go index 03a6b0e60f..38941b055c 100644 --- a/pkg/crc/machine/virtualmachine.go +++ b/pkg/crc/machine/virtualmachine.go @@ -3,6 +3,8 @@ package machine import ( "fmt" + crcPreset "github.com/crc-org/crc/v2/pkg/crc/preset" + "github.com/crc-org/crc/v2/pkg/crc/constants" "github.com/crc-org/crc/v2/pkg/crc/machine/bundle" "github.com/crc-org/crc/v2/pkg/crc/machine/state" @@ -26,6 +28,8 @@ type VirtualMachine interface { Driver() drivers.Driver API() libmachine.API Host() *libmachinehost.Host + ExposePorts(preset crcPreset.Preset, ingressHTTPPort, ingressHTTPSPort uint) error + UnExposePorts() error } type virtualMachine struct { diff --git a/pkg/crc/machine/vsock.go b/pkg/crc/machine/vsock.go index 7d9c72613a..eb7e129bb0 100644 --- a/pkg/crc/machine/vsock.go +++ b/pkg/crc/machine/vsock.go @@ -16,7 +16,7 @@ import ( "github.com/pkg/errors" ) -func exposePorts(preset crcPreset.Preset, ingressHTTPPort, ingressHTTPSPort uint) error { +func (vm *virtualMachine) ExposePorts(preset crcPreset.Preset, ingressHTTPPort, ingressHTTPSPort uint) error { portsToExpose := vsockPorts(preset, ingressHTTPPort, ingressHTTPSPort) daemonClient := daemonclient.New() alreadyOpenedPorts, err := listOpenPorts(daemonClient) @@ -47,7 +47,7 @@ func isOpened(exposed []types.ExposeRequest, port types.ExposeRequest) bool { return false } -func unexposePorts() error { +func (vm *virtualMachine) UnExposePorts() error { var mErr crcErrors.MultiError daemonClient := daemonclient.New() alreadyOpenedPorts, err := listOpenPorts(daemonClient)