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

Benoit Claise <bclaise@cisco.com> Fri, 11 August 2017 15:11 UTC

Return-Path: <bclaise@cisco.com>
X-Original-To: lime@ietfa.amsl.com
Delivered-To: lime@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id E128A13263F for <lime@ietfa.amsl.com>; Fri, 11 Aug 2017 08:11:12 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -13.478
X-Spam-Level:
X-Spam-Status: No, score=-13.478 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=0.001, MISSING_HEADERS=1.021, RCVD_IN_DNSWL_HI=-5, SPF_PASS=-0.001, USER_IN_DEF_DKIM_WL=-7.5, WEIRD_PORT=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=cisco.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id uVaCswZpIDmJ for <lime@ietfa.amsl.com>; Fri, 11 Aug 2017 08:11:10 -0700 (PDT)
Received: from aer-iport-3.cisco.com (aer-iport-3.cisco.com [173.38.203.53]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 5387313257A for <lime@ietf.org>; Fri, 11 Aug 2017 08:11:09 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=41897; q=dns/txt; s=iport; t=1502464269; x=1503673869; h=from:subject:cc:message-id:date:mime-version; bh=Q5QrhYh72JnOjAqSxRE66XFIq4lI0VH8FkCqKMMsKN4=; b=g6QX1X2MsRJigBtms5PWrm0FxaEeXz3jZhL0e8CbT6Rcuxw6tou4ncfc NXU4aITKhmVo9WQhPhSxwjBujKjn7rsMilU8zMzwFSiZmVtbWmAKQ1iEB 0uDs/ywu2VvYJn5NNMXNB3jzaf8g2iv3G564ekWks0gDzsH6m0TXvK8nN Q=;
X-IronPort-AV: E=Sophos;i="5.41,358,1498521600"; d="scan'208,217";a="654892621"
Received: from aer-iport-nat.cisco.com (HELO aer-core-3.cisco.com) ([173.38.203.22]) by aer-iport-3.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Aug 2017 15:11:07 +0000
Received: from [10.55.221.36] (ams-bclaise-nitro3.cisco.com [10.55.221.36]) by aer-core-3.cisco.com (8.14.5/8.14.5) with ESMTP id v7BFB6Pq025371; Fri, 11 Aug 2017 15:11:07 GMT
From: Benoit Claise <bclaise@cisco.com>
Cc: "Carl Moberg (camoberg)" <camoberg@cisco.com>, Alia Atlas <akatlas@gmail.com>, "lime@ietf.org" <lime@ietf.org>
Message-ID: <ce63b3cd-a45e-dbe2-7522-acbc4272a33d@cisco.com>
Date: Fri, 11 Aug 2017 17:11:06 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1
MIME-Version: 1.0
Content-Type: multipart/alternative; boundary="------------115B2D20EFFA2DDD0450622A"
Content-Language: en-US
Archived-At: <https://mailarchive.ietf.org/arch/msg/lime/5griCD_WqTSHsIfircqTe9h9H3c>
Subject: [Lime] AD review: draft-ietf-lime-yang-connectionless-oam
X-BeenThere: lime@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: "Layer Independent OAM Management in Multi-Layer Environment \(LIME\) discussion list." <lime.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lime>, <mailto:lime-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lime/>
List-Post: <mailto:lime@ietf.org>
List-Help: <mailto:lime-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lime>, <mailto:lime-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 11 Aug 2017 15:11:13 -0000

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
    ietf-connectionless-oam@2017-06-09.yangietf-network-instance@2017-07-03.yang:1:
    error: unexpected latest revision "2017-07-02" in
    ietf-network-instance@2017-07-03.yang, 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