search

sonic-net / DASH

Disaggregated APIs for SONiC Hosts
Apache License 2.0
77 stars 88 forks source link

Skip default IPv6 route with prefix zero and fix IPv6 prefix bug #565

Closed Pterosaur closed 2 weeks ago

Pterosaur commented 1 month ago

1.To set a default IPv6 route with the prefix 0, we will get following error logs. It's caused by the limitation of p4lang: https://github.com/p4lang/PI/blob/24e0a3c08c964e36d235973556b90e0ae922b894/proto/frontend/src/device_mgr.cpp#L2242-L2246 .

2024 Jun  3 15:26:21.033130 vlab-01 ERR syncd#syncd_dash: e:- mutateTableEntry: GRPC ERROR[2]: , #010#002#032¡#001#012#037type.googleapis.com/p4.v1.Error#022~#010#003#022gInvalid reprsentation of 'don't care' LPM match, omit match field instead of using a prefix length of 0#032#021ALL-sswitch-p4org
2024 Jun  3 15:26:21.033130 vlab-01 ERR syncd#syncd_dash: e:- mutateTableEntry: GRPC call Write::INSERT ERROR: table_id: 49279256 match { field_id: 1 lpm { value: "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000" } } action { action { action_id: 32404057 params { param_id: 1 value: "\000\000" } params { param_id: 2 value: "\000\000" } } }

In this case, I just ignore the default IPv6 route to skip this limitation.

  1. Fix IPv6 prefix length function bug. There are two issues on the IPv6 prefix:
    • when we calculate the prefix length from the mask, we should consider the trailing zeros from high address space, aka, the end of mask array, of SAI IPv6 attribution. So, I revised this issue by reversing the IPv6 mask array.
    • Because the function leadingNonZeroBits returns the exact length of prefix, we shouldn't directly subtract it from 129. For example, if the mask is 0xffffffffffffffffffffffff, the original implementation will get 129-0-32=97, it is obviously wrong. The correct answer is 128. So, I fix this bug.
r12f commented 1 month ago

Hi @Pterosaur , do you mind to add some description here to describe the issue and how we are fixing it?

Pterosaur commented 1 month ago

Hi @Pterosaur , do you mind to add some description here to describe the issue and how we are fixing it?

Hi @r12f , Thanks for your suggestion. This PR is just for testing right now, So it looks hacky and drafted. I will refactor it after confirmed.

r12f commented 1 month ago

hi Ze, do you mind to add some explanation on what bug we are fixing in this change and how?

Pterosaur commented 1 month ago

hi Ze, do you mind to add some explanation on what bug we are fixing in this change and how?

@r12f Thanks for your suggestion, Please check it again.

KrisNey-MSFT commented 3 weeks ago

@r12f - looks like a request for you to re-check after edits :)

KrisNey-MSFT commented 1 week ago

This is to ensure bmv2 can launch, but is a temporary patch. Not valid in LPM, and it fails.
@r12f will designate someone to file an Issue in P4 Runtime GitHub (leading 0's) if we feel it is related here. We need a volunteer to fix this if we can figure it out.

https://github.com/p4lang/p4runtime