Subject: [Softwires] Review of draft-ietf-softwire-yang-03

From: Linhui Sun <[email-masked]>
Date: Fri, 12 Jan 2018 09:12:50 +0800

Hi,

I have had a review of the new version of this yang model. The model looks
much better than the previous version and many thanks for the authors'
effort. And from my perspective, I think this model could be further
discussed in the following aspects. Please find my comments in the
following.

1) The common module contains some definition groupings that are only
applicable to the BR module (e.g. binding-entry and port-set), they should
be excluded from the common module.

2) The BR module should also augments ietf-interfaces since they are also a
tunnel endpoint and it is better to be consistent with the CE model. And in
this way, we do not need to declare the redundant br-instance id or name
anymore.

3) There are still some descriptions in the YANG code following the old
version, these should be updated accordingly.

4) The usage of choice statement is not very clear, why do we need to use
the 'case' and 'feature' statements together? It seems that we only need
one of them.

5) It seems that the state parameters of CE module have duplicated node
'name' and 'type' since they have already been defined in the
ietf-interfaces module.

BR,
Linhui

---

From: Ian Farrer <[email-masked]>
Date: Fri, 12 Jan 2018 10:17:37 +0100

Hi Linhui,

I agree with your comments on 1,3 and 5. Comments on 2&4 below.

Thanks,
Ian

> 
> 2) The BR module should also augments ietf-interfaces since they are also a tunnel endpoint and it is better to be consistent with the CE model. And in this way, we do not need to declare the redundant br-instance id or name anymore.

[if - From my personal perspective, I disagree. In both of the lw4o6 implementations I’ve worked with, the v6 address used as the BR’s tunnel endpoint address is not configured on any interface - i.e. it is used purely as a destination address for routing traffic to, validating received encapsulated traffic’s source address and as the source address for sent encapsulation.

If an implementation did rely on having an address assigned to an interface, then this could still be done using ietf-interfaces/ietf-ip in the normal way and the softwire-br model would be configured with the appropriate matching address.

> 
> 4) The usage of choice statement is not very clear, why do we need to use the 'case' and 'feature' statements together? It seems that we only need one of them.

[if - Well, a CE may implement binding and/or algorithm. If it implements one, then there is no choice to be made. If it implements both, then it can only configure one type hence the choice. If it needed to implement two types for whatever reason, then there would be two instances of the model attached to different interfaces.

---

From: Linhui Sun <[email-masked]>
Date: Fri, 12 Jan 2018 19:59:32 +0800

Hi Ian,

Thanks for the response, please see my comments inline.

BR,
Linhui

2018-01-12 17:17 GMT+08:00 Ian Farrer <[email-masked]>:

> > 2) The BR module should also augments ietf-interfaces since they are
> also a tunnel endpoint and it is better to be consistent with the CE model.
> And in this way, we do not need to declare the redundant br-instance id or
> name anymore.
>
> [if - From my personal perspective, I disagree. In both of the lw4o6
> implementations I’ve worked with, the v6 address used as the BR’s tunnel
> endpoint address is not configured on any interface - i.e. it is used
> purely as a destination address for routing traffic to, validating received
> encapsulated traffic’s source address and as the source address for sent
> encapsulation.
>
> If an implementation did rely on having an address assigned to an
> interface, then this could still be done using ietf-interfaces/ietf-ip in
> the normal way and the softwire-br model would be configured with the
> appropriate matching address.
>
[LH]: Well, if there is no such requirements from other vendors, just
ignore this comment. And a further question here, is the concept of
'instance' in BR module equivalent to the term of interface?

>
>
> >
> > 4) The usage of choice statement is not very clear, why do we need to
> use the 'case' and 'feature' statements together? It seems that we only
> need one of them.
>
> [if - Well, a CE may implement binding and/or algorithm. If it implements
> one, then there is no choice to be made. If it implements both, then it can
> only configure one type hence the choice. If it needed to implement two
> types for whatever reason, then there would be two instances of the model
> attached to different interfaces.
>
[LH]: That sounds strange, why not just restricting the CE/BR instance (not
the actual device) to be only one of them (i.e. binding & algorithm)? In
this way, only 'if-feature' is needed.

---

From: Ian Farrer <[email-masked]>
Date: Fri, 12 Jan 2018 13:04:49 +0100

Hi Linhui,

> [LH]: That sounds strange, why not just restricting the CE/BR instance (not the actual device) to be only one of them (i.e. binding & algorithm)? In this way, only 'if-feature' is needed. 

Can you provide a (high level) example of how you would structure this?

Thanks,
Ian

---

From: Linhui Sun <[email-masked]>
Date: Fri, 12 Jan 2018 20:14:08 +0800

2018-01-12 20:04 GMT+08:00 Ian Farrer <[email-masked]>:

> [LH]: That sounds strange, why not just restricting the CE/BR instance
> (not the actual device) to be only one of them (i.e. binding & algorithm)?
> In this way, only 'if-feature' is needed.
>
>
Hi Ian,

This is what defined in the current model:
choice br-type {
  case binding {
    if-feature binding;
    container binding {
      if-feature binding;
      ...
    }
  }
  case algorithm {...}
}

What if we just use the following:
container binding {
  if-feature binding;
  ...
}
container algorithm {
  if-feature algorithm;
  ...
}

---

From: Ian Farrer <[email-masked]>
Date: Fri, 12 Jan 2018 13:48:15 +0100

Hi Linhui,

Could you provide a proposed structure for your suggestion of using only ‘if-feature’?

Thanks,
Ian

> On 12. Jan 2018, at 13:14, Linhui Sun <[email-masked]> wrote:
> 
> 
> 2018-01-12 20:04 GMT+08:00 Ian Farrer <[email-masked] <mailto:[email-masked]>>:
> Hi Linhui,
>

---

From: Linhui Sun <[email-masked]>
Date: Fri, 12 Jan 2018 21:56:31 +0800

Hi Ian,

If you are talking about the tree model, I think it should just simply like
the following if only using the 'if-feature',
+--rw br-instances
     +--rw binding {binding}?
     |   +--rw br-instance* [id]
     |       +--rw ...
     |
     +--rw algorithm {algorithm}?
         +--rw algo-instance* [id]
             +--rw ...

BR,
Linhui

2018-01-12 20:48 GMT+08:00 Ian Farrer <[email-masked]>:

>>> [if - Well, a CE may implement binding and/or algorithm. If it
>>> implements one, then there is no choice to be made. If it implements both,
>>> then it can only configure one type hence the choice. If it needed to
>>> implement two types for whatever reason, then there would be two instances
>>> of the model attached to different interfaces.
>>>