Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store iptables when creating network attack #215

Merged
merged 7 commits into from
Sep 14, 2022

Conversation

FingerLeader
Copy link
Member

Signed-off-by: root root@localhost.localdomain
Close #213

root added 2 commits September 13, 2022 21:19
Signed-off-by: root <root@localhost.localdomain>
Signed-off-by: root <root@localhost.localdomain>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 13, 2022

[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.

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

root added 2 commits September 13, 2022 21:58
Signed-off-by: root <root@localhost.localdomain>
@cwen0
Copy link
Member

cwen0 commented Sep 13, 2022

@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

image

@FingerLeader
Copy link
Member Author

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
Copy link
Member

cwen0 commented Sep 14, 2022

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 <root@localhost.localdomain>
@ti-chi-bot ti-chi-bot added size/L and removed size/M labels Sep 14, 2022
@FingerLeader
Copy link
Member Author

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

@@ -79,11 +79,9 @@ func (networkAttack) Attack(options core.AttackConfig, env Environment) (err err
return perrors.WithStack(err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can delete NeedApplyIptables() and NeedApplyIPSet directly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update this comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

Signed-off-by: root <root@localhost.localdomain>
Copy link
Member

@cwen0 cwen0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cwen0
Copy link
Member

cwen0 commented Sep 14, 2022

/merge

@ti-chi-bot
Copy link
Member

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

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.

@cwen0 cwen0 merged commit 98f3ca4 into chaos-mesh:main Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

network partition doesn't work when network delay was applied in advance
3 participants