OSPF support for OpenBSD devices#2697
Conversation
Topology-level modules are copied into router nodes that do not have a module attribute. That's not done for host nodes because it's assumed hosts usually just do IP routing. We would have to set role: router on dut device in all OSPF integration tests, but that doesn't make much sense. Once a node supports a routing protocol, it's a router ;) Setting role on OpenBSD device is a much better solution. |
ipspace
left a comment
ipspace
left a comment
There was a problem hiding this comment.
Thanks a million. Great job!
I made a number of suggestions (plus the "|default(x)" thing which I think we need). I could fix them if you're short on time.
A bunch of OSPFv3 integration tests were failing, but it seems to be a timing thing. For example, I retried the 01-network test and the failing "is prefix there" test succeeded in 11 seconds (just above the 10-second deadline).
https://tests.netlab.tools/openbsd/libvirt/ospf/ospfv3/01-network.yml-validate.log
You probably need to tweak a few SPF/LSA origination timers.
| spf-holdtime msec 200 | ||
| {% if ospf_data.default is defined %} | ||
| {% set dfd = ospf_data.default %} | ||
| redistribute default set {{ '{' }} metric {{ dfd.cost or '100' }} type {{ dfd.type.replace('e','') or '1' }} {{ '}' }} |
There was a problem hiding this comment.
You can use { or } as simple text, no need to make an expression out of it. Jinja2 reacts just to {{ / }} and {% / %}
There was a problem hiding this comment.
Also, see the comment on using |default(x) in OSPFv3 template
There was a problem hiding this comment.
makes sense.
I can adapt it towards the end of the week. Feel free to adapt if you have the time earlier.
| {% for a in areas %} | ||
| area {{ a }} {{ '{' }} | ||
| {% for l in intf_data if l.ospf.area is defined and l.ospf.area is eq a and 'ipv4' in l %} | ||
| {% if l.ifname == "lo0" %} |
There was a problem hiding this comment.
Is this specific to "lo0" or all loopback interfaces? If it applies to all loopback interfaces, I'd go with if l.type == "loopback"
There was a problem hiding this comment.
This is specific to lo0. Otherwise 127.0.0.1 would be advertised ...
| {% else %} | ||
| interface {{ l.ifname }} {{ '{ '}} | ||
| {% endif %} | ||
| {% if l.ospf.network_type is defined and l.ospf.network_type == 'point-to-point' %} |
There was a problem hiding this comment.
Does OpenBSD support just "network" and "point-to-point"? We also have "point-to-multipoint" (but it's rarely implemented and thus not tested anywhere).
If OpenBSD supports "point-to-multipoint" then you might want to use a keyword map similar to what Junos does
You can also define the keyword map in device group_vars if you hate having hard-coded stuff in templates.
There was a problem hiding this comment.
nbma is not in the OpenBSD OSPF implementation.
| spf-holdtime 1 | ||
| {% if ospf_data.default is defined %} | ||
| {% set dfd = ospf_data.default %} | ||
| redistribute default set {{ '{' }} metric {{ dfd.cost or '100' }} type {{ dfd.type.replace('e','') or '1' }} {{ '}' }} |
There was a problem hiding this comment.
Default cost could be undefined. It would be better to use "dfd.cost|default(100)". Same for dfd.type
There was a problem hiding this comment.
makes sense. thanks for catching this!
ipspace
left a comment
ipspace
left a comment
There was a problem hiding this comment.
Implemented all suggested changes, added device quirks, documentation, and changes to integration tests
2 participants
This adds OSPFv2 and OSPFv3 support for OpenBSD.
I had to change the role from 'host' to 'router'. Overwriting it in the topology did not work (ospf templates were not evalutated).