Add missing neighbor resolution for MPLS route programming #1968
Conversation
|
This pull request fixes 1 alert when merging d6141c4 into 6b15584 - view on LGTM.com fixed alerts:
|
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| self.check_nexthop(False, "SAI_NEXT_HOP_TYPE_MPLS", "10.0.0.1", "SAI_OUTSEG_TYPE_SWAP", "1:101") | ||
| self.check_nexthop(False, "SAI_NEXT_HOP_TYPE_MPLS", "10.0.0.1", "SAI_OUTSEG_TYPE_SWAP", "1:201") | ||
| self.check_nexthop(False, "SAI_NEXT_HOP_TYPE_MPLS", "10.0.0.3", "SAI_OUTSEG_TYPE_SWAP", "1:202") | ||
|
|
There was a problem hiding this comment.
the test should indicate pass or fail. I don't see that here.
There was a problem hiding this comment.
I'm not sure what you mean here. This finishes the same way as every other test, and I can't see any return codes in other test suites. What should I be adding here?
There was a problem hiding this comment.
You should add 'asserts' for whatever condition that is expected for the test to claim pass.
e.g. https://github.com/Azure/sonic-swss/blob/master/tests/test_port.py
I missed to notice this in last review. Without asserts there's no way to know if test passed or failed. Please update in all the test cases.
There was a problem hiding this comment.
The assert is within the wait_for_result function called by each of the check_ functions (e.g. check_nexthop). This is standard across many of the test suites where it's waiting for programming to be done by orchagent - and I've seen many times that it does produce an assert if the programming doesn't reach the expected state in time.
| if self.mpls_appdb_mode(): | ||
| fieldValues = {"nexthop": "10.0.0.1", "ifname": "Ethernet0", "mpls_nh": "swap101", "mpls_pop": "1"} | ||
| self.create_inseg_entry(label, fieldValues) | ||
| fieldValues = {"nexthop": "10.0.0.1,10.0.0.3", "ifname": "Ethernet0,Ethernet4", "mpls_nh": "swap201,swap202", "mpls_pop": "1"} |
There was a problem hiding this comment.
which neighbor is unresolved here? I want to know how the new code in mplsrouteorch.cpp:529 getting exercised.
There was a problem hiding this comment.
I've extended the comment above this
|
This pull request fixes 1 alert when merging c819ef8 into d95823d - view on LGTM.com fixed alerts:
|
|
I've removed the test before the one I've added, as it's no longer valid (it was explicitly waiting to do a ping for the route to be programmed, whereas it now resolves itself) |
|
This pull request fixes 1 alert when merging 0922d83 into d95823d - view on LGTM.com fixed alerts:
|
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@smaheshm can this be merged? |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This reverts commit 0922d83.
|
This pull request fixes 1 alert when merging 733c84d into ac09bde - view on LGTM.com fixed alerts:
|
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This reverts commit 0922d83.
|
This pull request fixes 1 alert when merging bd9d0ae into ac09bde - view on LGTM.com fixed alerts:
|
What I did Added FDB debug dump module How I did it Used debug dump infra to add the FDB debug dump utility How to verify it Added Unit test cases to verify it
What I did
Added neighbor resolution when MPLS route's next hop not resolved, matching same code for IP route
Why I did it
Without this an MPLS route may end up not being programmed
How I verified it
Python UT
Details if related