[BGPCEP-740] BGP Best route selection not as expected when MED is present in the route Created: 10/Jan/18 Updated: 18/Apr/18 Resolved: 05/Apr/18 |
|
| Status: | Verified |
| Project: | bgpcep |
| Component/s: | BGP |
| Affects Version/s: | Fluorine, Nitrogen, Carbon, Oxygen |
| Fix Version/s: | Fluorine, Nitrogen, Carbon, Oxygen |
| Type: | Bug | Priority: | Medium |
| Reporter: | Luis Gomez | Assignee: | Ajay Lele |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
The BGP Best path selection in ODL is as follows: 1. prefer path with accessible next-hop So when ODL learns same prefix from 2 different iBGP routers and all path attributes above are identical, the routes tie-break at step 10 above, meaning that preferred route is the one coming from lower router ID. This actually works as long as MED is not present in the routes. If same, for example MED=0 (default) is present in the routes, then preferred route is the one coming from higher router ID. This seems like a bug because both scenarios no MED or having same MED should resolve in the route with lower router ID. See ODL printouts from BGP deployment: 1) Prefix learned from 10.100.16.161 in adj-rib-in:
{
"route-key": "WAAA/egAAP3qZBUg",
"label-stack": [
{
"label-value": 17
}
],
"route-distinguisher": "0:65000:65002",
"prefix": "100.21.32.0/20",
"attributes": {
"as-path": {
"segments": [
{
"as-sequence": [
65002
]
}
]
},
"extended-communities": [
{
"route-target-extended-community": {
"global-administrator": 65000,
"local-administrator": "AAD96g=="
},
"transitive": true
},
{
"route-origin-ipv4": {
"global-administrator": "169.254.2.2",
"local-administrator": 1
},
"transitive": true
}
],
"multi-exit-disc": {
"med": 0
},
"ipv4-next-hop": {
"global": "10.100.16.161"
},
"local-pref": {
"pref": 100
},
"origin": {
"value": "igp"
}
}
}
2) Same prefix learned from 10.100.16.162 in adj-rib-in:
{
"route-key": "WAAA/egAAP3qZBUg",
"label-stack": [
{
"label-value": 16
}
],
"route-distinguisher": "0:65000:65002",
"prefix": "100.21.32.0/20",
"attributes": {
"as-path": {
"segments": [
{
"as-sequence": [
65002
]
}
]
},
"extended-communities": [
{
"route-target-extended-community": {
"global-administrator": 65000,
"local-administrator": "AAD96g=="
},
"transitive": true
},
{
"route-origin-ipv4": {
"global-administrator": "169.254.2.2",
"local-administrator": 1
},
"transitive": true
}
],
"multi-exit-disc": {
"med": 0
},
"ipv4-next-hop": {
"global": "10.100.16.162"
},
"local-pref": {
"pref": 100
},
"origin": {
"value": "igp"
}
}
}
3) Prefix shown in loc-rib is the one with higher router ID (10.100.16.162):
{
"route-key": "WAAA/egAAP3qZBUg",
"label-stack": [
{
"label-value": 16
}
],
"route-distinguisher": "0:65000:65002",
"prefix": "100.21.32.0/20",
"attributes": {
"as-path": {
"segments": [
{
"as-sequence": [
65002
]
}
]
},
"extended-communities": [
{
"route-target-extended-community": {
"global-administrator": 65000,
"local-administrator": "AAD96g=="
},
"transitive": true
},
{
"route-origin-ipv4": {
"global-administrator": "169.254.2.2",
"local-administrator": 1
},
"transitive": true
}
],
"multi-exit-disc": {
"med": 0
},
"ipv4-next-hop": {
"global": "10.100.16.162"
},
"local-pref": {
"pref": 100
},
"origin": {
"value": "igp"
}
}
}
In the above example if MED is removed from the routes, then the route coming from lower router ID (10.100.16.161) is installed in loc-rib. Also note this behavior is observed in Carbon but it may happen in all branches as I believe there is no change in the best selection algorithm for a while. |
| Comments |
| Comment by Ajay Lele [ 02/Apr/18 ] |
| Comment by Claudio David Gasparini [ 04/Apr/18 ] |
|
Hi, I'll say that for equal paths and present MED, latest path gets selected as new best path, independently of the router-id,and as you mentioned it shouldn't.
ajayslele I added a test coverage for your patch, and I did a small change. If you are ok with them, and you could test it, add +1, and I'll merge it. Aside from that, for equal path advertizement, the first advertized will be keep as best path independently of the router ID. ecelgp not sure if you expect that Router Id should be taken in consideration.
Regards, |
| Comment by Luis Gomez [ 04/Apr/18 ] |
|
cdgasparini, I am not sure I full follow your comment but my expectation is for ODL to implement the tie-break rules at the top of the ticket, meaning that in case of 1-9 match, 10 can be used to select best path. |
| Comment by Ajay Lele [ 04/Apr/18 ] |
|
cdgasparini I agree with what ecelgp said that router ID should be used as tie-breaker at the end if everything else is same. And that's how the existing code behaves if I understand correctly. My patch causes the decision making using MED comparison to happen only when the values are not equal, so that premature decision is not made when the MED values are equal. |
| Comment by Claudio David Gasparini [ 05/Apr/18 ] |
|
Hi all, my fault, I wasn't sure if the Router Id was taken in account or if the path from the first peer(independently if the router ID) on advertize the path was taken as the best path. I just wanted to ensure that the patch was tested and it was doing what it was expected. ajayslele I understand from your comments that you tested it, so I merged the patch. Sorry if I brought some confusion. Regards |
| Comment by Ajay Lele [ 05/Apr/18 ] |
|
cdgasparini Yes I had tested my version of the patch (patch-set 1). The change you have proposed (patch-set 2) works differently and I don't think that is how we want the behavior to be, when MED values are equal.
Logic in patch-set 1: If MED values are equal, do not take decision (i.e. don't return) from the isExistingPathBetter() method, and let rest of the conditions decide which is the better path.
Logic in patch-set 2: If MED values are equal, specify that existing path is better (i.e. return true) from method isExistingPathBetter(). So if existing path has higher router ID, it is retained, which is not correct. |
| Comment by Claudio David Gasparini [ 05/Apr/18 ] |
|
checking it from gerrit, I think both logics do the same, I'll double check it tomorrow. But Logic in patch-set 1: If MED values are equal, do not take decision, is the last check then it goes to the end of the method where true is returned. |
| Comment by Claudio David Gasparini [ 06/Apr/18 ] |
|
Based on your comments on patch "Since the rest of the steps* are essentially no-op right now, you are right, both approaches produce same result. But this* can change in future, so I prefer returning from right place for the right reason." Based on code annotation // 6. prefer the path with the lowest multi-exit discriminator (MED) If MED of new path is lower than the present one, then new path is better. Therefore Logic 2 is in "right place for the right reason.", MED. If you want to ensure that any future code change would not affect result on such scenario, the best way is to implement a test covering it. Please provide such covering test, and I'll merge it. Regards,
|
| Comment by Ajay Lele [ 06/Apr/18 ] |
|
cdgasparini - comment inline [AJAY] — Based on your comments on patch "Since the rest of the steps* are essentially no-op right now, you are right, both approaches produce same result. But this* can change in future, so I prefer returning from right place for the right reason." Based on code annotation // 6. prefer the path with the lowest multi-exit discriminator (MED) If MED of new path is lower than the present one, then new path is better. Therefore Logic 2 is in "right place for the right reason.", MED. [AJAY] But now the check is >= i.e. it will hit if both MED values are same, not just when one value is lower than the other |
| Comment by Claudio David Gasparini [ 06/Apr/18 ] |
|
Header of the method * @return true if the existing path is better, false if the new path is better if MED are = then new path is not better, therefore we keep the actual best path. If you still don't like it, push the change again with the covering test and I ll merge it. Both keep doing the same, but we different approach. Regards, |