Re: [Softwires] [yang-doctors] Softwire YANG Review

[Qi Sun <[e-mail]>]

Dear Giles,

Thank you very much for your review! These are valuable comments, we're trying to address all of them. Please see inline for responses. 

And thanks to Bert for your kindness!

Thanks,
Qi


在 2015-3-16，17:49，"Giles Heron (giheron)" <[e-mail]> 写道：

> comments as promised:
> 
> 1) CPE vs CE.  We need consistency in the doc I think.

[Qi] Good catch. I think we should use CE to follow terms used in MAP.

> 
> 2) MAP-E.   Does that still exist?  Or is it MAP and MAP-T now?

[Qi] The term "MAP" actually means "MAP using Encapsulation", i.e. MAP-E. This term is well received in Softwire so we used it in the document. 

> 
> 3) Section 1.   You've stated that each mechanism has its own module - but that's no longer the case, is it?   It's all in one module, but with different sections.  

[Qi] That statement was not accurate. It's one module with different features. 

> Having said that there's so little shared information (see below) that I'd be tempted to put each mechanism in its own module (but with a common module for shared groupings).

[Qi] Are you suggesting to have one module with multiple submodules? Or several top level modules?

The intent of using features is to enable one device to function in different mode. And it doesn't have to implement those parts that it doesn't support. But I've got your point for total independent parts. I think it's also valid. 

> 
> 4) Section 2 (grammar nit).  I'd say "the configuration and management information..." (not "configure and manage").

[Qi] Thanks for pointing out.

> 
> 5) Section 6.  you probably need to add security considerations soon.

[Qi] Sure. Already added in our local version.

> 
> 6) section 7.  I don't think there would be IANA considerations for a YANG model?  Or not unless IANA sets up a namespace registry.

[Qi] Yes, a namespace registry is necessary.

> 
> 7) section 8.  you've acknowledged Rajiv but then added him as an author.
> 
> 8) section 9.1.   MAP is now up to -13.
> 
> 9) section 9.1.  You have listed RFC6021/RFC6241 but there's no ref to either in the text.
> 
> 10) section 9.1.  I think RFC6991 could be informative?
> 
> 11) section 9.2.  You've also listed RFC6333/RFC6470 without actually referring to those from the text.

[Qi] Followed your suggestions. Thanks. 

> 
> 12) (general).  Is it worth thinking about OAM?  For example you could enable multi-hop BFD on the tunnels for all cases bar MAP-T (where there's no tunnel).   Likewise might be worth thinking about fragmentation. We covered both issues in the keyed IP tunnel YANG.

[Qi] It does need considerations. But keyed v6 tunnel is layer 2 tunnel (more related to Ethernet OAM). Softwire mechanisms are layer 3 tunnel/translation mechanisms. Not sure if same actions should be taken here. Please can you shed some light on it. Thanks!

And yes, we need to include fragmentation by following requirements defined in related documents. 

> 
> (remaining comments are on the Yang model in section 4 - so I guess doing the YANG-doctor job here):
> 
> 13) (softwire-config).  Do we need a configurable description for softwire? (assuming we keep the shared data node - see below).

[Qi] Added, for now.

> 
> 14) (softwire-config).  Should the tunnel-mtu be uint16 rather than uint32?  

[Qi] Ok.

> Also I'm not convinced tunnel-mtu is a per-domain parameter.  isn't it per tunnel?   The "v6" way is probably to use PMTU discovery.   We put a bunch of stuff on this in the keyed-ip-tunnel YANG draft (relates to fragmentation of course - see above).   Also tunnel-mtu doesn't apply to MAP-T (as there's no tunnel)

[Qi] I assume you're suggesting one rule per tunnel, and per mtu value (please correct me if I'm wrong). The difference here might be that, keyed-v6-tunnel is typically configured as one tunnel per session. That is, the mtu parameter should be configured per tunnel. So it’s correct to put it inside the container/list.
But for softwire mechanisms, supporting multiple binding entries / rules might not mean there is multiple tunnels. There could be only one tunnel instance serving multiple subscribers. it doesn't have to be one rule per tunnel or one binding-entry per tunnel (though implementation can work that way). 

> 
> 15) (softwire-config).  Is there any value in enabling/disabling softwire globally (rather than just per mechanism)?

[Qi] Removed.

> 
> 16) Given the above 3 comments I'm not sure we need any shared softwire config (or status).

[Qi] It can also be structured in that way, as long as a device doesn't get configure with parameters it doesn't recognize. The current shape can achieve that. 

> 
> 17) (map-e/map-t).  I'd suggest renaming the "map-rules" list to "map-rule" and the map-rule-table" container to "map-rules".
> 
> 18) (map-e/map-t).  You could possibly make the id, name and map-rules all a common grouping between map-e/map-t (so then the only difference is the br-ipv6-addr vs the dmr-ipv6-prefix).  At that point you could remove the existing map-rule grouping as it would only be used by the new grouping.

[Qi] Thanks for the suggestion. Will follow.

> 
> 19) (map-t).  Yes - I think we need the IPv6 prefix for the MAP-E/MAP-T domain in the BR config.  The BRs need to originate that prefix into IPv6 iBGP.

[Qi] Sorry, I didn't quite follow here. There is a parameter about the rule ipv6 prefix in the MAP rule grouping. 

And could that prefix be configured through other standardised YANG, i.e. interface YANG or IP YANG?

> 
> 20) (map-rule).  Would we ever have FMR rules at a BR?  (FMR rules are for direct CE to CE communication)

[Qi] Mark Townsley pointed out that MAP BR and MAP CE can function the same. That is, BR can work as a CE, or talk to a CE directly. So I think FMR should be there (or maybe I didn't understand correctly...)?

> 
> 21) (map-rule).  The drafts specify that BMR rules may also be FMR rules - so you may need a way to specify that a rule is both

[Qi] The formats of BMR and FMR are the same. But I wouldn't think the matter you mentioned is about configuration. It seems that it’s more about whether the implementation will take BMR as FMR. In some cases, saying when there is only one BMR, that BMR can also be a FMR. 
 

> 
> 22) (softwire-state).  I don't think you need enable leaves on the status containers?  

[Qi] Removed.


> In fact I'm not sure there's much in the state containers that you need to retain.  The map-rules for example are only needed if there's a non-configuration way of creating them. 

[Qi] Actually, I agree that having those parameters there is kind of redundant. But What if some bindings / rules are created or modified manually? 

> 
> 23) (softwire-lwaftr-event).  Do you want "default false" on the exceed-sw-um-limit?  I generally go for empty types rather than booleans - so you only include the field if it's true.

[Qi] I agree it would be better. I will check other parameters in similar situation.

Another question about the list "binding-entries". We intend to use ipv6 address information as the key, which might be either ipv6-address or ipv6-prefix. There were suggestions to make "ipv6info" a choice with address and prefix as cases. However, I think "ipv6info" is not a legal key since it's not a leaf. Could I use "union" as the type for "ipv6info" (containing ipv6 address type and ipv6 prefix type), so that it can be the "key"?

Thanks again for the thorough review and the suggestions! 

Cheers,
Qi

Sent from my iPhone, maybe typos.