From d499a7d712bcc726f331ff07f0e0855ddc9046fa Mon Sep 17 00:00:00 2001 From: Sam Berning <113054166+sam-berning@users.noreply.github.com> Date: Thu, 26 Jan 2023 19:17:59 -0600 Subject: [PATCH] feat: adds a --force flag to vm stop and remove (#178) Signed-off-by: Sam Berning Issue #, if available: https://github.com/runfinch/finch/issues/171 *Description of changes:* Adds a force flag to `finch vm stop` and `finch vm remove`. This gives us a native way to recover from "unrecognized system status" as seen in https://github.com/runfinch/finch/issues/171. *Testing done:* Unit testing, E2E testing. Force "unrecognized system status" by messing with the Lima instance ``` $ ./_output/bin/finch vm stop FATA[0000] unrecognized system status $ ./_output/bin/finch vm stop --force INFO[0000] Forcibly stopping Finch virtual machine... INFO[0001] Finch virtual machine stopped successfully $ ./_output/bin/finch vm start INFO[0000] Starting existing Finch virtual machine... INFO[0038] Finch virtual machine started successfully ``` ``` $ ./_output/bin/finch vm stop FATA[0000] unrecognized system status $ ./_output/bin/finch vm remove --force INFO[0000] Forcibly removing Finch virtual machine... INFO[0000] Finch virtual machine removed successfully $ ./_output/bin/finch vm status Nonexistent ``` - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Sam Berning --- cmd/finch/virtual_machine_remove.go | 47 ++++++++++++++++++------ cmd/finch/virtual_machine_remove_test.go | 44 ++++++++++++++++++---- cmd/finch/virtual_machine_stop.go | 47 ++++++++++++++++++------ cmd/finch/virtual_machine_stop_test.go | 44 ++++++++++++++++++---- e2e/vm/lifecycle_test.go | 22 ++++++++++- 5 files changed, 165 insertions(+), 39 deletions(-) diff --git a/cmd/finch/virtual_machine_remove.go b/cmd/finch/virtual_machine_remove.go index 37614917b..bca27aad2 100644 --- a/cmd/finch/virtual_machine_remove.go +++ b/cmd/finch/virtual_machine_remove.go @@ -21,6 +21,8 @@ func newRemoveVMCommand(limaCmdCreator command.LimaCmdCreator, logger flog.Logge RunE: newRemoveVMAction(limaCmdCreator, logger).runAdapter, } + removeVMCommand.Flags().BoolP("force", "f", false, "forcibly remove finch VM") + return removeVMCommand } @@ -34,24 +36,24 @@ func newRemoveVMAction(creator command.LimaCmdCreator, logger flog.Logger) *remo } func (rva *removeVMAction) runAdapter(cmd *cobra.Command, args []string) error { - return rva.run() -} - -func (rva *removeVMAction) run() error { - err := rva.assertVMIsStopped(rva.creator, rva.logger) + force, err := cmd.Flags().GetBool("force") if err != nil { return err } + return rva.run(force) +} - limaCmd := rva.creator.CreateWithoutStdio("remove", limaInstanceName) - rva.logger.Info("Removing existing Finch virtual machine...") - logs, err := limaCmd.CombinedOutput() +func (rva *removeVMAction) run(force bool) error { + if force { + return rva.removeVM(force) + } + + err := rva.assertVMIsStopped(rva.creator, rva.logger) if err != nil { - rva.logger.Errorf("Finch virtual machine failed to remove, debug logs: %s", logs) return err } - rva.logger.Info("Finch virtual machine removed successfully") - return nil + + return rva.removeVM(false) } func (rva *removeVMAction) assertVMIsStopped(creator command.LimaCmdCreator, logger flog.Logger) error { @@ -69,3 +71,26 @@ func (rva *removeVMAction) assertVMIsStopped(creator command.LimaCmdCreator, log return nil } } + +func (rva *removeVMAction) removeVM(force bool) error { + limaCmd := rva.createVMRemoveCommand(force) + if force { + rva.logger.Info("Forcibly removing Finch virtual machine...") + } else { + rva.logger.Info("Removing existing Finch virtual machine...") + } + logs, err := limaCmd.CombinedOutput() + if err != nil { + rva.logger.Errorf("Finch virtual machine failed to remove, debug logs: %s", logs) + return err + } + rva.logger.Info("Finch virtual machine removed successfully") + return nil +} + +func (rva *removeVMAction) createVMRemoveCommand(force bool) command.Command { + if force { + return rva.creator.CreateWithoutStdio("remove", "--force", limaInstanceName) + } + return rva.creator.CreateWithoutStdio("remove", limaInstanceName) +} diff --git a/cmd/finch/virtual_machine_remove_test.go b/cmd/finch/virtual_machine_remove_test.go index 0b117cdcd..e77fdfbbe 100644 --- a/cmd/finch/virtual_machine_remove_test.go +++ b/cmd/finch/virtual_machine_remove_test.go @@ -11,7 +11,6 @@ import ( "github.com/runfinch/finch/pkg/mocks" "github.com/golang/mock/gomock" - "github.com/spf13/cobra" "github.com/stretchr/testify/assert" ) @@ -28,14 +27,10 @@ func TestRemoveVMAction_runAdapter(t *testing.T) { testCases := []struct { name string mockSvc func(*mocks.Logger, *mocks.LimaCmdCreator, *gomock.Controller) - cmd *cobra.Command args []string }{ { name: "should remove the instance", - cmd: &cobra.Command{ - Use: "remove", - }, args: []string{}, mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) { getVMStatusC := mocks.NewCommand(ctrl) @@ -49,6 +44,18 @@ func TestRemoveVMAction_runAdapter(t *testing.T) { logger.EXPECT().Info(gomock.Any()).AnyTimes() }, }, + { + name: "should forcibly remove the instance", + args: []string{ + "--force", + }, + mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) { + command := mocks.NewCommand(ctrl) + creator.EXPECT().CreateWithoutStdio("remove", "--force", limaInstanceName).Return(command) + command.EXPECT().CombinedOutput() + logger.EXPECT().Info(gomock.Any()).AnyTimes() + }, + }, } for _, tc := range testCases { @@ -59,9 +66,11 @@ func TestRemoveVMAction_runAdapter(t *testing.T) { ctrl := gomock.NewController(t) logger := mocks.NewLogger(ctrl) lcc := mocks.NewLimaCmdCreator(ctrl) - tc.mockSvc(logger, lcc, ctrl) - assert.NoError(t, newRemoveVMAction(lcc, logger).runAdapter(tc.cmd, tc.args)) + + cmd := newRemoveVMCommand(lcc, logger) + cmd.SetArgs(tc.args) + assert.NoError(t, cmd.Execute()) }) } } @@ -73,6 +82,7 @@ func TestRemoveVMAction_run(t *testing.T) { name string wantErr error mockSvc func(*mocks.Logger, *mocks.LimaCmdCreator, *gomock.Controller) + force bool }{ { name: "should remove the instance", @@ -89,6 +99,7 @@ func TestRemoveVMAction_run(t *testing.T) { logger.EXPECT().Info("Removing existing Finch virtual machine...") logger.EXPECT().Info("Finch virtual machine removed successfully") }, + force: false, }, { name: "running VM", @@ -100,6 +111,7 @@ func TestRemoveVMAction_run(t *testing.T) { getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil) logger.EXPECT().Debugf("Status of virtual machine: %s", "Running") }, + force: false, }, { name: "nonexistent VM", @@ -110,6 +122,7 @@ func TestRemoveVMAction_run(t *testing.T) { getVMStatusC.EXPECT().Output().Return([]byte(""), nil) logger.EXPECT().Debugf("Status of virtual machine: %s", "") }, + force: false, }, { name: "unknown VM status", @@ -120,6 +133,7 @@ func TestRemoveVMAction_run(t *testing.T) { getVMStatusC.EXPECT().Output().Return([]byte("Broken"), nil) logger.EXPECT().Debugf("Status of virtual machine: %s", "Broken") }, + force: false, }, { name: "status command returns an error", @@ -129,6 +143,7 @@ func TestRemoveVMAction_run(t *testing.T) { creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC) getVMStatusC.EXPECT().Output().Return([]byte("Broken"), errors.New("get status error")) }, + force: false, }, { name: "should print error if virtual machine failed to remove", @@ -146,6 +161,19 @@ func TestRemoveVMAction_run(t *testing.T) { logger.EXPECT().Info("Removing existing Finch virtual machine...") logger.EXPECT().Errorf("Finch virtual machine failed to remove, debug logs: %s", logs) }, + force: false, + }, + { + name: "should forcibly remove the instance", + wantErr: nil, + mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) { + command := mocks.NewCommand(ctrl) + creator.EXPECT().CreateWithoutStdio("remove", "--force", limaInstanceName).Return(command) + command.EXPECT().CombinedOutput() + logger.EXPECT().Info("Forcibly removing Finch virtual machine...") + logger.EXPECT().Info("Finch virtual machine removed successfully") + }, + force: true, }, } @@ -159,7 +187,7 @@ func TestRemoveVMAction_run(t *testing.T) { lcc := mocks.NewLimaCmdCreator(ctrl) tc.mockSvc(logger, lcc, ctrl) - err := newRemoveVMAction(lcc, logger).run() + err := newRemoveVMAction(lcc, logger).run(tc.force) assert.Equal(t, tc.wantErr, err) }) } diff --git a/cmd/finch/virtual_machine_stop.go b/cmd/finch/virtual_machine_stop.go index cabe01583..4bece4c89 100644 --- a/cmd/finch/virtual_machine_stop.go +++ b/cmd/finch/virtual_machine_stop.go @@ -20,6 +20,8 @@ func newStopVMCommand(limaCmdCreator command.LimaCmdCreator, logger flog.Logger) RunE: newStopVMAction(limaCmdCreator, logger).runAdapter, } + stopVMCommand.Flags().BoolP("force", "f", false, "forcibly stop finch VM") + return stopVMCommand } @@ -33,24 +35,24 @@ func newStopVMAction(creator command.LimaCmdCreator, logger flog.Logger) *stopVM } func (sva *stopVMAction) runAdapter(cmd *cobra.Command, args []string) error { - return sva.run() -} - -func (sva *stopVMAction) run() error { - err := sva.assertVMIsRunning(sva.creator, sva.logger) + force, err := cmd.Flags().GetBool("force") if err != nil { return err } + return sva.run(force) +} - limaCmd := sva.creator.CreateWithoutStdio("stop", limaInstanceName) - sva.logger.Info("Stopping existing Finch virtual machine...") - logs, err := limaCmd.CombinedOutput() +func (sva *stopVMAction) run(force bool) error { + if force { + return sva.stopVM(force) + } + + err := sva.assertVMIsRunning(sva.creator, sva.logger) if err != nil { - sva.logger.Errorf("Finch virtual machine failed to stop, debug logs: %s", logs) return err } - sva.logger.Info("Finch virtual machine stopped successfully") - return nil + + return sva.stopVM(false) } func (sva *stopVMAction) assertVMIsRunning(creator command.LimaCmdCreator, logger flog.Logger) error { @@ -67,3 +69,26 @@ func (sva *stopVMAction) assertVMIsRunning(creator command.LimaCmdCreator, logge return nil } } + +func (sva *stopVMAction) stopVM(force bool) error { + limaCmd := sva.createLimaStopCommand(force) + if force { + sva.logger.Info("Forcibly stopping Finch virtual machine...") + } else { + sva.logger.Info("Stopping existing Finch virtual machine...") + } + logs, err := limaCmd.CombinedOutput() + if err != nil { + sva.logger.Errorf("Finch virtual machine failed to stop, debug logs: %s", logs) + return err + } + sva.logger.Info("Finch virtual machine stopped successfully") + return nil +} + +func (sva *stopVMAction) createLimaStopCommand(force bool) command.Command { + if force { + return sva.creator.CreateWithoutStdio("stop", "--force", limaInstanceName) + } + return sva.creator.CreateWithoutStdio("stop", limaInstanceName) +} diff --git a/cmd/finch/virtual_machine_stop_test.go b/cmd/finch/virtual_machine_stop_test.go index 2966843dd..8134e7b0b 100644 --- a/cmd/finch/virtual_machine_stop_test.go +++ b/cmd/finch/virtual_machine_stop_test.go @@ -11,7 +11,6 @@ import ( "github.com/runfinch/finch/pkg/mocks" "github.com/golang/mock/gomock" - "github.com/spf13/cobra" "github.com/stretchr/testify/assert" ) @@ -28,14 +27,10 @@ func TestStopVMAction_runAdapter(t *testing.T) { testCases := []struct { name string mockSvc func(*mocks.Logger, *mocks.LimaCmdCreator, *gomock.Controller) - cmd *cobra.Command args []string }{ { name: "should stop the instance", - cmd: &cobra.Command{ - Use: "stop", - }, args: []string{}, mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) { getVMStatusC := mocks.NewCommand(ctrl) @@ -49,6 +44,18 @@ func TestStopVMAction_runAdapter(t *testing.T) { logger.EXPECT().Info(gomock.Any()).AnyTimes() }, }, + { + name: "should force stop the instance", + args: []string{ + "--force", + }, + mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) { + command := mocks.NewCommand(ctrl) + creator.EXPECT().CreateWithoutStdio("stop", "--force", limaInstanceName).Return(command) + command.EXPECT().CombinedOutput() + logger.EXPECT().Info(gomock.Any()).AnyTimes() + }, + }, } for _, tc := range testCases { @@ -59,9 +66,11 @@ func TestStopVMAction_runAdapter(t *testing.T) { ctrl := gomock.NewController(t) logger := mocks.NewLogger(ctrl) lcc := mocks.NewLimaCmdCreator(ctrl) - tc.mockSvc(logger, lcc, ctrl) - assert.NoError(t, newStopVMAction(lcc, logger).runAdapter(tc.cmd, tc.args)) + + cmd := newStopVMCommand(lcc, logger) + cmd.SetArgs(tc.args) + assert.NoError(t, cmd.Execute()) }) } } @@ -73,6 +82,7 @@ func TestStopVMAction_run(t *testing.T) { name string wantErr error mockSvc func(*mocks.Logger, *mocks.LimaCmdCreator, *gomock.Controller) + force bool }{ { name: "should stop the instance", @@ -89,6 +99,7 @@ func TestStopVMAction_run(t *testing.T) { logger.EXPECT().Info("Stopping existing Finch virtual machine...") logger.EXPECT().Info("Finch virtual machine stopped successfully") }, + force: false, }, { name: "stopped VM", @@ -99,6 +110,7 @@ func TestStopVMAction_run(t *testing.T) { getVMStatusC.EXPECT().Output().Return([]byte("Stopped"), nil) logger.EXPECT().Debugf("Status of virtual machine: %s", "Stopped") }, + force: false, }, { name: "nonexistent VM", @@ -109,6 +121,7 @@ func TestStopVMAction_run(t *testing.T) { getVMStatusC.EXPECT().Output().Return([]byte(""), nil) logger.EXPECT().Debugf("Status of virtual machine: %s", "") }, + force: false, }, { name: "unknown VM status", @@ -119,6 +132,7 @@ func TestStopVMAction_run(t *testing.T) { getVMStatusC.EXPECT().Output().Return([]byte("Broken"), nil) logger.EXPECT().Debugf("Status of virtual machine: %s", "Broken") }, + force: false, }, { name: "status command returns an error", @@ -128,6 +142,7 @@ func TestStopVMAction_run(t *testing.T) { creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC) getVMStatusC.EXPECT().Output().Return([]byte("Broken"), errors.New("get status error")) }, + force: false, }, { name: "should print error if virtual machine failed to stop", @@ -145,6 +160,19 @@ func TestStopVMAction_run(t *testing.T) { logger.EXPECT().Info("Stopping existing Finch virtual machine...") logger.EXPECT().Errorf("Finch virtual machine failed to stop, debug logs: %s", logs) }, + force: false, + }, + { + name: "should force stop virtual machine", + wantErr: nil, + mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) { + command := mocks.NewCommand(ctrl) + creator.EXPECT().CreateWithoutStdio("stop", "--force", limaInstanceName).Return(command) + command.EXPECT().CombinedOutput() + logger.EXPECT().Info("Forcibly stopping Finch virtual machine...") + logger.EXPECT().Info("Finch virtual machine stopped successfully") + }, + force: true, }, } @@ -158,7 +186,7 @@ func TestStopVMAction_run(t *testing.T) { lcc := mocks.NewLimaCmdCreator(ctrl) tc.mockSvc(logger, lcc, ctrl) - err := newStopVMAction(lcc, logger).run() + err := newStopVMAction(lcc, logger).run(tc.force) assert.Equal(t, tc.wantErr, err) }) } diff --git a/e2e/vm/lifecycle_test.go b/e2e/vm/lifecycle_test.go index 1dd84cca3..43f82b2fd 100644 --- a/e2e/vm/lifecycle_test.go +++ b/e2e/vm/lifecycle_test.go @@ -23,6 +23,20 @@ var testVMLifecycle = func(o *option.Option) { gomega.Expect(command.StdoutStr(o, virtualMachineRootCmd, "status")).To(gomega.Equal("Running")) }) + ginkgo.It("should be able to force stop the virtual machine", func() { + command.Run(o, "images") + command.New(o, virtualMachineRootCmd, "stop", "--force").WithTimeoutInSeconds(90).Run() + command.RunWithoutSuccessfulExit(o, "images") + command.New(o, virtualMachineRootCmd, "start").WithTimeoutInSeconds(120).Run() + }) + + ginkgo.It("should be able to force remove the virtual machine", func() { + command.Run(o, "images") + command.New(o, virtualMachineRootCmd, "remove", "--force").WithTimeoutInSeconds(60).Run() + command.RunWithoutSuccessfulExit(o, "images") + command.New(o, virtualMachineRootCmd, "init").WithTimeoutInSeconds(600).Run() + }) + ginkgo.It("should be able to stop the virtual machine", func() { command.Run(o, "images") command.New(o, virtualMachineRootCmd, "stop").WithTimeoutInSeconds(90).Run() @@ -43,12 +57,18 @@ var testVMLifecycle = func(o *option.Option) { ginkgo.It("should be able to start the virtual machine", func() { command.New(o, virtualMachineRootCmd, "start").WithTimeoutInSeconds(120).Run() command.Run(o, "images") + command.New(o, virtualMachineRootCmd, "stop").WithTimeoutInSeconds(90).Run() }) ginkgo.It("should be able to remove the virtual machine", func() { - command.New(o, virtualMachineRootCmd, "stop").WithTimeoutInSeconds(90).Run() command.New(o, virtualMachineRootCmd, "remove").WithTimeoutInSeconds(60).Run() command.RunWithoutSuccessfulExit(o, "images") + command.New(o, virtualMachineRootCmd, "init").WithTimeoutInSeconds(600).Run() + command.New(o, virtualMachineRootCmd, "stop").WithTimeoutInSeconds(90).Run() + }) + ginkgo.It("should be able to force remove the virtual machine", func() { + command.New(o, virtualMachineRootCmd, "remove", "--force").WithTimeoutInSeconds(60).Run() + command.RunWithoutSuccessfulExit(o, "images") }) })