feature: cli support add kill command #2924

lang710 · 2019-06-22T14:14:36Z

Signed-off-by: Lang Chi 21860405@zju.edu.cn

Ⅰ. Describe what this PR did

cli support add kill command

Ⅱ. Does this pull request fix one issue?

fixes #2917

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Added

Ⅳ. Describe how to verify it

root@compatibility:# pouch run -d busybox top
df0487e6cc386f9340d27e62ab8a3797e5ccccb5d199bbebb87a0155aa69a84f
root@compatibility:# pouch kill df0487e6cc386f9340d27e62ab8a3797e5ccccb5d199bbebb87a0155aa69a84f
df0487e6cc386f9340d27e62ab8a3797e5ccccb5d199bbebb87a0155aa69a84f
root@compatibility:# pouch run -d busybox top
75d8d902f78315b065c41e81a2a9967f7a3edf891c36bd50f36ab3b1691d9c73
root@compatibility:# pouch kill -s 0 75d8d902f78315b065c41e81a2a9967f7a3edf891c36bd50f36ab3b1691d9c73
Error: {"message":"Invalid signal: 0"}

Ⅴ. Special notes for reviews

codecov · 2019-06-22T14:14:38Z

Codecov Report

Merging #2924 into master will decrease coverage by 2.05%.
The diff coverage is 51.96%.

@@            Coverage Diff             @@
##           master    #2924      +/-   ##
==========================================
- Coverage   68.23%   66.17%   -2.06%     
==========================================
  Files         291      293       +2     
  Lines       18328    18445     +117     
==========================================
- Hits        12506    12206     -300     
- Misses       4368     4761     +393     
- Partials     1454     1478      +24

Flag	Coverage Δ
#criv1alpha2_test	`34.59% <0.98%> (-0.24%)`	⬇️
#integration_test_0	`35.99% <51.96%> (-0.08%)`	⬇️
#integration_test_1	`35.80% <0.98%> (+0.30%)`	⬆️
#integration_test_2	`?`
#integration_test_3	`35.34% <0.98%> (-0.30%)`	⬇️
#node_e2e_test	`33.94% <0.98%> (-0.42%)`	⬇️
#unittest	`27.78% <0.00%> (-0.21%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
client/container_kill.go	`0.00% <0.00%> (ø)`
daemon/mgr/container.go	`59.10% <ø> (-1.25%)`	⬇️
pkg/errtypes/errors.go	`76.00% <0.00%> (-15.31%)`	⬇️
daemon/mgr/container_kill.go	`43.75% <43.75%> (ø)`
pkg/utils/utils.go	`82.35% <60.00%> (-0.92%)`	⬇️
ctrd/container.go	`52.37% <68.00%> (-1.93%)`	⬇️
apis/server/container_bridge.go	`88.64% <80.00%> (-1.36%)`	⬇️
apis/server/router.go	`91.56% <100.00%> (+0.05%)`	⬆️
storage/volume/modules/tmpfs/tmpfs.go	`28.20% <0.00%> (-47.44%)`	⬇️
daemon/events/filter.go	`60.00% <0.00%> (-40.00%)`	⬇️
... and 42 more

ZYecho

in killWithSignal， I didn't find any call to containerd， so when pouch kill -2 xxx， what will happen？

ZYecho · 2019-06-23T01:58:59Z

apis/server/container_bridge.go

+		}
+	}
+
+	fmt.Printf("signal: %d\n", uint64(sig))


change to logrus.debug

cli/kill.go

ZYecho · 2019-06-23T02:01:58Z

daemon/mgr/container.go

