lib,zebra: Append labels on nexthop resolve

[sworleys]

When we resolve a nexthop appends its labels to the resolved nexthop, whether it already has labels or not. Before, we were iterating up the resolution tree to get all labels right before install.

K>* 7.7.7.7/32 [0/0] is directly connected, dummy1, label 11111, 00:06:29
D>  8.8.8.8/32 [150/0] via 7.7.7.7 (recursive), 00:06:20
  *                      via 7.7.7.7, dummy1 onlink, label 11111, 00:06:20
S>  9.9.9.9/32 [1/0] via 7.7.7.7 (recursive), label 2222, 00:03:49
  *                    via 7.7.7.7, dummy1 onlink, label 11111/2222, 00:03:49
S>  10.10.10.10/32 [1/0] via 7.7.7.7 (recursive), label 3333, 00:00:05
  *                        via 7.7.7.7, dummy1 onlink, label 11111/3333, 00:00:05
C>* 192.168.122.0/24 is directly connected, ens3, 00:06:29
K>* 192.168.122.1/32 [0/100] is directly connected, ens3, 00:06:29
ubuntu_nh# 

[LabN-CI]

🚧 Basic BGPD CI results: Partial FAILURE, 1 tests failed

Results table

_	_
Result	SUCCESS git merge/4808 c802e65
Date	08/08/2019
Start	20:30:25
Finish	20:52:11
Run-Time	21:46
Total	1815
Pass	1814
Fail	1
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2019-08-08-20:30:25.txt
Log	autoscript-2019-08-08-20:31:18.log.bz2
Memory	428 434 360

For details, please contact louberger

[sworleys]

ci:rerun

[sworleys]

The CI system hit a 504 when it tried to comment back on github.

[NetDEF-CI]

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-8569/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Checkout code: Successful with additional warnings

Report for nexthop.h | 6 issues
===============================================
< WARNING: function definition argument 'uint8_t' should also have an identifier name
< #122: FILE: /tmp/f1-19843/nexthop.h:122:
< WARNING: function definition argument 'mpls_label_t *' should also have an identifier name
< #122: FILE: /tmp/f1-19843/nexthop.h:122:
< WARNING: function definition argument 'struct nexthop *' should also have an identifier name
< #124: FILE: /tmp/f1-19843/nexthop.h:124:

---

CLANG Static Analyzer Summary

• Github Pull Request 4808, comparing to Git base SHA 3af7a9e

No Changes in Static Analysis warnings compared to base

1 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-8569/artifact/shared/static_analysis/index.html

[mjstapp]

Maybe this is a dumb question, but ... could you say a bit about just why this needs to be done? Is this related to the nexthop-group work-in-progress, for example? If so, why wouldn't a shared nexthop-group still have the list of resolving nexthops? There's a change here to nexthop_set_resolved() where we append labels to the recursive nexthop - right after making a copy of the resolving nexthop and copying the labels to it. Why do we need to change what's being done there - why do we need that second copy of the labels?

[sworleys]

Maybe this is a dumb question, but ... could you say a bit about just why this needs to be done? Is this related to the nexthop-group work-in-progress, for example? If so, why wouldn't a shared nexthop-group still have the list of resolving nexthops? There's a change here to nexthop_set_resolved() where we append labels to the recursive nexthop - right after making a copy of the resolving nexthop and copying the labels to it. Why do we need to change what's being done there - why do we need that second copy of the labels?

Indeed it is related to the nexthop group work but I also think it makes sense separately which is why I opened this PR.

So the resolved nexthop is what is actually going to be installed in the kernel with the nexthop group work.

To me it makes much more sense to have the labels on it in memory exactly how it appears in the kernel in memory rather than constructing it at the time we send it to the kernel.

Before what we would do is iterate up to the rparent and construct the label stack at install time. What I have done is essentially reversed that so that the parents labels are copied to the resolved nexthop (what is actually being installed) during its resolution rather than going up the tree at install.

The first label copying being done in nexthop_set_resolved() is for labels present in the nexthop we resolved to. We must have those. The second append is to also get the parents labels which I talked about above.

I hope I explained that somewhat decently.

[sworleys]

@rwestphal This is the patch we discussed.

[mjstapp]

I guess I'm still missing something - maybe this comment is throwing me off - why can't we iterate as we ... always have done? We have the recursive nexthop that is being resolved, and we have the resolving nexthops - what is it that we can't iterate over?

[sworleys]

So this patch actually creates an issue where if you resolved to a nexthop that was already resolved to another nexthop and all 3 have labels you end up getting 3 labels rather than 2.

S>  2.2.2.2/32 [1/0] via 7.7.7.7 (recursive), label 2222, 00:00:04
  *                    via 7.7.7.7, dummy1 onlink, label 1111/2222, 00:00:04
S>  3.3.3.3/32 [1/0] via 2.2.2.2 (recursive), label 3333, 00:00:02
  *                    via 7.7.7.7, dummy1 onlink, label 1111/2222/3333, 00:00:02
K>* 7.7.7.7/32 [0/0] is directly connected, dummy1, label 1111, 00:00:11
C>* 192.168.122.0/24 is directly connected, ens3, 00:00:11
K>* 192.168.122.1/32 [0/100] is directly connected, ens3, 00:00:11
ubuntu_nh(config)# 

Basically they keeping stacking every resolve since we are putting it on the resolved nexthop. Closing this patch for now until I figure this out.