Skip to content

Commit

Permalink
CLI Remove command improvements (#224)
Browse files Browse the repository at this point in the history 
[#217] CLI Remove command improvements
 - Timeout was added to the remove command
 - Timeout was changed in the stop command
 - Changed default container stop timeout to duration string

Signed-off-by: Daniel Milchev [email protected]
  • Loading branch information
@daniel-milchev @k-gostev
daniel-milchev authored and k-gostev committed Apr 30, 2024
1 parent e0bd86f commit f4ee3c6
Show file tree
Hide file tree
Showing 41 changed files with 534 additions and 332 deletions.
395 changes: 206 additions & 189 deletions containerm/api/services/containers/containers.pb.go
View file

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions containerm/api/services/containers/containers.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ message RemoveContainerRequest {

// Whether the container should be removed disregarding its current state.
bool force = 2;
github.com.eclipse_kanto.container_management.containerm.api.types.containers.StopOptions stopOptions = 3;
}

message GetLogsRequest {
Expand Down
2 changes: 1 addition & 1 deletion containerm/cli/cli_command_ctrs_list.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func (cc *listCmd) run(args []string) error {
return nil
}
if len(ctrs) == 0 {
fmt.Println("No found containers.")
fmt.Println("No containers found.")
} else {
prettyPrint(ctrs)
}
Expand Down
26 changes: 17 additions & 9 deletions containerm/cli/cli_command_ctrs_remove.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ type removeCmd struct {
}

type removeConfig struct {
force bool
name string
force bool
name string
timeout string
}

func (cc *removeCmd) init(cli *cli) {
Expand All @@ -48,22 +49,28 @@ func (cc *removeCmd) init(cli *cli) {

func (cc *removeCmd) run(args []string) error {
var (
ctr *types.Container
err error
ctx = context.Background()
errs errorutil.CompoundError
ctr *types.Container
err error
ctx = context.Background()
errs errorutil.CompoundError
stopOpts *types.StopOpts
)
// parse parameters
if cc.config.force && cc.config.timeout != "" {
stopOpts = &types.StopOpts{Force: true}
if stopOpts.Timeout, err = durationStringToSeconds(cc.config.timeout); err != nil {
return err
}
}
if len(args) == 0 {
if ctr, err = utilcli.ValidateContainerByNameArgsSingle(ctx, nil, cc.config.name, cc.cli.gwManClient); err != nil {
return err
}
return cc.cli.gwManClient.Remove(ctx, ctr.ID, cc.config.force)
return cc.cli.gwManClient.Remove(ctx, ctr.ID, cc.config.force, stopOpts)
}
for _, arg := range args {
ctr, err = utilcli.ValidateContainerByNameArgsSingle(ctx, []string{arg}, cc.config.name, cc.cli.gwManClient)
if err == nil {
if err = cc.cli.gwManClient.Remove(ctx, ctr.ID, cc.config.force); err != nil {
if err = cc.cli.gwManClient.Remove(ctx, ctr.ID, cc.config.force, stopOpts); err != nil {
errs.Append(err)
}
} else {
Expand All @@ -83,4 +90,5 @@ func (cc *removeCmd) setupFlags() {
flagSet.BoolVarP(&cc.config.force, "force", "f", false, "Force stopping before removing a container")
// init name flags
flagSet.StringVarP(&cc.config.name, "name", "n", "", "Remove a container with a specific name.")
flagSet.StringVarP(&cc.config.timeout, "time", "t", "", "Sets the timeout period to gracefully stop the container as duration string, e.g. 15s or 1m15s. When timeout expires the container process would be forcibly killed. If not specified the daemon default container stop timeout will be used.")
}
77 changes: 53 additions & 24 deletions containerm/cli/cli_command_ctrs_remove_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ import (

const (
// command flags
removeCmdFlagForce = "force"
removeCmdFlagName = "name"
removeCmdFlagForce = "force"
removeCmdFlagName = "name"
removeCmdFlagTimeout = "time"

// test input constants
removeContainerID = "test-ctr"
Expand All @@ -53,12 +54,14 @@ func TestRemoveCmdSetupFlags(t *testing.T) {
rmCliTest.init()

expectedCfg := removeConfig{
force: true,
name: removeContainerName,
force: true,
name: removeContainerName,
timeout: "50",
}
flagsToApply := map[string]string{
removeCmdFlagForce: strconv.FormatBool(expectedCfg.force),
removeCmdFlagName: expectedCfg.name,
removeCmdFlagForce: strconv.FormatBool(expectedCfg.force),
removeCmdFlagName: expectedCfg.name,
removeCmdFlagTimeout: expectedCfg.timeout,
}

execTestSetupFlags(t, rmCliTest, flagsToApply, expectedCfg)
Expand Down Expand Up @@ -87,8 +90,9 @@ func (rmTc *removeCommandTest) commandConfig() interface{} {

func (rmTc *removeCommandTest) commandConfigDefault() interface{} {
return removeConfig{
force: false,
name: "",
force: false,
name: "",
timeout: "",
}
}

Expand Down Expand Up @@ -176,14 +180,30 @@ func (rmTc *removeCommandTest) generateRunExecutionConfigs() map[string]testRunE
flags: map[string]string{removeCmdFlagForce: "true"},
mockExecution: rmTc.mockExecForceRemoveError,
},
"test_remove_with_timeout": {
args: stopCmdArgs,
flags: map[string]string{
removeCmdFlagForce: "true",
stopCmdFlagTimeout: "20s",
},
mockExecution: rmTc.mockExecRemoveWithTimeout,
},
"test_remove_with_timeout_round": {
args: stopCmdArgs,
flags: map[string]string{
removeCmdFlagForce: "true",
stopCmdFlagTimeout: "19.7s",
},
mockExecution: rmTc.mockExecRemoveWithTimeout,
},
}
}

// Mocked executions ---------------------------------------------------------------------------------
func (rmTc *removeCommandTest) mockExecRemoveIDAndName(args []string) error {
rmTc.mockClient.EXPECT().Get(context.Background(), args[0]).Times(0)
rmTc.mockClient.EXPECT().List(context.Background(), gomock.Any()).Times(0)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false).Times(0)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false, nil).Times(0)
return log.NewError("Container ID and --name (-n) cannot be provided at the same time - use only one of them")
}

Expand All @@ -201,7 +221,7 @@ func (rmTc *removeCommandTest) mockExecRemoveNoErrors(args []string) error {
Name: removeContainerName,
}
rmTc.mockClient.EXPECT().Get(context.Background(), args[0]).Times(1).Return(ctr, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false).Times(1).Return(nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false, nil).Times(1).Return(nil)
// no error expected
return nil
}
Expand All @@ -218,8 +238,8 @@ func (rmTc *removeCommandTest) mockExecRemoveMultipleNoErrors(args []string) err
}
rmTc.mockClient.EXPECT().Get(context.Background(), args[0]).Times(1).Return(ctr1, nil)
rmTc.mockClient.EXPECT().Get(context.Background(), args[1]).Times(1).Return(ctr2, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false).Times(1).Return(nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[1], false).Times(1).Return(nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false, nil).Times(1).Return(nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[1], false, nil).Times(1).Return(nil)
// no error expected
return nil
}
Expand All @@ -235,7 +255,7 @@ func (rmTc *removeCommandTest) mockExecRemoveMultipleWithErrors(args []string) e
}
rmTc.mockClient.EXPECT().Get(context.Background(), args[0]).Times(1).Return(nil, err1)
rmTc.mockClient.EXPECT().Get(context.Background(), args[1]).Times(1).Return(ctr, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[1], false).Times(1).Return(err2)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[1], false, nil).Times(1).Return(err2)
errs.Append(err1, err2)

return errors.New(errs.ErrorWithMessage("containers couldn't be removed due to the following reasons: "))
Expand All @@ -249,15 +269,15 @@ func (rmTc *removeCommandTest) mockExecRemoveError(args []string) error {
Name: removeContainerName,
}
rmTc.mockClient.EXPECT().Get(context.Background(), args[0]).Times(1).Return(ctr, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false).Times(1).Return(err)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false, nil).Times(1).Return(err)
return err
}

func (rmTc *removeCommandTest) mockExecRemoveByNameNoErrors(args []string) error {
// setup expected calls
ctrs := []*types.Container{{ID: removeContainerID, Name: removeContainerName}}
rmTc.mockClient.EXPECT().List(context.Background(), gomock.AssignableToTypeOf(client.WithName(removeContainerName))).Times(1).Return(ctrs, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), ctrs[0].ID, false).Times(1).Return(nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), ctrs[0].ID, false, nil).Times(1).Return(nil)
// no error expected
return nil
}
Expand All @@ -267,7 +287,7 @@ func (rmTc *removeCommandTest) mockExecRemoveByNameError(args []string) error {
err := errors.New("failed to remove container")
ctrs := []*types.Container{{ID: removeContainerID, Name: removeContainerName}}
rmTc.mockClient.EXPECT().List(context.Background(), gomock.AssignableToTypeOf(client.WithName(removeContainerName))).Times(1).Return(ctrs, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), ctrs[0].ID, false).Times(1).Return(err)
rmTc.mockClient.EXPECT().Remove(context.Background(), ctrs[0].ID, false, nil).Times(1).Return(err)
// no error expected
return err
}
Expand All @@ -276,43 +296,43 @@ func (rmTc *removeCommandTest) mockExecRemoveByNameListError(args []string) erro
// setup expected calls
err := errors.New("failed to list containers")
rmTc.mockClient.EXPECT().List(context.Background(), gomock.AssignableToTypeOf(client.WithName(removeContainerName))).Times(1).Return(nil, err)
rmTc.mockClient.EXPECT().Remove(context.Background(), gomock.Any(), false).Times(0)
rmTc.mockClient.EXPECT().Remove(context.Background(), gomock.Any(), false, nil).Times(0)
return err
}

