search

sonic-net / sonic-swss

SONiC Switch State Service (SwSS)
https://azure.github.io/SONiC
Other
170 stars 514 forks source link

[bfdorch] bfdorch needs to query SAI_BFD_SESSION_ATTR_PORT before programming it when port is not default #3051

Open dgsudharsan opened 7 months ago

dgsudharsan commented 7 months ago

BFD configuration can specify ifname when creating the session or it can be set to default. When non default ifname is set, the attribute SAI_BFD_SESSION_ATTR_PORT will be programmed. However SAI library may not support this attribute. Hence bfdorch should query support for this attribute and enable programming of non default interface only when this is supported

dgsudharsan commented 7 months ago

@prsunny FYI

prsunny commented 7 months ago

@kperumalbfn

kperumalbfn commented 6 months ago

@dgsudharsan Currently bfdorch sets SAI_BFD_SESSION_ATTR_HW_LOOKUP_VALID as False. In that case, SAI_BFD_SESSION_ATTR_PORT is mandatory. How does ASIC determines determine the destination port if the SAI_BFD_SESSION_ATTR_PORT is not passed to SAI? Please clarify.

dgsudharsan commented 6 months ago

@kperumalbfn This flow is true when interface is non default https://github.com/sonic-net/sonic-swss/blob/97c7f3edbfedddceaa3bbac04f9d56e9bcad558c/orchagent/bfdorch.cpp#L437C9-L437C27

If a vendor SAI doesn't support SAI_BFD_SESSION_ATTR_PORT attribute, orchagent should return error without further calling SAI with create bfd session. Currently there is no such check and the vendor SAI call will be invoked which will result in orchagent crash.

kperumalbfn commented 6 months ago

@dgsudharsan as per the BFD SAI spec, BFD session port/src_mac/dst_mac fields are valid only when hw_lookup_valid is false. Current bfdorch.cpp sets 'hw_lookup_valid = false' for all BFD sessions.

https://github.com/opencomputeproject/SAI/blob/bc1d6ec90fb462c5171dbaa1aedcab1aac890ce9/inc/saibfd.h#L179

If vendor SAI uses h/w lookup to derive the egress port and BFD packet's L2 rewrite, then we need to update the bfdorch.cpp as well based on the capability check. Could you confirm the behavior?

dgsudharsan commented 5 months ago

@kperumalbfn The logic in bfdorch is to set SAI_BFD_SESSION_ATTR_PORT when alias is not "default". It doesn't check if the attribute is supported. https://github.com/sonic-net/sonic-swss/blob/774973d95de07209e8280d4c090620293dc7ff08/orchagent/bfdorch.cpp#L437 Since this can be SAI vendor implementation specific and not mandatory attribute as well, if SAI has not implemented orchagent will crash. So if the user sets alias as not default, before setting SAI_BFD_SESSION_ATTR_PORT it is better to query if the attribute is supported and if not, log error message in orchagent and avoid programming SAI

dgsudharsan commented 5 months ago

@kperumalbfn The logic in bfdorch is to set SAI_BFD_SESSION_ATTR_PORT when alias is not "default". It doesn't check if the attribute is supported. https://github.com/sonic-net/sonic-swss/blob/774973d95de07209e8280d4c090620293dc7ff08/orchagent/bfdorch.cpp#L437 Since this can be SAI vendor implementation specific and not mandatory attribute as well, if SAI has not implemented orchagent will crash. So if the user sets alias as not default, before setting SAI_BFD_SESSION_ATTR_PORT it is better to query if the attribute is supported and if not, log error message in orchagent and avoid programming SAI