Add TLV to the ICMP payload by lolyu · Pull Request #11 · sonic-net/sonic-linkmgrd

lolyu · 2021-12-09T13:10:50Z

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

Change the IcmpPayload format as:

|cookie(4bytes)|version(4bytes)|uuid(8bytes)|seq(8bytes)|

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

lguohan · 2021-12-09T18:34:26Z

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

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

lolyu · 2021-12-13T15:06:00Z

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

Updated, thanks!

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

src/link_prober/IcmpPayload.h

src/link_prober/LinkProber.cpp

lolyu · 2021-12-17T04:44:29Z

/azp run

azure-pipelines · 2021-12-17T04:44:37Z

Azure Pipelines successfully started running 1 pipeline(s).

src/link_prober/IcmpPayload.cpp

src/link_prober/LinkProber.cpp

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

zjswhhh · 2021-12-22T01:12:17Z

src/link_prober/LinkProber.cpp

-    icmpPayload->command = htonl(static_cast<uint32_t> (Command::COMMAND_NONE));
-    computeChecksum(icmpHeader, sizeof(icmphdr) + sizeof(*icmpPayload));
+    resetTxBufferTlv();
+    appendTlvSentinel();


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?

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

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

src/link_prober/LinkProber.cpp

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

This reverts commit 12b9951.

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

lolyu changed the title ~~Add tlv~~ [draft] Add tlv Dec 9, 2021

lolyu force-pushed the add_tlv branch 4 times, most recently from 5479758 to da05b00 Compare December 13, 2021 13:41

Add TLV support to ICMP payload

189ff90

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

lolyu force-pushed the add_tlv branch from da05b00 to 189ff90 Compare December 13, 2021 14:48

lolyu marked this pull request as ready for review December 13, 2021 14:55

lolyu changed the title ~~[draft] Add tlv~~ Add TLV to the ICMP payload Dec 13, 2021

lolyu requested review from lguohan, yxieca and zjswhhh December 13, 2021 15:25

lolyu force-pushed the add_tlv branch 2 times, most recently from f2fd3b4 to c062704 Compare December 16, 2021 01:57

Add sentinel TLV

b7ef68b

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

lolyu force-pushed the add_tlv branch from c062704 to b7ef68b Compare December 16, 2021 02:03