Subject: Re: [L3sm] [Supa] draft-zaalouk-supa-vpn-service-management-model

From: Ying Cheng <[email-masked]>
Date: Wed, 18 Mar 2015 13:23:47 +0800

如果您错误接收了该邮件，请通过电子邮件立即通知我们。[email-masked]，即可以退订此邮件。我们将立即将您的信息从我们的发送目录中删除。
If you have received this email in error please notify us immediately by e-mail. Please reply to [email-masked] if you want to unsubscribe from this mail. We will immediately remove your information from our sending catalogue.

Subject: Re: Re: [Supa] [L3sm] draft-zaalouk-supa-vpn-service-management-model
From: Ying Cheng <[email-masked]>
To: "Liushucheng (Will)" <[email-masked]>, "[email-masked]" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>
Date: Wed, 18 Mar 2015 13:23:47 +0800

Hi all,

Many thanks for Jan's kind review and will's forwarding it. These comments are very useful for us. I will discuss with other co-authors and update the draft accordingly. Some quick response are below inline starts with”A:”.

BR,
Ying

2015-03-18 

Ying CHENG  程莹
China Unicom Network Technology Research Institute 中国联通网络技术研究院
Tel: +86-10-68799999-7702
Mobile: +86-18601102971

发件人： Liushucheng (Will) 
发送时间： 2015-03-17  20:53:05 
收件人： [email-masked] 
抄送： [email-masked]; [email-masked]; [email-masked] 
主题： Re: [Supa] [L3sm] draft-zaalouk-supa-vpn-service-management-model 
 
Hi authors of supa-vpn-service-mgmt-model,
 
Just subscribed to l3sm and read the mailing list, I found there is a kindly email from Jan Lindblad (copied) who gave a set of detailed comments. I guess maybe you didn’t follow l3sm list so I copy them here below.
 
Regards,
Will
 
Re: [L3sm] draft-zaalouk-supa-vpn-service-management-model

From: Jan Lindblad <janl at tail-f.com>
To: Benoit Claise <bclaise at cisco.com>
Cc: l3sm at ietf.org
Date: Wed, 11 Mar 2015 14:23:31 +0100
In-reply-to: <[email-masked]>
References: <[email-masked]>
List-id: L3VPN Service YANG Model discussion group <l3sm.ietf.org>

FYI. http://tools.ietf.org/id/draft-zaalouk-supa-vpn-service-management-model-02.txt
 
I took a little while to review this model from a YANG Doctor point of view. This model seems pretty good from a YANG perspective, I think.

ietf-supa-l3vpn.yang:
- list l3vpn-Instance; suggest name change to “l3vpn-instance” to conform to IETF naming conventions
A: typo, will fix it
- it is customary to surround the top level list with a container, e.g. container l3vpn-instances
A: yes you’re right, should replace it with container.
- max-elements 65535; // to be discussed ― what is the point with this limit?
A: I think this is related to the capability of controller, however for a model this should not be limited. If other people agree I will delete this line.
- leaf instance-name pattern; why is this pattern here?
A: because there are some restrictions to the names. If you think it’s  unnecessary, we will think about to remove it.
- mandatory true; on a key leaf is superfluous, suggest removing it
A: ok.
- leaf servic-type; should presumably be “service-type”?
A: typo, will fix it.
 
- list access-interface-id; somewhat odd name for a list, wouldn’t “access-interface” be more appropriate?
A: yes, will fix it.
- max-elements 65535 ― what is the point with this limit?
A: similar to above, but this is to limit the number of PE connecting to one VPN. If you think it’s  unnecessary, we will think about to remove it.
- leaf access-interface-id; should this be a reference to the interface table in ietf-interfaces.yang?
A: yes, you’re right. The reference way  is much better and we will use it in next version.
- mandatory true; on a key leaf is superfluous, suggest removing it
A:  yes, will fix it
- leaf access-interface-address, ip-address-mask-length, maybe replace with "ip-prefix” of type “inet:ip-prefix”? This type allows both IPv4 and IPv6 prefixes (IP/mask)
A: yes, will fix it
- leaf role; since this leaf seems relevant only for hub-spoke service types, maybe add a when-_expression_?   when "../../service-type = ‘hub-spoke’”;
A: Good point. The when expression  when "../../service-type = ‘hub-spoke’” will be added.
 
