sonic-swss
sonic-swss copied to clipboard
sonic-net
[bfdorch] bfdorch needs to query SAI_BFD_SESSION_ATTR_PORT before programming it when port is not default
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 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.
@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.
@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?
@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
@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