func (rmTc *removeCommandTest) mockExecRemoveByNameListNilCtrs(args []string) error {
// setup expected calls
rmTc.mockClient.EXPECT().List(context.Background(), gomock.AssignableToTypeOf(client.WithName(removeContainerName))).Times(1).Return(nil, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), gomock.Any(), false).Times(0)
rmTc.mockClient.EXPECT().Remove(context.Background(), gomock.Any(), false, nil).Times(0)
return log.NewErrorf("The requested container with name = %s was not found. Try using an ID instead.", removeContainerName)
}

func (rmTc *removeCommandTest) mockExecRemoveByNameListZeroCtrs(args []string) error {
// setup expected calls
rmTc.mockClient.EXPECT().List(context.Background(), gomock.AssignableToTypeOf(client.WithName(removeContainerName))).Times(1).Return([]*types.Container{}, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), gomock.Any(), false).Times(0)
rmTc.mockClient.EXPECT().Remove(context.Background(), gomock.Any(), false, nil).Times(0)
return log.NewErrorf("The requested container with name = %s was not found. Try using an ID instead.", removeContainerName)
}

func (rmTc *removeCommandTest) mockExecRemoveByNameListMoreThanOneCtrs(args []string) error {
// setup expected calls
rmTc.mockClient.EXPECT().List(context.Background(), gomock.AssignableToTypeOf(client.WithName(removeContainerName))).Times(1).Return([]*types.Container{{}, {}}, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), gomock.Any(), false).Times(0)
rmTc.mockClient.EXPECT().Remove(context.Background(), gomock.Any(), false, nil).Times(0)
return log.NewErrorf("There are more than one containers with name = %s. Try using an ID instead.", removeContainerName)
}

