Re: [Lime] AD review: draft-ietf-lime-yang-connectionless-oam

[Qin Wu <[e-mail]>]

Thanks for AD Review. We have addressed your comments in v-(09) together with other comments.

https://datatracker.ietf.org/doc/draft-ietf-lime-yang-connectionless-oam/
Just to clarify, ‘tp-tools’ grouping defined in  CL model support both proactive and on demand activation.
Grouping defined for common session  statistics on support proactive activation.
We have made this clear in the text.
Also it is intentional to separate retrieval- data from retrieval procedure, the rationale is clarified in the
introduction 1, last paragraph of draft-ietf-lime-yang-connectionless-oam-methods.

-Qin
发件人: Lime [mailto:[e-mail]] 代表 Benoit Claise
发送时间: 2017年8月11日 23:11
抄送: [e-mail]; Carl Moberg (camoberg); Alia Atlas
主题: [Lime] AD review: draft-ietf-lime-yang-connectionless-oam

Dear all,

Here is my AD review.

- I see that the draft is NMDA-compliant.  Good.
https://yangcatalog.org:8443/search/modules/ietf-connectionless-oam.yang,2017-06-09,ietf

-

   RPC - A Remote Procedure Call, as used within the NETCONF protocol
This document is about a YANG data model. So independently of NETCONF or RESTCONF or something else.
So referring to NETCONF only is not right. I would remove "as used within the NETCONF protocol"
Btw, RFC 7950 makes the distinction between :

   o  RPC: A Remote Procedure Call.



   o  RPC operation: A specific Remote Procedure Call.
You should include both RPC and RPC operations in section 2
You want to review all "RPC" instances. Most of the time, RPC should be: RPC operation(s).

