Skip to content

Commit

Permalink
feat: adds option to remove persistent user data
Browse files Browse the repository at this point in the history 
Provides a native way for Finch to delete a persistent user data disk.
Calling `finch vm remove --user-data` will now cleanup both the VM and
any persistent user data.

Signed-off-by: Sam Berning <bernings@amazon.com>
  • Loading branch information
@sam-berning
sam-berning committed Feb 10, 2023
1 parent 7aaa381 commit 3542863
Show file tree
Hide file tree
Showing 7 changed files with 300 additions and 38 deletions.
2 changes: 1 addition & 1 deletion cmd/finch/virtual_machine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func newVirtualMachineCommand(
virtualMachineCommand.AddCommand(
newStartVMCommand(limaCmdCreator, logger, optionalDepGroups, lca, nca, fs, fp.LimaSSHPrivateKeyPath(), diskManager),
newStopVMCommand(limaCmdCreator, logger),
newRemoveVMCommand(limaCmdCreator, logger),
newRemoveVMCommand(limaCmdCreator, logger, diskManager),
newStatusVMCommand(limaCmdCreator, logger, os.Stdout),
newInitVMCommand(limaCmdCreator, logger, optionalDepGroups, lca, nca, fp.BaseYamlFilePath(), fs,
fp.LimaSSHPrivateKeyPath(), diskManager),
Expand Down
56 changes: 46 additions & 10 deletions cmd/finch/virtual_machine_remove.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ package main
import (
"fmt"

"github.com/runfinch/finch/pkg/disk"

"github.com/spf13/cobra"

"github.com/runfinch/finch/pkg/command"
Expand All @@ -14,46 +16,80 @@ import (
"github.com/runfinch/finch/pkg/flog"
)

func newRemoveVMCommand(limaCmdCreator command.LimaCmdCreator, logger flog.Logger) *cobra.Command {
func newRemoveVMCommand(limaCmdCreator command.LimaCmdCreator, logger flog.Logger, dm disk.UserDataDiskManager) *cobra.Command {
removeVMCommand := &cobra.Command{
Use: "remove",
Short: "Remove the virtual machine instance",
RunE: newRemoveVMAction(limaCmdCreator, logger).runAdapter,
RunE: newRemoveVMAction(limaCmdCreator, logger, dm).runAdapter,
}

removeVMCommand.Flags().BoolP("force", "f", false, "forcibly remove finch VM")
removeVMCommand.Flags().Bool("user-data", false, "remove persistent containerd and nerdctl data")

return removeVMCommand
}

type removeVMAction struct {
creator command.LimaCmdCreator
logger flog.Logger
creator command.LimaCmdCreator
logger flog.Logger
diskManager disk.UserDataDiskManager
}

func newRemoveVMAction(creator command.LimaCmdCreator, logger flog.Logger) *removeVMAction {
return &removeVMAction{creator: creator, logger: logger}
func newRemoveVMAction(creator command.LimaCmdCreator, logger flog.Logger, dm disk.UserDataDiskManager) *removeVMAction {
return &removeVMAction{creator: creator, logger: logger, diskManager: dm}
}

func (rva *removeVMAction) runAdapter(cmd *cobra.Command, args []string) error {
force, err := cmd.Flags().GetBool("force")
if err != nil {
return err
}
return rva.run(force)
deleteDisk, err := cmd.Flags().GetBool("user-data")
if err != nil {
return err
}
return rva.run(force, deleteDisk)
}

func (rva *removeVMAction) run(force bool) error {
func (rva *removeVMAction) run(force bool, deleteDisk bool) error {
if force {
return rva.removeVM(force)
err := rva.removeVM(force)
if err != nil {
return err
}

if deleteDisk {
rva.logger.Info("Forcibly deleting user data...")
err := rva.diskManager.DeleteUserDataDisk(force)
if err != nil {
return err
}
rva.logger.Info("Successfully deleted user data.")
}

return nil
}

err := rva.assertVMIsStopped(rva.creator, rva.logger)
if err != nil {
return err
}

return rva.removeVM(false)
err = rva.removeVM(false)
if err != nil {
return err
}

if deleteDisk {
rva.logger.Info("Deleting user data...")
err := rva.diskManager.DeleteUserDataDisk(false)
if err != nil {
return err
}
rva.logger.Info("Successfully deleted user data.")
}

return nil
}

func (rva *removeVMAction) assertVMIsStopped(creator command.LimaCmdCreator, logger flog.Logger) error {
Expand Down
137 changes: 111 additions & 26 deletions cmd/finch/virtual_machine_remove_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
func TestNewRemoveVMCommand(t *testing.T) {
t.Parallel()

cmd := newRemoveVMCommand(nil, nil)
cmd := newRemoveVMCommand(nil, nil, nil)
assert.Equal(t, cmd.Name(), "remove")
}

Expand All @@ -26,13 +26,13 @@ func TestRemoveVMAction_runAdapter(t *testing.T) {

testCases := []struct {
name string
mockSvc func(*mocks.Logger, *mocks.LimaCmdCreator, *gomock.Controller)
mockSvc func(*mocks.Logger, *mocks.LimaCmdCreator, *gomock.Controller, *mocks.MockUserDataDiskManager)
args []string
}{
{
name: "should remove the instance",
args: []string{},
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) {
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
getVMStatusC := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
getVMStatusC.EXPECT().Output().Return([]byte("Stopped"), nil)
Expand All @@ -49,13 +49,51 @@ func TestRemoveVMAction_runAdapter(t *testing.T) {
args: []string{
"--force",
},
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) {
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
command := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("remove", "--force", limaInstanceName).Return(command)
command.EXPECT().CombinedOutput()
logger.EXPECT().Info(gomock.Any()).AnyTimes()
},
},
{
name: "should remove the instance and user data",
args: []string{
"--user-data",
},
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
getVMStatusC := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
getVMStatusC.EXPECT().Output().Return([]byte("Stopped"), nil)
logger.EXPECT().Debugf("Status of virtual machine: %s", "Stopped")

command := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("remove", limaInstanceName).Return(command)
command.EXPECT().CombinedOutput()
logger.EXPECT().Info("Removing existing Finch virtual machine...")
logger.EXPECT().Info("Finch virtual machine removed successfully")
logger.EXPECT().Info("Deleting user data...")
dm.EXPECT().DeleteUserDataDisk(false).Return(nil)
logger.EXPECT().Info("Successfully deleted user data.")
},
},
{
name: "should forcibly remove the instance and user data",
args: []string{
"--force",
"--user-data",
},
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
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")
logger.EXPECT().Info("Forcibly deleting user data...")
dm.EXPECT().DeleteUserDataDisk(true).Return(nil)
logger.EXPECT().Info("Successfully deleted user data.")
},
},
}

for _, tc := range testCases {
Expand All @@ -66,9 +104,10 @@ func TestRemoveVMAction_runAdapter(t *testing.T) {
ctrl := gomock.NewController(t)
logger := mocks.NewLogger(ctrl)
lcc := mocks.NewLimaCmdCreator(ctrl)
tc.mockSvc(logger, lcc, ctrl)
dm := mocks.NewMockUserDataDiskManager(ctrl)
tc.mockSvc(logger, lcc, ctrl, dm)

cmd := newRemoveVMCommand(lcc, logger)
cmd := newRemoveVMCommand(lcc, logger, dm)
cmd.SetArgs(tc.args)
assert.NoError(t, cmd.Execute())
})
Expand All @@ -79,15 +118,16 @@ func TestRemoveVMAction_run(t *testing.T) {
t.Parallel()

testCases := []struct {
name string
wantErr error
mockSvc func(*mocks.Logger, *mocks.LimaCmdCreator, *gomock.Controller)
force bool
name string
wantErr error
mockSvc func(*mocks.Logger, *mocks.LimaCmdCreator, *gomock.Controller, *mocks.MockUserDataDiskManager)
force bool
deleteDisk bool
}{
{
name: "should remove the instance",
wantErr: nil,
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) {
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
getVMStatusC := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
getVMStatusC.EXPECT().Output().Return([]byte("Stopped"), nil)
Expand All @@ -99,56 +139,61 @@ func TestRemoveVMAction_run(t *testing.T) {
logger.EXPECT().Info("Removing existing Finch virtual machine...")
logger.EXPECT().Info("Finch virtual machine removed successfully")
},
force: false,
force: false,
deleteDisk: false,
},
{
name: "running VM",
wantErr: fmt.Errorf("the instance %q is running, run `finch %s stop` to stop the instance first",
limaInstanceName, virtualMachineRootCmd),
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) {
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
getVMStatusC := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
},
force: false,
force: false,
deleteDisk: false,
},
{
name: "nonexistent VM",
wantErr: fmt.Errorf("the instance %q does not exist", limaInstanceName),
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) {
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
getVMStatusC := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
getVMStatusC.EXPECT().Output().Return([]byte(""), nil)
logger.EXPECT().Debugf("Status of virtual machine: %s", "")
},
force: false,
force: false,
deleteDisk: false,
},
{
name: "unknown VM status",
wantErr: errors.New("unrecognized system status"),
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) {
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
getVMStatusC := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
getVMStatusC.EXPECT().Output().Return([]byte("Broken"), nil)
logger.EXPECT().Debugf("Status of virtual machine: %s", "Broken")
},
force: false,
force: false,
deleteDisk: false,
},
{
name: "status command returns an error",
wantErr: errors.New("get status error"),
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) {
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
getVMStatusC := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
getVMStatusC.EXPECT().Output().Return([]byte("Broken"), errors.New("get status error"))
},
force: false,
force: false,
deleteDisk: false,
},
{
name: "should print error if virtual machine failed to remove",
wantErr: errors.New("failed to remove instance"),
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) {
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
getVMStatusC := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
getVMStatusC.EXPECT().Output().Return([]byte("Stopped"), nil)
Expand All @@ -161,19 +206,58 @@ 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,
force: false,
deleteDisk: false,
},
{
name: "should forcibly remove the instance",
wantErr: nil,
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller) {
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
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,
deleteDisk: false,
},
{
name: "should remove the instance and user data",
wantErr: nil,
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
getVMStatusC := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
getVMStatusC.EXPECT().Output().Return([]byte("Stopped"), nil)
logger.EXPECT().Debugf("Status of virtual machine: %s", "Stopped")

command := mocks.NewCommand(ctrl)
creator.EXPECT().CreateWithoutStdio("remove", limaInstanceName).Return(command)
command.EXPECT().CombinedOutput()
logger.EXPECT().Info("Removing existing Finch virtual machine...")
logger.EXPECT().Info("Finch virtual machine removed successfully")
logger.EXPECT().Info("Deleting user data...")
dm.EXPECT().DeleteUserDataDisk(false).Return(nil)
logger.EXPECT().Info("Successfully deleted user data.")
},
force: false,
deleteDisk: true,
},
{
name: "should forcibly remove the instance and user data",
wantErr: nil,
mockSvc: func(logger *mocks.Logger, creator *mocks.LimaCmdCreator, ctrl *gomock.Controller, dm *mocks.MockUserDataDiskManager) {
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")
logger.EXPECT().Info("Forcibly deleting user data...")
dm.EXPECT().DeleteUserDataDisk(true).Return(nil)
logger.EXPECT().Info("Successfully deleted user data.")
},
force: true,
force: true,
deleteDisk: true,
},
}

Expand All @@ -185,9 +269,10 @@ func TestRemoveVMAction_run(t *testing.T) {
ctrl := gomock.NewController(t)
logger := mocks.NewLogger(ctrl)
lcc := mocks.NewLimaCmdCreator(ctrl)
dm := mocks.NewMockUserDataDiskManager(ctrl)

tc.mockSvc(logger, lcc, ctrl)
err := newRemoveVMAction(lcc, logger).run(tc.force)
tc.mockSvc(logger, lcc, ctrl, dm)
err := newRemoveVMAction(lcc, logger, dm).run(tc.force, tc.deleteDisk)
assert.Equal(t, tc.wantErr, err)
})
}
Expand Down
15 changes: 14 additions & 1 deletion e2e/vm/additional_disk_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const (
)