func (rmTc *removeCommandTest) mockExecRemoveGetNilError(args []string) error {
// setup expected calls
rmTc.mockClient.EXPECT().Get(context.Background(), args[0]).Times(1).Return(nil, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false).Times(0)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false, nil).Times(0)
return log.NewErrorf("The requested container with ID = %s was not found.", args[0])
}

func (rmTc *removeCommandTest) mockExecRemoveGetError(args []string) error {
// setup expected calls
err := errors.New("failed to remove container")
rmTc.mockClient.EXPECT().Get(context.Background(), args[0]).Times(1).Return(nil, err)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false).Times(0)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], false, nil).Times(0)
return err
}

Expand All @@ -323,7 +343,7 @@ func (rmTc *removeCommandTest) mockExecForceRemoveNoErrors(args []string) error
Name: removeContainerName,
}
rmTc.mockClient.EXPECT().Get(context.Background(), args[0]).Times(1).Return(ctr, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], true).Times(1).Return(nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], true, nil).Times(1).Return(nil)
// no error expected
return nil
}
Expand All @@ -336,7 +356,16 @@ func (rmTc *removeCommandTest) mockExecForceRemoveError(args []string) error {
Name: removeContainerName,
}
rmTc.mockClient.EXPECT().Get(context.Background(), args[0]).Times(1).Return(ctr, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], true).Times(1).Return(err)
rmTc.mockClient.EXPECT().Remove(context.Background(), args[0], true, nil).Times(1).Return(err)
// no error expected
return err
}

