Add container kill chaos #203

fewdan · 2020-02-07T08:08:26Z

What problem does this PR solve?

Add container kill

What is changed and how does it work?

Using client in chaos-daemon to achieve container-kill

Check List

Tests

No code

Code changes

Has Go code change

Side effects

No

Related changes

Need to update the documentation

Does this PR introduce a user-facing change?:

NONE

zhouqiang-cl · 2020-02-07T13:14:56Z

controllers/podchaos/containerkill/types.go

+	"errors"
+	"fmt"
+
+	v1 "k8s.io/api/core/v1"


please group import

The result after I executed "goimports -w -l -local github.com/pingcap/chaos-mesh ./" is like this. Maybe I can adjust it manually. But "goimports" in "makefile" will change back.

Goimports only groups and formats imported packages in the same group. Each group is a continous packages which is not seperated with Enter. You could group it with ctrl "sigs.k8s.io/controller-runtime"

thx. I have edited it.

zhouqiang-cl · 2020-02-07T13:16:02Z

controllers/podchaos/containerkill/types.go

+	Log logr.Logger
+}
+
+func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {


Can you split this function, it is too long

@fewdan PTAL

controllers/podchaos/types.go

zhouqiang-cl

rest LGTM

codecov-io · 2020-02-07T15:27:17Z

Codecov Report

❗ No coverage uploaded for pull request base (master@91dbba8). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #203   +/-   ##
=========================================
  Coverage          ?   43.64%           
=========================================
  Files             ?       18           
  Lines             ?      685           
  Branches          ?        0           
=========================================
  Hits              ?      299           
  Misses            ?      353           
  Partials          ?       33

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91dbba8...06c3ab4. Read the comment docs.

yeya24 · 2020-02-07T19:48:15Z

controllers/podchaos/containerkill/types.go

+
+func (r *Reconciler) KillContainer(ctx context.Context, pod *v1.Pod, containerID string) error {
+
+	r.Log.Info("try to kill container")


Is it better to add more info to this log? Maybe we can also show some info about the pod and container here. Just my personal thought

I have added "pod.Namespace", "pod.Name" and containerID.

controllers/podchaos/containerkill/types.go

pkg/chaosdaemon/container-kill.go

pkg/chaosdaemon/util.go

yeya24 · 2020-02-07T20:22:01Z

pkg/chaosdaemon/util.go

+		return err
+	}
+
+	err = container.Delete(ctx)


I think maybe we need to add the deleteOption containerd.WithSnapshotCleanup here. I am not familiar with containerd, but I check the implementation in container cri and ctr, they all have this option. I am not sure whether this is needed.

Also, I am not sure whether this is equivalent with sending SIGKILL to that container or not? There is already an implementation to do kill to the task, check here

yeya24 · 2020-02-07T20:26:54Z

pkg/chaosdaemon/pb/chaosdaemon.proto

+}
+
+message ContainerRequest{
+  string container_id = 1;
 }


Any design for this RPC request? For future scalability, I think kill is just one type of action to containers. So maybe we can add a field like type or action

yes. I think maybe other signal can be set in type

But this is ok currently. We can leave that change to future PR

Any design for this RPC request? For future scalability, I think kill is just one type of action to containers. So maybe we can add a field like type or action

I agree with your option, @fewdan you can add a field to represent the chaos action.

zhouqiang-cl · 2020-02-08T03:53:59Z

@fewdan PTAL

zhouqiang-cl

Maybe we also need support duration kill? @Yisaer @cwen0

AngleNet · 2020-02-08T14:56:11Z

config/crd/bases/pingcap.com_podchaos.yaml

@@ -38,6 +38,9 @@ spec:
              description: 'Action defines the specific pod chaos action. Supported
                action: pod-kill / pod-failure Default action: pod-kill'
              type: string
+            containerName:
+              description: Needed in container-kill. Indicates the name of the container


Could you please tweak the description so that it complies with "ContainerName blablabla..."

I changed the order. PTAL

AngleNet · 2020-02-08T14:57:53Z

controllers/podchaos/containerkill/types.go

+	"errors"
+	"fmt"
+
+	v1 "k8s.io/api/core/v1"


Goimports only groups and formats imported packages in the same group. Each group is a continous packages which is not seperated with Enter. You could group it with ctrl "sigs.k8s.io/controller-runtime"