- section 1

   It can be used in conjunction with data retrieval method model

   [I-D.ietf-lime-yang-connectionless-oam-methods<https://tools.ietf.org/html/draft-ietf-lime-yang-connectionless-oam-07#ref-I-D.ietf-lime-yang-connectionless-oam-methods>], which focuses on

   data retrieval procedures like RPC.


"focuses on data retrieval procedures like RPC". Is that right?
It seems to me that  [I-D.ietf-lime-yang-connectionless-oam-methods<https://tools.ietf.org/html/draft-ietf-lime-yang-connectionless-oam-07#ref-I-D.ietf-lime-yang-connectionless-oam-methods>]  is only about on-demand activation and retrieval.
See the AD review for draft-ietf-lime-yang-connectionless-oam-methods

- section 3 is too dense to understand without a YANG tree diagram.
Btw, having a YANG tree diagram is a MUST in RFC 6087bis.
I had to create my own in order to (easily) understand this draft
$ pyang -f tree [e-mail][e-mail]:1<mailto:[e-mail][e-mail]:1>: error: unexpected latest revision "2017-07-02" in [e-mail]<mailto:[e-mail]>, should be 2017-07-03
module: ietf-connectionless-oam
    +--ro cc-session-statistics-data {continuity-check}?
       +--ro cc-ipv4-sessions-statistics
       |  +--ro cc-session-statistics
       |     +--ro session-count?              uint32
       |     +--ro session-up-count?           uint32
       |     +--ro session-down-count?         uint32
       |     +--ro session-admin-down-count?   uint32
       +--ro cc-ipv6-sessions-statistics
          +--ro cc-session-statistics
             +--ro session-count?              uint32
             +--ro session-up-count?           uint32
             +--ro session-down-count?         uint32
             +--ro session-admin-down-count?   uint32
  augment /nd:networks/nd:network/nd:node:
    +--rw tp-location-type-value?                      identityref
    +--rw (location-type)?
       +--:(ipv4-location-type)
       |  +--rw test-point-ipv4-location-list
       |     +--rw test-point-locations* [ipv4-location]
       |        +--rw ipv4-location    inet:ipv4-address
       |        +--rw vrf?             routing-instance-ref
       |        +--rw (technology)?
       |        |  +--:(technology-null)
       |        |     +--rw tech-null?       empty
       |        +--rw tp-tools
       |        |  +--rw continuity-check    boolean
       |        |  +--rw path-discovery      boolean
       |        +--rw root?            <anydata>
       |        +--rw oam-layers* [index]
       |           +--rw index                        uint16
       |           +--rw level?                       int32
       |           +--rw (tp-location)?
       |              +--:(mac-address)
       |              |  +--rw mac-address-location?        yang:mac-address
       |              +--:(ipv4-address)
       |              |  +--rw ipv4-location?               inet:ipv4-address
       |              +--:(ipv6-location)
       |              |  +--rw ipv6-address?                inet:ipv6-address
       |              +--:(group-ip-address-location)
       |              |  +--rw group-ip-address-location?   ip-multicast-group-address
       |              +--:(as-number-location)
       |              |  +--rw as-number-location?          inet:as-number
       |              +--:(system-id-location)
       |                 +--rw system-id-location?          router-id
       +--:(ipv6-location-type)
       |  +--rw test-point-ipv6-location-list
       |     +--rw test-point-locations* [ipv6-location]
       |        +--rw ipv6-location    inet:ipv6-address
       |        +--rw vrf?             routing-instance-ref
       |        +--rw (technology)?
       |        |  +--:(technology-null)
       |        |     +--rw tech-null?       empty
       |        +--rw tp-tools
       |        |  +--rw continuity-check    boolean
       |        |  +--rw path-discovery      boolean
       |        +--rw root?            <anydata>
       |        +--rw oam-layers* [index]
       |           +--rw index                        uint16
       |           +--rw level?                       int32
       |           +--rw (tp-location)?
       |              +--:(mac-address)
       |              |  +--rw mac-address-location?        yang:mac-address
       |              +--:(ipv4-address)
       |              |  +--rw ipv4-location?               inet:ipv4-address
       |              +--:(ipv6-location)
       |              |  +--rw ipv6-address?                inet:ipv6-address
       |              +--:(group-ip-address-location)
       |              |  +--rw group-ip-address-location?   ip-multicast-group-address
       |              +--:(as-number-location)
       |              |  +--rw as-number-location?          inet:as-number
       |              +--:(system-id-location)
       |                 +--rw system-id-location?          router-id
       +--:(mac-location-type)
       |  +--rw test-point-mac-address-location-list
       |     +--rw test-point-locations* [mac-address-location]
       |        +--rw mac-address-location    yang:mac-address
       |        +--rw (technology)?
       |        |  +--:(technology-null)
       |        |     +--rw tech-null?              empty
       |        +--rw tp-tools
       |        |  +--rw continuity-check    boolean
       |        |  +--rw path-discovery      boolean
       |        +--rw root?                   <anydata>
       |        +--rw oam-layers* [index]
       |           +--rw index                        uint16
       |           +--rw level?                       int32
       |           +--rw (tp-location)?
       |              +--:(mac-address)
       |              |  +--rw mac-address-location?        yang:mac-address
       |              +--:(ipv4-address)
       |              |  +--rw ipv4-location?               inet:ipv4-address
       |              +--:(ipv6-location)
       |              |  +--rw ipv6-address?                inet:ipv6-address
       |              +--:(group-ip-address-location)
       |              |  +--rw group-ip-address-location?   ip-multicast-group-address
       |              +--:(as-number-location)
       |              |  +--rw as-number-location?          inet:as-number
       |              +--:(system-id-location)
       |                 +--rw system-id-location?          router-id
       +--:(group-ip-address-location-type)
       |  +--rw test-point-group-ip-address-location-list
       |     +--rw test-point-locations* [group-ip-address-location]
       |        +--rw group-ip-address-location    ip-multicast-group-address
       |        +--rw vrf?                         routing-instance-ref
       |        +--rw (technology)?
       |        |  +--:(technology-null)
       |        |     +--rw tech-null?                   empty
       |        +--rw tp-tools
       |        |  +--rw continuity-check    boolean
       |        |  +--rw path-discovery      boolean
       |        +--rw root?                        <anydata>
       |        +--rw oam-layers* [index]
       |           +--rw index                        uint16
       |           +--rw level?                       int32
       |           +--rw (tp-location)?
       |              +--:(mac-address)
       |              |  +--rw mac-address-location?        yang:mac-address
       |              +--:(ipv4-address)
       |              |  +--rw ipv4-location?               inet:ipv4-address
       |              +--:(ipv6-location)
       |              |  +--rw ipv6-address?                inet:ipv6-address
       |              +--:(group-ip-address-location)
       |              |  +--rw group-ip-address-location?   ip-multicast-group-address
       |              +--:(as-number-location)
       |              |  +--rw as-number-location?          inet:as-number
       |              +--:(system-id-location)
       |                 +--rw system-id-location?          router-id
       +--:(group-as-number-location-type)
       |  +--rw test-point-as-number-location-list
       |     +--rw test-point-locations* [as-number-location]
       |        +--rw as-number-location    inet:as-number
       |        +--rw vrf?                  routing-instance-ref
       |        +--rw (technology)?
       |        |  +--:(technology-null)
       |        |     +--rw tech-null?            empty
       |        +--rw tp-tools
       |        |  +--rw continuity-check    boolean
       |        |  +--rw path-discovery      boolean
       |        +--rw root?                 <anydata>
       |        +--rw oam-layers* [index]
       |           +--rw index                        uint16
       |           +--rw level?                       int32
       |           +--rw (tp-location)?
       |              +--:(mac-address)
       |              |  +--rw mac-address-location?        yang:mac-address
       |              +--:(ipv4-address)
       |              |  +--rw ipv4-location?               inet:ipv4-address
       |              +--:(ipv6-location)
       |              |  +--rw ipv6-address?                inet:ipv6-address
       |              +--:(group-ip-address-location)
       |              |  +--rw group-ip-address-location?   ip-multicast-group-address
       |              +--:(as-number-location)
       |              |  +--rw as-number-location?          inet:as-number
       |              +--:(system-id-location)
       |                 +--rw system-id-location?          router-id
       +--:(group-system-id-location-type)
          +--rw test-point-system-info-location-list
             +--rw test-point-locations* [system-id-location]
                +--rw system-id-location    inet:uri
                +--rw vrf?                  routing-instance-ref
                +--rw (technology)?
                |  +--:(technology-null)
                |     +--rw tech-null?            empty
                +--rw tp-tools
                |  +--rw continuity-check    boolean
                |  +--rw path-discovery      boolean
                +--rw root?                 <anydata>
                +--rw oam-layers* [index]
                   +--rw index                        uint16
                   +--rw level?                       int32
                   +--rw (tp-location)?
                      +--:(mac-address)
                      |  +--rw mac-address-location?        yang:mac-address
                      +--:(ipv4-address)
                      |  +--rw ipv4-location?               inet:ipv4-address
                      +--:(ipv6-location)
                      |  +--rw ipv6-address?                inet:ipv6-address
                      +--:(group-ip-address-location)
                      |  +--rw group-ip-address-location?   ip-multicast-group-address
                      +--:(as-number-location)
                      |  +--rw as-number-location?          inet:as-number
                      +--:(system-id-location)
                         +--rw system-id-location?          router-id
An example of this section being "dense", you wrote "Each test point location includes a 'test-point-location-info'."
And I searched for it in the tree, and it's not present ... because it's a grouping!
NEW: "Each test point location includes a 'test-point-location-info' grouping

Another example:

   Grouping is also defined for common session

   statistics and these are applicable for proactive OAM sessions.
At this point in time, we don't know what a "proactive OAM session" is. I scratched my head on "proactive" and I I know about OAM... (It's only later that I understand that proactive = continuous = persistent)

My advice: this section 3 is key to understand this module, make it crystal clear.


- Editorial

   In connectionless OAM, the tp address is defined with the following

   type:
Since you defined "TP" in the terminology section, use the upper case "TP" throughout the document.

-

   To define a forwarding treatment of a test packet, the 'tp-address'

   needs to be associated with additional parameters, e.g.  DSCP for IP

   or TC for MPLS.
TC?
I'm still used to EXP, and had to look TC up.
Quoting the Internet (so it can't be wrong): Based on RFC5462, the EXP bit has been renamed to Traffic Class field. TC name is not uses widely.

- section 3.2

   In connectionless OAM, 'session-

   type' is defined to indicate which kind of activation will be used by

   the current session.
And I searched for "session-type" in the pyang tree, without success.
Until I realized it's part of the other draft: https://tools.ietf.org/html/draft-ietf-lime-yang-connectionless-oam-methods-05
Hmm, wait, the session-type grouping is in this draft, but not used in this draft.
At this point, I wonder who has recently looked at the draft with a fresh pair of eyes. Not impressed.

-
Why don't you clearly name the groupings in section 3.4, 3.5, 3.6, 3.7?

- 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/

- section 3.3

             list oam-layers {

                  key "index";

                  leaf index {

                     type uint16 {

                        range "0..65535";

                     }

                  }

                  leaf level {

                      type int32 {

                           range "-1..1";

                      }

                      description

                        "Level";

                  }



                  description

                     "List of related oam layers.";

                }


This is not even complete. Description "level", really?
At the very minimum be aligned with the YANG module.

     list oam-layers {

        key "index";

        leaf index {

          type uint16 {

            range "0..65535";

          }

          description

            "Index";

        }

        leaf level {

          type int32 {

            range "-1..1";

          }

          default "0";

          description

            "Level 0 indicates default level,

             -1 means server and +1 means client layer.

             In relationship 0 means same layer.";

        }

        ...

        description

          "List of related oam layers.

           0 means they are in same level, especially

           interworking scenarios of stitching multiple

           technology at same layer. -1 means server layer,

           for eg:- in case of Overlay and Underlay,

           Underlay is server layer for Overlay Test Point.

           +1 means client layer, for example in case of

           Service OAM and Transport OAM, Service OAM is client

           layer to Transport OAM.";

      }




- Throughout the document, yang -> YANG

- The security considerations have been updated: https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines

My quick summary:
Those two LIME drafts are hard to read. I've been spending hours hours to try to understand...
I advice the authors, shepherd, chairs to review the two drafts with a fresh mind and improve readability.

Regards, Benoit