Store iptables when creating network attack #215

FingerLeader · 2022-09-13T13:28:24Z

Signed-off-by: root [email protected]
Close #213

ti-chi-bot · 2022-09-13T13:28:26Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

…into fix-partition

cwen0 · 2022-09-13T15:10:35Z

@FingerLeader I test this pr on my environment, I found the network delay still cancel when I applied a partition action.

./bin/chaosd version
Chaosd Version: version.Info{GitVersion:"fix-partition-g83eccfacc55fe7", GitCommit:"83eccfacc55fe7a29c102df7cb05daa2ea1ac00f", BuildDate:"2022-09-13T15:03:27Z", GoVersion:"go1.19", Compiler:"gc", Platform:"linux/amd64"} 
./bin/chaosd attack network delay -i 39.156.66.10 -d eth0 -l 200ms
./bin/chaosd attack network partition -i 140.205.220.96 -d eth0

FingerLeader · 2022-09-13T16:54:36Z

I found the network delay still cancel when I applied a partition action.
./bin/chaosd version

I test it again and find it works. Maybe you can delete ./bin/chaosd.dat and try again.

cwen0 · 2022-09-14T02:39:35Z

I found the network delay still cancel when I applied a partition action.
./bin/chaosd version
I test it again and find it works. Maybe you can delete ./bin/chaosd.dat and try again.

OK, I will test it again

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

FingerLeader · 2022-09-14T12:19:57Z

@cwen0 I have updated, please have a try again.

cwen0 · 2022-09-14T15:10:50Z

pkg/server/chaosd/network.go


 		if attack.NeedApplyIptables() {
 			if err = env.Chaos.applyIptables(attack, ipsetName, env.AttackUid); err != nil {
 				return perrors.WithStack(err)


We can delete NeedApplyIptables() and NeedApplyIPSet directly.

I think leaving them can make the code logic clearly, and they may be needed to add features later (by the way, NeedApplyIPSet() is working now)

I think the logic will be clearer If we delete them. The logic: Chaos Mesh will reset all network action when you apply the network actions. This logic will be more simple.

OK, I think it's OK to be the same as chaos mesh, NeedApplyIptables can be deleted, but I find chaosd attack network delay -l 1s -d eth0 will not work without NeedApplyIPSet()

cwen0 · 2022-09-14T15:12:17Z

pkg/server/chaosd/network.go

-			if err := env.Chaos.recoverIPSet(env.AttackUid); err != nil {
-				return perrors.WithStack(err)
-			}
+		// `chaosdaemon.DeamonServer.SetTcs()` may build new iptables which will not be recorded in DB,


We should update this comment.

I don't think this comment should be updated, the filter iptables still not be stored in the DB

Yes, but here we recover all because chaos mesh will apply all network actions when we apply a new one.

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

cwen0

LGTM

cwen0 · 2022-09-14T16:38:59Z

/merge

ti-chi-bot · 2022-09-14T16:39:01Z

@cwen0: /merge in this pull request requires 2 approval(s).

Details

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

root added 2 commits September 13, 2022 21:19

store iptables

f4e1f42

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

make check

8db22b3

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

Merge branch 'main' into fix-partition

8b3c320

ti-chi-bot requested review from Colstuwjx and Hexilee September 13, 2022 13:28

ti-chi-bot added the size/M label Sep 13, 2022

FingerLeader requested a review from cwen0 September 13, 2022 13:29

root added 2 commits September 13, 2022 21:58

update network test

15a185a

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

Merge branch 'fix-partition' of https://github.com/FingerLeader/chaosd …

83eccfa

…into fix-partition

move NeedApplyTC() at the begin of applying to the middle

a0efa92

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

ti-chi-bot added size/L and removed size/M labels Sep 14, 2022

cwen0 reviewed Sep 14, 2022

View reviewed changes

remove NeedApplyIptables

2d423a5

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

cwen0 approved these changes Sep 14, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 label Sep 14, 2022

cwen0 merged commit 98f3ca4 into chaos-mesh:main Sep 14, 2022

Store iptables when creating network attack #215

Store iptables when creating network attack #215

Uh oh!

Conversation

FingerLeader commented Sep 13, 2022

Uh oh!

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

Uh oh!

Uh oh!

cwen0 commented Sep 13, 2022

Uh oh!

FingerLeader commented Sep 13, 2022

Uh oh!

cwen0 commented Sep 14, 2022

Uh oh!

FingerLeader commented Sep 14, 2022

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!

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 Sep 14, 2022

Uh oh!

ti-chi-bot commented Sep 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ti-chi-bot commented Sep 13, 2022 •

edited

Loading