- leaf user-name, leaf user-password; I seriously doubt that we want to have device management information in the l3vpn model. That kind of information should be stored in a separate model, or left out of scope. It’s not the customer defining the VPN that should fill in these credentials.
A: This is because VPN need the authentication, one typical way is to use user name和password, so that in CE dynamic migration case, CEs are able to access with authentication regardless location. 
- leaf physical-node-id; I think this should be a reference to another module where the managed physical nodes are listed. An arbitrary string value will not do.
A: This is the id of access PE, I’m not sure whether there is exactly the same yang definition in other document that can be directly referred to. 
- leaf physical-access-interface; I think this should be a reference to the list of interfaces on that device. An arbitrary string value will not do. Also, is the the interface referenced always going to be a “physical” interface? How does this leaf differ from the leaf access-interface-id above? Maybe a clarifying description would help.
A: We will think about this and come back to you later. 
 
- container protocol; the description is a little short; expand or remove.
A: yes, will fix this.
- leaf protocol-type; are the only relevant protocols ever going to be “bgp”, “ospf”, “isis”? If this set could grow over time, potentially, then an identity-ref might be a better idea. This way it’s easy to define a module with additional alternatives in the future. Even proprietary ones.
A: there might be static and RIP. We will think about to use identityref.
- container igp-attribute and container bgp-attribute would probably be better integrated with the protocol-type, perhaps making the whole construct a choice between igp and egp protocols, with an identityref inside (if it is conceivable that the set of protocols could grow). That would also remove the current issue with strange default “0” statements.
- leaf remote-peer-address is currently defined as mandatory in such a way that it has to be specified even if the selected protocol is an igp protocol. Reworking the protocol-type into a choice would fix that.
A: actually  all of three comments here(including above 2) are linked up. We will think about to reworking this part following your suggestion to make things clear.

ietf-supa-ddc.yang:
- container ddc-service; suggest name “ddc-services” to conform to conventions
A: will fix this.
- leaf name; mandatory true; superfluous for a key, suggest to remove this
A: yes, will remove this.
- leaf tenant-name; mandatory true; if an operator wants to leave this blank, is that a problem?
A: we assume that the ddc service was created for tenant, but you’re right in some particular case blank is an option. It will be removed.
- leaf-list dc-name; should this be a reference to a list of data centers? keeping this as a string is weak
A: for such a service, there will be a list of DCs with their names as identifiers, like what we wrote in the description. I’m not sure what do you mean?
- leaf-list interface-name; modeling this as a leaf-list of strings seems very weak to me. Could we turn this into references of some kind?
A: Will think about this.
- leaf connection-type; suggest “L2VPN” is named “l2vpn”, “L3VPN” is named “l3vpn”, why is native-ipv4 described as “L4VPN”?
A: sorry for the typo J , will fix this
- leaf connection-name; this should be a reference to the l3vpn instance, type leafref { path "/l3vpn:l3vpn-instance/instance-name”; }
A: thanks for pointing this. This will be replaced with leafref.
- leaf vlanId; suggest name “vlan-id”; description could tell what happens if vlan-id is not set
A: sorry for the typo, will change to vlan-id and with a default vlan-id value.
- leaf bandWidth; suggest name “bandwidth” or “speed”; would be good to use a larger type, e.g. uint64; would be good to specify unit, e.g. units “bps”;
A: will change to bandwidth and add units
- leaf latency; is this something that is configured? would be good to specify unit, e.g. units “ns”;
A: Here the latency is a requirement

In other l3vpn models I have seen, there have usually been some qos configurations as well. Would we want to add that?
A: Sure, we could add qos info in next version.
 
/Jan Lindblad