controllers/podchaos/containerkill/types.go

manifests/crd.yaml

yeya24 · 2020-02-11T14:39:17Z

pkg/chaosdaemon/util.go

@@ -40,6 +41,7 @@ const (
 // ContainerRuntimeInfoClient represents a struct which can give you information about container runtime
 type ContainerRuntimeInfoClient interface {
 	GetPidFromContainerID(ctx context.Context, containerID string) (uint32, error)
+	ContainerKillByContainerID(ctx context.Context, containerID string) error


Actually I am still not sure about the naming. I think both GetPidFromContainerID and ContainerKillByContainerID is quite redundant. It is okay to just use GetPid and KillContainer. WDYT @fewdan @zhouqiang-cl

I'm not sure if there are other ways in the future, such as not based on "ID" but on "image" and the like. So I think we can name it longer now.

Sure. That makes sense 😄

yeya24 · 2020-02-11T14:42:52Z

controllers/podchaos/containerkill/types.go

+		podchaos.Status.Experiment.Pods = append(podchaos.Status.Experiment.Pods, ps)
+	}
+	if err := r.Update(ctx, &podchaos); err != nil {
+		r.Log.Error(err, "unable to update chaosctl status")


Hmm, may I ask what does this chaosctl mean

Actually, I see this "log msg" appear multiple times in the project. So this is consistent with other places.

It is not clear enough to users.

zhouqiang-cl · 2020-02-12T04:00:49Z

@fewdan could you please fix ci

…nto temp20200207

zhouqiang-cl · 2020-02-13T03:57:00Z

@fewdan we should also add doc, it can be another pr

…nto temp20200207

cwen0 · 2020-02-14T08:50:52Z

controllers/podchaos/containerkill/types.go

+			containerID := pod.Status.ContainerStatuses[containerIndex].ContainerID
+
+			if containerName == podchaos.Spec.ContainerName {
+				haveSelected = true


I think haveSelected is unnecessary, because If the only one from all selected pod met this name, we will think all pod meets the name.

…nto temp20200207

hound · 2020-02-14T09:50:50Z

controllers/podchaos/containerkill/types.go

+	Log logr.Logger
+}
+
+func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {


exported method Reconciler.Reconcile should have comment or be unexported

hound · 2020-02-14T09:50:51Z

controllers/podchaos/containerkill/types.go

+	containerKillActionMsg = "delete container %s"
+)
+
+type Reconciler struct {


exported type Reconciler should have comment or be unexported

zhouqiang-cl

LGTM

cwen0

LGTM

fewdan added 4 commits February 7, 2020 15:35

container kill

4b71e87

container kill

766fb03

container kill

05c71d5

container kill

f8e53b8

fewdan changed the title ~~Temp20200207~~ add container kill Feb 7, 2020

cwen0 added status/WIP component/operator labels Feb 7, 2020

zhouqiang-cl changed the title ~~add container kill~~ [WIP] add container kill Feb 7, 2020

zhouqiang-cl linked an issue Feb 7, 2020 that may be closed by this pull request

Add container kill #138

Closed

zhouqiang-cl reviewed Feb 7, 2020

View reviewed changes

controllers/podchaos/types.go Show resolved Hide resolved

zhouqiang-cl reviewed Feb 7, 2020

View reviewed changes

fewdan added 3 commits February 7, 2020 21:41

container kill

e4a77e6

container kill

ff4fab1

container kill

e208a22

yeya24 reviewed Feb 7, 2020

View reviewed changes

zhouqiang-cl removed the status/WIP label Feb 8, 2020

zhouqiang-cl changed the title ~~[WIP] add container kill~~ add container kill Feb 8, 2020

zhouqiang-cl added the status/ReqChange label Feb 8, 2020

zhouqiang-cl reviewed Feb 8, 2020

View reviewed changes

AngleNet requested changes Feb 8, 2020

View reviewed changes

container kill

416a2e4

yeya24 reviewed Feb 11, 2020

View reviewed changes

fewdan added 2 commits February 12, 2020 20:23

container kill

da17547

container kill

21cab00

fewdan and others added 3 commits February 13, 2020 11:19

Merge branch 'temp20200207' of https://github.com/fewdan/chaos-mesh i…

681a818

…nto temp20200207

container kill

4faaa12

Merge branch 'master' into temp20200207

01650fe

mahjonp previously approved these changes Feb 13, 2020

View reviewed changes

fewdan added 2 commits February 13, 2020 16:13

container kill

ec94490

Merge branch 'temp20200207' of https://github.com/fewdan/chaos-mesh i…

d299c4a

…nto temp20200207

fewdan dismissed mahjonp’s stale review via d299c4a February 13, 2020 08:14

fewdan and others added 2 commits February 13, 2020 16:21

container kill

91dbba8

Merge branch 'master' into temp20200207

8ac08b8

mahjonp previously approved these changes Feb 14, 2020

View reviewed changes

Merge branch 'master' into temp20200207

8f01859

zhouqiang-cl added status/PTAL and removed status/ReqChange labels Feb 14, 2020

cwen0 reviewed Feb 14, 2020

View reviewed changes

fewdan added 2 commits February 14, 2020 17:49

container kill

3ab059b

Merge branch 'temp20200207' of https://github.com/fewdan/chaos-mesh i…

06c3ab4

…nto temp20200207

fewdan dismissed mahjonp’s stale review via 06c3ab4 February 14, 2020 09:50

hound bot reviewed Feb 14, 2020

View reviewed changes

zhouqiang-cl approved these changes Feb 14, 2020

View reviewed changes

cwen0 approved these changes Feb 14, 2020

View reviewed changes

cwen0 merged commit cdb9c61 into chaos-mesh:master Feb 14, 2020

zhouqiang-cl changed the title ~~add container kill~~ Add container kill chaos Feb 14, 2020

fewdan deleted the temp20200207 branch February 17, 2020 07:03

dcalvin mentioned this pull request Feb 5, 2021

promote committers #1492

Merged

sjwsl pushed a commit to sjwsl/chaos-mesh that referenced this pull request May 6, 2021

add container kill (chaos-mesh#203)

367fa88

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add container kill chaos #203

Add container kill chaos #203

fewdan commented Feb 7, 2020

zhouqiang-cl Feb 7, 2020

fewdan Feb 7, 2020

AngleNet Feb 8, 2020

fewdan Feb 12, 2020

zhouqiang-cl Feb 7, 2020

zhouqiang-cl Feb 12, 2020

zhouqiang-cl left a comment

codecov-io commented Feb 7, 2020 •

edited

Loading

yeya24 Feb 7, 2020

fewdan Feb 11, 2020

yeya24 Feb 7, 2020

yeya24 Feb 7, 2020

zhouqiang-cl Feb 8, 2020

yeya24 Feb 8, 2020

cwen0 Feb 12, 2020

zhouqiang-cl commented Feb 8, 2020

zhouqiang-cl left a comment

AngleNet Feb 8, 2020

fewdan Feb 11, 2020

AngleNet Feb 8, 2020

yeya24 Feb 11, 2020

fewdan Feb 12, 2020

yeya24 Feb 12, 2020

yeya24 Feb 11, 2020

fewdan Feb 12, 2020

yeya24 Feb 12, 2020

zhouqiang-cl commented Feb 12, 2020

zhouqiang-cl commented Feb 13, 2020

cwen0 Feb 14, 2020

hound bot Feb 14, 2020

hound bot Feb 14, 2020

zhouqiang-cl left a comment

cwen0 left a comment


		func (r Reconciler) KillContainer(ctx context.Context, pod v1.Pod, containerID string) error {

		r.Log.Info("try to kill container")

Add container kill chaos #203

Add container kill chaos #203

Conversation

fewdan commented Feb 7, 2020

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

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

zhouqiang-cl left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 7, 2020 • edited Loading

Codecov Report

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

zhouqiang-cl commented Feb 8, 2020

zhouqiang-cl 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

zhouqiang-cl commented Feb 12, 2020

zhouqiang-cl commented Feb 13, 2020

Choose a reason for hiding this comment

hound bot Feb 14, 2020

Choose a reason for hiding this comment

hound bot Feb 14, 2020

Choose a reason for hiding this comment

zhouqiang-cl left a comment

Choose a reason for hiding this comment

cwen0 left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 7, 2020 •

edited

Loading