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

Revert "[dualtor] Leave icmp_responder running on dualtor topology" #9084

Closed
wants to merge 1 commit into from

Conversation

lolyu
Copy link
Contributor

@lolyu lolyu commented Jul 21, 2023

Reverts #8909

The rationale of this reversion:

It appeared that leaving icmp_responder running will have unexpected dataplane impact:

  1. with garp_service running, the icmp probe replies will pollute the fdb table, those tests that assigns Vlan subnet address to ptf port will be not able to verify the traffic forwarding as the fdb mapping is not correct.
  2. without garp_service running, linkmgrd will send heartbeats with dest mac as zero mac, and the probe replies that has zero mac as src mac will be dropped by the root fanout.

So let's revert it for now, and add it back when we have better solutions for the above two impacts.

Copy link
Contributor

@wsycqyz wsycqyz left a comment

Choose a reason for hiding this comment

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

LGTM

@zjswhhh
Copy link
Contributor

zjswhhh commented Jul 21, 2023

Hi @lolyu - can you elaborate impact#1. If you mean icmp_responder is replying non link prober requests, we should update icmp_responder.

For #2, not understanding why this is an impact. The reply is anyway not there with or without icmp_responder running.

@zjswhhh
Copy link
Contributor

zjswhhh commented Jul 21, 2023

Created internal bug item#24616436 to track.

@lolyu lolyu closed this Jul 24, 2023
@lolyu lolyu reopened this Jul 25, 2023
@lolyu lolyu closed this Jul 26, 2023
@wangxin wangxin deleted the revert-8909-icmp_res_pub_mas branch July 31, 2023 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants