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.

Sign up for GitHub

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

Jump to bottom

Add TLV to the ICMP payload #11

Merged
merged 11 commits into from
Jan 12, 2022
Merged

Add TLV to the ICMP payload #11

merged 11 commits into from
Jan 12, 2022

Conversation

lolyu
Copy link
Contributor

@lolyu lolyu commented Dec 9, 2021 •
edited
Loading

Description of PR

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Test case(new/improvement)

Approach

What is the motivation for this PR?

Add TLV support for the ICMP payload.

How did you do it?

This PR introduces a few changes to IcmpPayload

  1. Change the IcmpPayload format as:
|cookie(4bytes)|version(4bytes)|uuid(8bytes)|seq(8bytes)|
  1. Introduce TlvCommand and TlvSentinel
  • TlvCommand is used for signaling peer ToR to switch status
|type(1bytes) = 0x1|length(2bytes) = 1|command(1bytes)|
  • TlvSentinel is used for marking the end of the ICMP packet
|type(1bytes) = 0xff|length(2bytes) = 0|

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@lolyu lolyu changed the title Add tlv [draft] Add tlv Dec 9, 2021
@lguohan
Copy link
Contributor

lguohan commented Dec 9, 2021

in general, can we get better description for the pr?

@lolyu lolyu force-pushed the add_tlv branch 4 times, most recently from 5479758 to da05b00 Compare December 13, 2021 13:41
@lolyu
Add TLV support to ICMP payload
189ff90 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu lolyu marked this pull request as ready for review December 13, 2021 14:55
@lolyu lolyu changed the title [draft] Add tlv Add TLV to the ICMP payload Dec 13, 2021
@lolyu
Copy link
Contributor Author

lolyu commented Dec 13, 2021

in general, can we get better description for the pr?

Updated, thanks!

@lolyu lolyu requested review from yxieca, lguohan and zjswhhh December 13, 2021 15:25
@lolyu lolyu force-pushed the add_tlv branch 2 times, most recently from f2fd3b4 to c062704 Compare December 16, 2021 01:57
@lolyu
Add sentinel TLV
b7ef68b 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu
Copy link
Contributor Author

lolyu commented Dec 17, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lolyu lolyu force-pushed the add_tlv branch 2 times, most recently from 1a2520e to 7edbd5e Compare December 21, 2021 06:26
@lolyu
Fix comments
0474e0d 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
zjswhhh
zjswhhh reviewed Dec 22, 2021
View reviewed changes
src/link_prober/LinkProber.cpp
icmpPayload->command = htonl(static_cast<uint32_t> (Command::COMMAND_NONE));
computeChecksum(icmpHeader, sizeof(icmphdr) + sizeof(*icmpPayload));
resetTxBufferTlv();
appendTlvSentinel();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we only append TlvCommand when it's COMMAND_SWITCH_ACTIVE instead of having it in every heartbeat (COMMAND_NONE)? Shouldn't we have it in every heartbeat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, currently the logic is to put command tlv only if there is need to send COMMAND_SWITCH_ACTIVE. Since now we remove the fixed command field from IcmpPayload and wrap it as a TLV, no TlvCommand is the same a TlvCommand with COMMAND_NONE.

src/link_prober/LinkProber.cpp Outdated Show resolved Hide resolved
lolyu added 2 commits January 6, 2022 05:17
@lolyu
Use union to represent tlv payload
8b00880 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu
Fix unittest
afc162e 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
lolyu added 5 commits January 6, 2022 08:48
@lolyu
Improve linting
fb47ee1 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu
Fix icmp peer event issue
2ed6178 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu
Remove redundant lines
ecc1e4e 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu
Fix unittest
51c2a2c 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu
Add unittest to test variable sized tlv
a4c9ec3 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu lolyu requested a review from yxieca January 7, 2022 05:17
@lolyu lolyu merged commit 0ba431f into sonic-net:master Jan 12, 2022
yxieca pushed a commit that referenced this pull request Jan 14, 2022
@lolyu @yxieca
Add TLV support to ICMP payload (#11)
12b9951 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
zjswhhh pushed a commit to zjswhhh/sonic-linkmgrd that referenced this pull request Jan 19, 2022
@lolyu @zjswhhh
Add TLV support to ICMP payload (sonic-net#11)
5438347 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
zjswhhh added a commit to zjswhhh/sonic-linkmgrd that referenced this pull request Jan 19, 2022
@zjswhhh
Revert "Add TLV support to ICMP payload (sonic-net#11)"
0dd8eda 
This reverts commit 12b9951.
zjswhhh pushed a commit to zjswhhh/sonic-linkmgrd that referenced this pull request Jan 24, 2022
@lolyu @zjswhhh
Add TLV support to ICMP payload (sonic-net#11)
aa03765 
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu lolyu deleted the add_tlv branch January 19, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bingwang-ms bingwang-ms bingwang-ms left review comments

@zjswhhh zjswhhh zjswhhh left review comments

@yxieca yxieca yxieca approved these changes

@lguohan lguohan Awaiting requested review from lguohan

Assignees
No one assigned
Labels
Included in 202012 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lolyu @lguohan @bingwang-ms @yxieca @zjswhhh