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

From: Benoit Claise <[email-masked]>
Date: Fri, 11 Aug 2017 17:11:06 +0200

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
    [email-masked]@2017-07-03.yang:1:
    error: unexpected latest revision "2017-07-02" in
    [email-masked], 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

---

From: Qin Wu <[email-masked]>
Date: Fri, 1 Sep 2017 04:12:38 +0000

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:[email-masked]] 代表 Benoit Claise
发送时间: 2017年8月11日 23:11
抄送: [email-masked]; Carl Moberg (camoberg); Alia Atlas
主题: [Lime] AD review: draft-ietf-lime-yang-connectionless-oam

Dear all,

Here is my AD review.

-

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

   [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

"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

$ pyang -f tree [email-masked]@2017-07-03.yang:1<mailto:[email-masked]@2017-07-03.yang:1>: error: unexpected latest revision "2017-07-02" in [email-masked]<mailto:[email-masked]>, should be 2017-07-03
       |              |  +--rw group-ip-address-location?   ip-multicast-group-address
       |              |  +--rw group-ip-address-location?   ip-multicast-group-address
       |              |  +--rw group-ip-address-location?   ip-multicast-group-address
       |        +--rw group-ip-address-location    ip-multicast-group-address
       |              |  +--rw group-ip-address-location?   ip-multicast-group-address
       |              |  +--rw group-ip-address-location?   ip-multicast-group-address
                      |  +--rw group-ip-address-location?   ip-multicast-group-address
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:

   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

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

-

   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

   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.

-

- Cut and paste the typedefs from https://datatracker.ietf.org/doc/draft-ietf-rtgwg-routing-types/

    typedef router-id {

        ...
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 oam-layers {

        key "index";

        leaf index {

          type uint16 {

            range "0..65535";

          }

          description

            "Index";

        }

        leaf level {

          type int32 {

            range "-1..1";

          }

          default "0";

          description

        }

        ...

        description

      }

- 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