@@ -68,6 +68,9 @@ type ContainerMgr interface {
 	// List returns the list of containers.
 	List(ctx context.Context, option *ContainerListOption) ([]*Container, error)

+	// Kill one or more running containers
+	Kill(ctx context.Context, name string, signal uint64) (err error)


the desc here I think is kill one running container， wdyt？

daemon/mgr/container_kill.go

ZYecho · 2019-06-23T02:06:01Z

daemon/mgr/container_kill.go

+	err := mgr.killWithSignal(ctx, c, signal)
+	if err == syscall.ESRCH {
+		e := errNoSuchProcess{c.State.Pid, signal}
+		logrus.Debug(e)


please add more msg to this err

ZYecho · 2019-06-23T02:07:40Z

daemon/mgr/container_kill.go

+			return nil
+		}
+
+		if c.IsRunning() {


why killDeadProcess don't lock the container here？

The function killDeadProcess will call the function killWithSignal. The latter will lock the container.

ZYecho · 2019-06-23T02:08:58Z

daemon/mgr/container_kill.go

+		err = fmt.Errorf("Cannot kill container %s: %s", c.ID, err)
+		if strings.Contains(err.Error(), "container not found") ||
+			strings.Contains(err.Error(), "no such process") {
+			logrus.Warnf("container kill failed because of 'container not found' or 'no such process': %s", err.Error())


return err？

Actually, return fmt.Errorf("Cannot kill container %s: %s", c.ID, err).

ZYecho

in killWithSignal， I didn't find any call to containerd， so when pouch kill -2 xxx， what will happen？

zhuangqh

the process inside the container is in another PID namespace. directly do syscall.Kill will kill the process on host. using containerd task kill instead.
distinguish kill and destroy. destroy is the combination of kill and delete, the latter one will remove the meta data.

ZYecho · 2019-06-23T12:23:37Z

add more input：#2733

ZYecho · 2019-06-26T12:15:06Z

test/cli_kill_test.go

+
+	"github.com/alibaba/pouch/test/command"
+	"github.com/alibaba/pouch/test/environment"
+	"github.com/go-check/check"


add a blank line

ZYecho · 2019-06-26T12:16:55Z

daemon/mgr/container_kill.go

+		return err
+	}
+
+	if signal == 0 || syscall.Signal(signal) == syscall.SIGKILL {


could you explain when signal = 0？

Thanks for your advice. I think that signal == 0 is redundant.

ZYecho · 2019-06-26T12:17:56Z

ctrd/container.go

+	}
+	// the caller need to execute the all hooks.
+	pack.l.Lock()
+	pack.skipStopHooks = true


why need this？

Actually I don't know if it is necessary to add these things. But if you want to kill a container, you should skip the stop hooks.

Signed-off-by: Lang Chi <21860405@zju.edu.cn>

allencloud · 2019-07-10T02:26:29Z

apis/server/container_bridge.go

+		}
+	}
+
+	if err := s.ContainerMgr.Kill(ctx, name, uint64(sig)); err != nil {


please just return s.ContainerMgr.Kill(ctx, name, uint64(sig)) to simplify code.

allencloud · 2019-07-10T02:29:36Z

daemon/mgr/container_kill.go

+	"github.com/pkg/errors"
+
+	"github.com/alibaba/pouch/pkg/errtypes"
+	"github.com/sirupsen/logrus"


please restruct the package import sequence.

allencloud · 2019-07-10T03:38:36Z

daemon/mgr/container_kill.go

+		}
+	}
+
+	if pid := c.State.Pid; pid != 0 {


Please use fast return to reduce ident:
https://github.com/alibaba/pouch/blob/master/docs/contributions/code_styles.md#rule006---return-fast-to-indent-less

allencloud · 2019-07-10T03:39:21Z

daemon/mgr/container_kill.go

+	defer c.Unlock()
+
+	if c.State.Paused {
+		return fmt.Errorf("Container %s is paused. Unpause the container before stopping", c.ID)


before killing?

allencloud · 2019-07-10T03:40:51Z

docs/commandline/pouch_kill.md

@@ -0,0 +1,49 @@
+## pouch kill


Please remove this file. This file is auto-generated. https://github.com/alibaba/pouch/tree/master/docs#commandline

What's more, all commandline docs are auto generated via source code

allencloud · 2019-07-10T03:41:41Z

pkg/utils/utils.go

+
+// ParseSignal translates a string to a valid syscall signal.
+// It returns an error if the signal map doesn't include the given signal.
+func ParseSignal(rawSignal string) (syscall.Signal, error) {


please add unit test for this function. Thanks.

pouchrobot · 2019-10-26T13:30:47Z

ping @lang710
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

CLAassistant · 2020-06-12T06:35:26Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

cxz66666 · 2022-07-10T06:49:52Z

ping @lang710
Can you have time to update those questions? Or I will closed this PR. Thank you for your response!

cxz66666 · 2022-07-11T03:15:33Z

Seem that author doesn't response for a long time, I open another PR but some different details(don't skip hooks), so I will close PR.

pouchrobot added areas/cli kind/feature size/XL labels Jun 22, 2019

lang710 force-pushed the addkill branch from 0eec6d6 to 5e35f9b Compare June 22, 2019 14:22

ZYecho approved these changes Jun 23, 2019

View reviewed changes

ZYecho self-requested a review June 23, 2019 02:12

ZYecho suggested changes Jun 23, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Jun 23, 2019

Weekly Digest (16 June, 2019 - 23 June, 2019) #2925

Closed

zhuangqh reviewed Jun 23, 2019

View reviewed changes

lang710 force-pushed the addkill branch 2 times, most recently from 1645ffb to 5ff3367 Compare June 23, 2019 08:46

lang710 force-pushed the addkill branch 3 times, most recently from ed40008 to fb7eb64 Compare June 24, 2019 05:47

ZYecho reviewed Jun 30, 2019

View reviewed changes

feature: add kill command

a3d222f

Signed-off-by: Lang Chi <21860405@zju.edu.cn>

lang710 force-pushed the addkill branch from fb7eb64 to a3d222f Compare July 2, 2019 02:27

weekly-digest bot mentioned this pull request Jul 7, 2019

Weekly Digest (30 June, 2019 - 7 July, 2019) #2943

Closed

allencloud reviewed Jul 10, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Jul 14, 2019

Weekly Digest (7 July, 2019 - 14 July, 2019) #2948

Closed

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Oct 22, 2019

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Oct 26, 2019

rudyfly requested a review from cxz66666 July 7, 2022 11:46

cxz66666 mentioned this pull request Jul 11, 2022

feat: support kill command #3034

Merged

cxz66666 closed this Jul 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: cli support add kill command #2924

feature: cli support add kill command #2924

lang710 commented Jun 22, 2019

codecov bot commented Jun 22, 2019 •

edited

Loading

ZYecho left a comment

ZYecho Jun 23, 2019

ZYecho Jun 23, 2019

lang710 Jun 23, 2019

ZYecho Jun 23, 2019

lang710 Jun 24, 2019

ZYecho Jun 23, 2019

lang710 Jun 24, 2019

ZYecho Jun 23, 2019

lang710 Jun 24, 2019

ZYecho left a comment

zhuangqh left a comment

ZYecho commented Jun 23, 2019

ZYecho Jun 26, 2019

lang710 Jul 2, 2019

ZYecho Jun 26, 2019

lang710 Jul 2, 2019

ZYecho Jun 26, 2019

lang710 Jul 2, 2019

allencloud Jul 10, 2019

allencloud Jul 10, 2019

allencloud Jul 10, 2019

allencloud Jul 10, 2019

allencloud Jul 10, 2019

allencloud Jul 10, 2019

pouchrobot commented Oct 26, 2019

CLAassistant commented Jun 12, 2020 •

edited

Loading

cxz66666 commented Jul 10, 2022

cxz66666 commented Jul 11, 2022

feature: cli support add kill command #2924

feature: cli support add kill command #2924

Conversation

lang710 commented Jun 22, 2019

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Jun 22, 2019 • edited Loading

Codecov Report

ZYecho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZYecho left a comment

Choose a reason for hiding this comment

zhuangqh left a comment

Choose a reason for hiding this comment

ZYecho commented Jun 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pouchrobot commented Oct 26, 2019

CLAassistant commented Jun 12, 2020 • edited Loading

cxz66666 commented Jul 10, 2022

cxz66666 commented Jul 11, 2022

codecov bot commented Jun 22, 2019 •

edited

Loading

CLAassistant commented Jun 12, 2020 •

edited

Loading