Network: support down NIC #130

FingerLeader · 2022-02-17T10:51:09Z

Notice: The parameter duration is set to mandatory, because disabling the remote server's NIC is dangerous.

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

ti-chi-bot · 2022-02-17T10:51:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

WangXiangUSTC
cwen0

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

WangXiangUSTC · 2022-02-21T03:21:24Z

pkg/server/chaosd/network.go

+		NICDownCommand := fmt.Sprintf("ifconfig %s down", attack.Device)
+
+		cmd := exec.Command("bash", "-c", NICDownCommand)
+		if err = cmd.Start(); err != nil {


I run it under a non-root user, it runs failed but did not report any error here. Because it will not wait for the command to complete.
https://pkg.go.dev/os/exec#Cmd.Start
It's better to use cmd.CombinedOutput.

But using cmd.CombinedOutput or cmd.Run will make process stalled when exectuing sleep $duration, is it acceptable?

I think it's OK. chaosd should't exit before recover it

If chaosd will not exit, I think nohup script is useless. Should I remove it?

Yeah, we can remove the nohup in cmd

pkg/core/network.go

pkg/server/chaosd/network.go

WangXiangUSTC · 2022-02-21T06:22:50Z

pkg/server/chaosd/network.go

+	return nil
+}
+
+func (s *Server) recoverNICDownScheduled(attack *core.NetworkCommand) error {


In this way, we need to set the experiment's status to Destroyed.

I think it's better to refine the schedule framework, and let it recover automatically by the schedule framework.

We can refine the schedule framework in next pr

Yes, this solution is temporary, real schedule is need, especially for some dangerous experiments.

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

WangXiangUSTC

rest LGTM

WangXiangUSTC · 2022-02-22T05:51:14Z

pkg/server/chaosd/network.go

+			return errors.WithStack(err)
+		}
+
+		if attack.Duration != "-1" {


is this should be len(attack.Duration) != 0?

len(attack.Duration) has been checked before this function, -1 means attack will run continuously. It will be marked in doc.

Why not check this field before executing this attack?

This field has been checked for necessity before executing, if len(n.Duration) == 0 { return errors.New("duration is required") }, the judgement here is only to decide whether to run continuously.

I remember you add a note that the attack.Duration is required, but if users set it to -1, this attack won't be recovered automatically. This behavior is very dangerous. Can we forbid users to set it to -1?

Actually, this -1 option is added on purpose, because I think some user may need this option to let the experiment run continuously, especially those who run NIC down locally rather than remotely. And attack.Duration is hardly ever set to negative numbers, I think -1 is enough to be a warning sign (maybe -2333 or -32768 is better).

WangXiangUSTC

LGTM

cwen0 · 2022-03-02T03:40:36Z

pkg/server/chaosd/network.go

+	recoverCmd := exec.Command("bash", "-c", NICUpCommand)
+	stdout, err := recoverCmd.CombinedOutput()
+	if err != nil {
+		log.Error(recoverCmd.String()+string(stdout), zap.Error(err))


We can delete this log. refer to https://github.com/chaos-mesh/rfcs/blob/main/text/2021-12-09-logging.md#relations-between-error-and-logging

I removed these log.Error, PTAL : )

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

cwen0 · 2022-03-02T06:51:48Z

pkg/server/chaosd/network.go

+	if err != nil {
+		return errors.WithStack(err)
+	}
+	attack.IPAddress = strings.TrimRight(string(stdoutBytes), "\n\x00")


Add some comments to explain why need to trim \x00

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

cwen0

LGTM

cwen0 · 2022-03-02T10:34:40Z

/merge

ti-chi-bot · 2022-03-02T10:34:42Z

This pull request has been accepted and is ready to merge.

Commit hash: 2af752c

FingerLeader added 13 commits January 10, 2022 18:31

add new feature nic down

bacff83

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

add nic down

d836369

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

add nic down validate

b94544b

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

edit some details

6a7e0a7

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

edit some details

c591c0b

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

use SchedulerConfig

d0336c5

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

edit some details

e965e69

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

edit some details

f74fd55

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

edit some details

be47675

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

edit some details

2aef494

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

add support fot sub-interface

aab691f

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

add test for nic down

04f85b0

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

add test for nic down

ad42bf6

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

ti-chi-bot requested review from cwen0 and WangXiangUSTC February 17, 2022 10:51

ti-chi-bot added the size/L label Feb 17, 2022

WangXiangUSTC mentioned this pull request Feb 18, 2022

network attack: support down network device #120

Open

use bash to implement nic down

e19ff4a

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

FingerLeader force-pushed the add-down-nic branch from b864518 to e19ff4a Compare February 18, 2022 11:27

FingerLeader and others added 2 commits February 18, 2022 19:30

Merge branch 'main' into add-down-nic

b910e22

edit some details

757cca1

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

WangXiangUSTC reviewed Feb 21, 2022

View reviewed changes

FingerLeader added 2 commits February 22, 2022 11:46

replace cmd.Start with cmd.CombinedOutput

6d9be7b

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

remove nohup

f569070

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

WangXiangUSTC reviewed Feb 22, 2022

View reviewed changes

WangXiangUSTC approved these changes Feb 23, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 label Feb 23, 2022

cwen0 reviewed Mar 2, 2022

View reviewed changes

remove useless log.Error

899ed84

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

cwen0 reviewed Mar 2, 2022

View reviewed changes

FingerLeader added 3 commits March 2, 2022 15:27

add some comments

a250152

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

add some comments

9d0c306

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

add some comments

54e8072

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

cwen0 approved these changes Mar 2, 2022

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels Mar 2, 2022

Merge branch 'main' into add-down-nic

2af752c

ti-chi-bot added the status/can-merge label Mar 2, 2022

ti-chi-bot merged commit 81dc95a into chaos-mesh:main Mar 2, 2022

FingerLeader deleted the add-down-nic branch March 5, 2022 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network: support down NIC #130

Network: support down NIC #130

FingerLeader commented Feb 17, 2022 •

edited

Loading

ti-chi-bot commented Feb 17, 2022 •

edited

Loading

WangXiangUSTC Feb 21, 2022

FingerLeader Feb 21, 2022

WangXiangUSTC Feb 21, 2022

FingerLeader Feb 21, 2022

WangXiangUSTC Feb 21, 2022

WangXiangUSTC Feb 21, 2022

WangXiangUSTC Feb 21, 2022

FingerLeader Feb 21, 2022

WangXiangUSTC left a comment

WangXiangUSTC Feb 22, 2022

FingerLeader Feb 22, 2022

cwen0 Mar 2, 2022

FingerLeader Mar 2, 2022

cwen0 Mar 2, 2022

FingerLeader Mar 2, 2022

WangXiangUSTC left a comment

cwen0 Mar 2, 2022

FingerLeader Mar 2, 2022

cwen0 Mar 2, 2022

FingerLeader Mar 2, 2022

cwen0 left a comment

cwen0 commented Mar 2, 2022

ti-chi-bot commented Mar 2, 2022

Network: support down NIC #130

Network: support down NIC #130

Conversation

FingerLeader commented Feb 17, 2022 • edited Loading

ti-chi-bot commented Feb 17, 2022 • edited Loading

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

WangXiangUSTC 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

WangXiangUSTC 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

cwen0 left a comment

Choose a reason for hiding this comment

cwen0 commented Mar 2, 2022

ti-chi-bot commented Mar 2, 2022

FingerLeader commented Feb 17, 2022 •

edited

Loading

ti-chi-bot commented Feb 17, 2022 •

edited

Loading