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.

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

CRM incorrect processing for CRM_MPLS_NEXTHOP #2044

Closed
msosyak opened this issue Nov 23, 2021 · 4 comments · Fixed by #2057
Closed

CRM incorrect processing for CRM_MPLS_NEXTHOP #2044

msosyak opened this issue Nov 23, 2021 · 4 comments · Fixed by #2057
Assignees

Comments

@msosyak
Copy link
Contributor

msosyak commented Nov 23, 2021

In void CrmOrch::getResAvailableCounters() for case CrmResourceType::CRM_MPLS_NEXTHOP: we do
static_cast<sai_object_type_t>(crmResSaiAvailAttrMap.at(res.first)); but after this PR #2008 the value was changed from SAI_OBJECT_TYPE_NEXT_HOP to SAI_SWITCH_ATTR_AVAILABLE_IPV4_NEXTHOP_ENTRY so the logic in void CrmOrch::getResAvailableCounters() for case CrmResourceType::CRM_MPLS_NEXTHOP: become broken and start produce the followin errors in SONiC:

Nov 23 16:23:54.673681 sonic ERR swss#orchagent: :- objectTypeGetAvailability: attr SAI_L2MC_ENTRY_ATTR_PACKET_ACTION is not resource type
Nov 23 16:23:54.673681 sonic ERR swss#orchagent: :- getResAvailableCounters: Failed to get availability for object_type 53 , rv:-5

Link to related source code https://github.com/Azure/sonic-swss/blob/01c243a1cfa397065f9d8f5519168bc556cf6328/orchagent/crmorch.cpp#L589
https://github.com/Azure/sonic-swss/blob/01c243a1cfa397065f9d8f5519168bc556cf6328/orchagent/crmorch.cpp#L67

@msosyak
Copy link
Contributor Author

msosyak commented Nov 23, 2021

@smaheshm @qbdwlr @prsunny @kcudnik Please take a look

@kperumalbfn FYI

@msosyak msosyak changed the title CRM bad icorrect proceesing for CRM_MPLS_NEXTHOP CRM incorrect processing for CRM_MPLS_NEXTHOP Nov 23, 2021
@qbdwlr
Copy link
Contributor

qbdwlr commented Nov 23, 2021

@smahesh @prsunny @kcudnik @kperumalbfn The fix in #2008 is completely wrong and likely still triggers the same error that it was supposed to fix.
Look at the code for CrmOrch::getResAvailableCounters(): CRM_MPLS_NEXTHOP uses the SAI API sai_object_type_get_availability(); it does not call sai_switch_api->get_switch_attribute().
There are no SAI_SWITCH_ATTRs for MPLS Nexthops, because during the SAI Community review, I was asked to remove the SAI_SWITCH_ATTRs that I had defined and use sai_object_type_get_availability with SAI_OBJECT_TYPE instead.
Not sure what platform was being tested when the issue for #2008 was seen, but the correct solution is to revert the change in #2008 and add support for the SAI_OBJECT_TYPE in sai_object_type_get_availability(). For VS platform, see the support I provided in VirtualSwitchSaiInterface::objectTypeGetAvailability().
Reviewing other recent changes in CrmOrch::getResAvailableCounters(), it appears that CRM_SRV6_NEXTHOP will have similar issues as CRM_MPLS_NEXTHOP and should be addressed as I have recommended.
CRM_SRV6_MY_SID_ENTRY has no unique processing and should be grouped with CRM_MPLS_INSEG and CRM_NEXTHOP_GROUP_MAP; it does not require a separate case.
I see no support for either CRM_SRV6_MY_SID_ENTRY or CRM_SRV6_NEXTHOP in vslib. Support needs to be added in VirtualSwitchSaiInterface::objectTypeGetAvailability().

@prsunny
Copy link
Collaborator

prsunny commented Nov 23, 2021

@smaheshm , would you take a look. Looks like we may have to revert #2008

@smaheshm
Copy link
Contributor

smaheshm commented Dec 2, 2021

#2057

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants