Igalia / snabb

Snabb Switch: Fast open source packet processing
Apache License 2.0
48 stars 5 forks source link

Migrate lwAFTR to Yang configuration #510

Closed wingo closed 7 years ago

wingo commented 7 years ago

With the yang branch we will be migrating the lwaftr to YANG configuration, using the schema that currently lives at src/apps/lwaftr/snabb-softwire.yang. Here is a random quick braindump of things to do.

plajjan commented 7 years ago

I have some feedback on the YANG model https://github.com/Igalia/snabb/blob/yang/src/lib/yang/snabb-softwire-v1.yang which I understand will be used.

A range of 0..0 is not allowed - https://github.com/Igalia/snabb/blob/yang/src/lib/yang/snabb-softwire-v1.yang#L358 - pyang will complain over this (try to run everything through pyang --lint (but you can ignore some of the IETF rules, like references)). Above all, ncsc, the YANG compiler in NCS, complains over this so we can't actually compile the module at all and thus not load it in NCS. I can hand-patch it of course, but would prefer to have that upstreamed :) Perhaps you could use a must-statement instead to limit it to 0, or simply omit the leaf from the model altogether - YANG backwards compatibility rules do allow for additions to the model at a later point in time.

The leaf-list br-address doesn't specify ordering, which means it will default to be in system order (https://tools.ietf.org/html/rfc6020#section-7.7.5) as opposed to being in an order defined by the user. This wouldn't be an issue if it wasn't for https://github.com/Igalia/snabb/blob/yang/src/lib/yang/snabb-softwire-v1.yang#L364 that makes references into the br-address leaf-list. As a user we can't know the order in the leaf-list unless it is ordered-by user so at a minimum we should add the ordered-by user statement. I would however prefer if this referencing could happen inside the AFTR so that we only need to put the br address in the softwire, which I suppose would make the br-address leaf-list completely redundant. This would make provisioning slightly easier. Thoughts?

There are a number of places where leaves that are used as keys in a list are marked as mandatory. This is not necessary so you could remove it. Mostly cosmetic thing.

I think the MTU leaves deserve a slightly elaborated description. The way I interpret them (without looking in the code) is that we are dealing with the Ethernet payload. This is not obvious - for example, Cisco IOS uses the same, but JUNOS or IOS XR defines MTU including the Ethernet overhead so 1500b would become 1514 or 1518 if you have a VLAN tag. Just expand what it's actually specifies :)

If you wanted to you could formalise the requirement that the psid-length, shift and reserved-ports-bit-count must add up to 16 through a must statement... not super important though.

wingo commented 7 years ago

Thanks for the feedback @plajjan, it's very much appreciated :) It will take a little bit of time to fix these things but we will address them. Thanks for the detailed notes.

calvincheng8 commented 7 years ago

Regarding the problem with "range 0..0;", you could simply replace it with "range 0;"