var testAdditionalDisk = func(o *option.Option) {
ginkgo.Describe("Additional disk", ginkgo.Serial, func() {
ginkgo.FDescribe("Additional disk", ginkgo.Serial, func() {
ginkgo.It("Retains container user data after the VM is deleted", func() {
command.Run(o, "pull", savedImage)
ginkgo.DeferCleanup(command.Run, o, "rmi", savedImage)
Expand All @@ -39,5 +39,18 @@ var testAdditionalDisk = func(o *option.Option) {
newPsOutput := command.StdoutStr(o, "ps", "--all", "--format", "{{.Names}}")
gomega.Expect(newPsOutput).Should(gomega.Equal(oldPsOutput))
})
ginkgo.FIt("Removes the user data on `finch vm remove --user-data`", func() {
command.Run(o, "pull", savedImage)
oldImagesOutput := command.StdoutStr(o, "images", "--format", "{{.Name}}")
gomega.Expect(oldImagesOutput).Should(gomega.ContainSubstring(savedImage))

command.New(o, virtualMachineRootCmd, "stop").WithoutCheckingExitCode().WithTimeoutInSeconds(90).Run()
command.Run(o, virtualMachineRootCmd, "remove", "--user-data")

command.New(o, virtualMachineRootCmd, "init").WithTimeoutInSeconds(240).Run()

newImagesOutput := command.StdoutStr(o, "images", "--format", "{{.Name}}")
gomega.Expect(newImagesOutput).ShouldNot(gomega.Equal(oldImagesOutput))
})
})
}
Loading

0 comments on commit 3542863

Please sign in to comment.