Re: [Lime] Alia Atlas' Discuss on draft-ietf-lime-yang-connectionless-oam-14: (with DISCUSS and COMMENT)

[Alia Atlas <[e-mail]>]

Thanks!   This all looks good.

My assumption for augments is that the base has to be a normative reference.

Good news is that all those drafts are progressing...

On Oct 25, 2017 10:45 PM, "Qin Wu" <[e-mail]> wrote:

> Thank Alia for valuable comments and Thanks Benoit for clarification,
>
> Please see my reply inline below.
>
> *发件人:* Benoit Claise [mailto:[e-mail]]
> *发送时间:* 2017年10月26日 10:27
> *收件人:* Alia Atlas; The IESG
> *抄送:* Ron Bonica; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> *主题:* Re: Alia Atlas' Discuss on draft-ietf-lime-yang-connectionless-oam-14:
> (with DISCUSS and COMMENT)
>
>
>
> Hi Alia,
>
> Some answers below.
> I let the authors chime in.
>
> Alia Atlas has entered the following ballot position for
>
> draft-ietf-lime-yang-connectionless-oam-14: Discuss
>
>
>
> When responding, please keep the subject line intact and reply to all
>
> email addresses included in the To and CC lines. (Feel free to cut this
>
> introductory paragraph, however.)
>
>
>
>
>
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
>
> for more information about IESG DISCUSS and COMMENT positions.
>
>
>
>
>
> The document, along with other ballot positions, can be found here:
>
> https://datatracker.ietf.org/doc/draft-ietf-lime-yang-connectionless-oam/
>
>
>
>
>
>
>
> ----------------------------------------------------------------------
>
> DISCUSS:
>
> ----------------------------------------------------------------------
>
>
>
> Thank you for your work on this document.  I have a number of serious concerns
>
> - but they all amount to fixing up your references and slight restructuring
>
> for clarity and reuse.
>
>
>
> 1) In Sec 3.1, the reference is system-id to represent the device or
>
> node.[I-D.ietf-spring-sr-yang] I believe that should be "typedef router-id {
>
>        type yang:dotted-quad;
>
>        description
>
>          "A 32-bit number in the dotted quad format assigned to each
>
>           router. This number uniquely identifies the router within
>
>           an Autonomous System.";
>
>      }"
>
> from draft-ietf-rtgwg-routing-types.
>
> Certainly "[I-D.ietf-spring-sr-yang]" is NOT an informative reference with such
>
> a dependency.
>
>
>
> I see that this document actually redefines router-id, instead of using it as
>
> part of the included import from
>
>  import ietf-routing-types {
>
>    prefix rt;
>
>   }
>
> This was part of my AD review (on the LIME mailing list) in August.
>
> - Cut and paste the typedefs from https://datatracker.ietf.org/
> doc/draft-ietf-rtgwg-routing-types/
>
>     typedef router-id {
>
>     typedef ipv4-multicast-group-address
>
>     typedef ipv6-multicast-group-address {
>
>         ...
>
> If published fast enough, you should import the types from
> https://datatracker.ietf.org/doc/draft-ietf-rtgwg-routing-types/
>
> I'm happy to see that draft-ietf-rtgwg-routing-types is finally approved.
> So authors, it's time to import the router-id
>
> [Qin]: My fault, we thought we have addressed comments raised by AD Review
> but forget to clean up router-id we ourselves defined,
>
> We will fix this.
>
>
>
> On p.27, I see "leaf system-id {
>
>           type rt:router-id;
>
>           description
>
>             "System ID assigned to this node.";
>
>         }"
>
> so it is using the routing-yang-types, but renaming it as system-id, there.
>
> Consistency isn't just the hobgoblin of little minds - it's actually useful.
>
> Make sense to me.
>
> [Qin]: Good catch, will fix this, thanks.
>
>
>
> In choice to-location, again "case system-id {
>
>           leaf system-id-location {
>
>             type router-id;
>
>             description
>
>               "System id location";
>
>           }
>
>           description
>
>             "System ID";"
>
> using the locally defined router-id and renaming it instead of using
>
> rt:router-id.
>
>
>
> [Qin]: Will fix this, thanks.
>
>
>
> 2) On p. 13 & 14, there are many identities associated with time and
>
> time-stamps.  I cannot believe that the best way to handle these is by having
>
> them as part of an OAM model!   At a minimum, they should be defined as a
>
> separate module and then included, even if it is in the same draft.  Then they
>
> will be available for reuse elsewhere.
>
>
>
> [Qin]:Umm, good suggestion, I prefer to keep it in the same draft. Thanks.
>
>
>
> 3) This is extending [I-D.ietf-i2rs-yang-network-topo] - I do not believe this
>
> should be merely an informative reference.
>
> Note sure what you mean:
>
>
>
>  augment "/nd:networks/nd:network/nd:node" {
>
>     description
>
>       "Augment test points of connectionless oam.";
>
>         uses test-point-locations;
>
>   }
>
> [Qin]: We can make description more clear with the following changes：
>
> NEW TEXT:
>
> “
>
> augment "/nd:networks/nd:network/nd:node" {
>
>     description
>
>       " augments /networks/network/node path defined in the ietf-
>
>    network module (I-D.ietf-i2rs-yang-network-topo) with test-point-
>
>    locations grouping”
>
>   }
>
>
>
> ”
>
> https://www.yangcatalog.org/yang-search/impact_analysis.php?modules
> []=ietf-connectionless-oam&recurse=0&rfcs=1&show_subm=1&show_dir=
> dependencies
>
>
>
>
>
> 4) I cannot tell if I-D.ietf-rtgwg-ni-model is informative or normative; it is
>
> not referenced in the draft - though there are fields that are labeled NI
>
> without adequate description.
>
> https://www.yangcatalog.org/yang-search/impact_analysis.php?modules
> []=ietf-connectionless-oam&recurse=0&rfcs=1&show_subm=1&show_dir=
> dependencies
>
> The generic question is: if I import a YANG module from draft D, then D
> should be a normative reference.
>
>
>
>
>
> 5) [I-D.ietf-rtgwg-routing-types] is not an informative reference.  Its module
>
> is imported and used.  It must be normative.
>
> Yes.
>
> Regards, Benoit
>
>
>
>
>
> 6) [I-D.ietf-spring-sr-yang] is listed as an informative reference, but if it
>
> were actually used as described, it would need to be normative. Instead, I
>
> believe this can be removed as a reference.
>
>
>
>
>
> ----------------------------------------------------------------------
>
> COMMENT:
>
> ----------------------------------------------------------------------
>
>
>
> a) Sec 3.8: It is unfortunate that the cc-session-statistics-data structure is
>
> not a list of {traffic type, cc-session-statistics} instead of hardcoded
>
> members for IPv4 and IPv6 traffic only.  While it can still be extended for
>
> additional traffic types, the naming may be inconsistent and there's no
>
> requirement that the contents are cc-session-statistics.
>
>
>
> [Qin]: Good suggestion, the proposed change could be:
>
> “
>
>   container cc-session-statistics-data {
>
>     if-feature "continuity-check";
>
>     config false;
>
>     list cc-session-statistics {
>
>        key type;
>
>        leaf type {
>
>         type identityref {
>
>          base traffic-type;
>
>         }
>
>         description
>
>          "Type of traffic.";
>
>        }
>
> container cc-ipv4-sessions-statistics {
>
>         when "../type = 'ipv4'" {
>
>          description
>
>           "Only applies when traffic type is Ipv4.";
>
>         }
>
>
>
>       description
>
>         "CC ipv4 sessions";
>
>       uses cc-session-statistics;
>
>     }
>
> container cc-ipv6-sessions-statistics {
>
>         when "../type = 'ipv6'" {
>
>          description
>
>           "Only applies when traffic type is Ipv6.";
>
>         }
>
>       description
>
>         "CC ipv6 sessions";
>
>       uses cc-session-statistics;
>
>     }
>
>                       description
>
>       "List of CC session statistics data.";
>
>                     }
>
>                      description
>
>     "CC operational information.";
>
>   }
>
> ”
>
>
>
> b) On p.9: " +--:(system-id)
>
>       |                 +--rw system-id-location?      router-id"
>
>
>
> Why isn't this just named router-id instead of system-id, for consistency?
>
> This comment applies throughout the draft.
>
>
>
> [Qin]: Okay, will fix this consistency issue.
>
>
>
> c) The use of "tp" to mean test-point is a bit unfortunate in a model that is
>
> building off of the network topology one, which uses "tp" for termination-point.
>
>
>
> [Qin]: YANG validation tool doesn’t complain about this.
>
>
>
> d) On p. 13: "identity address-attribute-types {
>
>     description
>
>       "This is base identity of address
>
>        attribute types which are ip-prefix,
>
>        bgp, tunnel, pwe3, vpls, etc.";
>
>   }"
>
>
>
> I haven't a clue what is meant by a bgp address attribute type or a tunnel one.
>
>  Can you please expand the description to be substantially more meaningful?
>
> How is it used?
>
>
>
> [Qin]: Good comments, we actually reference RFC4379 for these address attribute types. The proposed changes look like as follows:
>
> “
>
>   identity address-attribute-types {
>
>     description
>
>       "This is base identity of address
>
>        attribute types which are Generic
>
> IPv4/IPv6 Prefix,BGP Labeled
>
> IPv4/IPv6 Prefix,Tunnel ID,
>
>        PW ID, vpls VE ID, etc.(See RFC4379
>
>                        for details.)";
>
>   }
>
> ”
>
> On p. 24, I see these defined
>
> " case bgp {
>
>             leaf bgp {
>
>               type inet:ip-prefix;
>
>               description
>
>                 "BGP Labeled Prefix ";
>
>             }
>
>           }
>
>           case tunnel {
>
>
>
>             leaf tunnel-interface {
>
>               type uint32;
>
>               description
>
>                 "VPN Prefix ";
>
>             }
>
>           }
>
>           case pw {
>
>             leaf remote-pe-address {
>
>               type inet:ip-address;
>
>               description
>
>                 "Remote pe address.";
>
>             }
>
> "
>
> but unlike the other cases with clear descriptions and references to the
>
> relevant RFCs, these are NOT clear and do not even fully expand acronyms.
>
>
>
> [Qin]: Same as above, we will add RFC4379 as reference. Thanks.
>
>
>
> e) "grouping tp-address-ni "  Please expand what NI is the abbreviation for in
>
> the description.
>
>
>
> [Qin]: Okay. Thanks.
>
>