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 <[email protected]>

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.

Details

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

Signed-off-by: FingerLeader <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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.

Details

Commit hash: 2af752c

FingerLeader added 13 commits January 10, 2022 18:31

add new feature nic down

bacff83

Signed-off-by: FingerLeader <[email protected]>

add nic down

d836369

Signed-off-by: FingerLeader <[email protected]>

add nic down validate

b94544b

Signed-off-by: FingerLeader <[email protected]>

edit some details

6a7e0a7

Signed-off-by: FingerLeader <[email protected]>

edit some details

c591c0b

Signed-off-by: FingerLeader <[email protected]>

use SchedulerConfig

d0336c5

Signed-off-by: FingerLeader <[email protected]>

edit some details

e965e69

Signed-off-by: FingerLeader <[email protected]>

edit some details

f74fd55

Signed-off-by: FingerLeader <[email protected]>

edit some details

be47675

Signed-off-by: FingerLeader <[email protected]>

edit some details

2aef494

Signed-off-by: FingerLeader <[email protected]>

add support fot sub-interface

aab691f

Signed-off-by: FingerLeader <[email protected]>

add test for nic down

04f85b0

Signed-off-by: FingerLeader <[email protected]>

add test for nic down

ad42bf6

Signed-off-by: FingerLeader <[email protected]>

ti-chi-bot requested review from WangXiangUSTC and cwen0 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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

remove nohup

f569070

Signed-off-by: FingerLeader <[email protected]>

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 <[email protected]>

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 <[email protected]>

add some comments

9d0c306

Signed-off-by: FingerLeader <[email protected]>

add some comments

54e8072

Signed-off-by: FingerLeader <[email protected]>

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

Network: support down NIC #130

Network: support down NIC #130

Uh oh!

Conversation

FingerLeader commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-chi-bot commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WangXiangUSTC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WangXiangUSTC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwen0 left a comment

Choose a reason for hiding this comment

Uh oh!

cwen0 commented Mar 2, 2022

Uh oh!

ti-chi-bot commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FingerLeader commented Feb 17, 2022 •

edited

Loading

ti-chi-bot commented Feb 17, 2022 •

edited

Loading