func (rmTc *removeCommandTest) mockExecRemoveWithTimeout(args []string) error {
ctr := &types.Container{
ID: args[0],
}
rmTc.mockClient.EXPECT().Get(context.Background(), args[0]).Times(1).Return(ctr, nil)
rmTc.mockClient.EXPECT().Remove(context.Background(), ctr.ID, true, &types.StopOpts{Timeout: 20, Force: true}).Times(1).Return(nil)
return nil
}
21 changes: 16 additions & 5 deletions containerm/cli/cli_command_ctrs_stop.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ package main

import (
"context"
"math"
"time"

"github.com/eclipse-kanto/container-management/containerm/containers/types"
"github.com/eclipse-kanto/container-management/containerm/util"
Expand All @@ -28,7 +28,7 @@ type stopCmd struct {
}

type stopConfig struct {
timeout int64
timeout string
name string
force bool
signal string
Expand Down Expand Up @@ -63,19 +63,30 @@ func (cc *stopCmd) run(args []string) error {
Force: cc.config.force,
Signal: cc.config.signal,
}
if cc.config.timeout != math.MinInt64 {
stopOpts.Timeout = cc.config.timeout
if stopOpts.Timeout, err = durationStringToSeconds(cc.config.timeout); err != nil {
return err
}
if err = util.ValidateStopOpts(stopOpts); err != nil {
return err
}
return cc.cli.gwManClient.Stop(ctx, container.ID, stopOpts)
}

func durationStringToSeconds(duration string) (int64, error) {
if duration == "" {
return 0, nil
}
stopTime, err := time.ParseDuration(duration)
if err != nil {
return 0, err
}
return int64(stopTime.Round(time.Second).Seconds()), nil
}

func (cc *stopCmd) setupFlags() {
flagSet := cc.cmd.Flags()
// init timeout flag
flagSet.Int64VarP(&cc.config.timeout, "time", "t", math.MinInt64, "Sets the timeout period in seconds to gracefully stop the container. When timeout expires the container process would be forcibly killed.")
flagSet.StringVarP(&cc.config.timeout, "time", "t", "", "Sets the timeout period to gracefully stop the container as duration string, e.g. 15s or 1m15s. When timeout expires the container process would be forcibly killed. If not specified the daemon default container stop timeout will be used.")
// init name flag
flagSet.StringVarP(&cc.config.name, "name", "n", "", "Stop a container with a specific name.")
// init force flag
Expand Down
Loading

0 comments on commit f4ee3c6

Please sign in to comment.