[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
- [Lime] AD review: draft-ietf-lime-yang-connection… Benoit Claise
- Re: [Lime] AD review: draft-ietf-lime-yang-connec… Qin Wu
- Re: [Lime] AD review: draft-ietf-lime-yang-connec… Benoit Claise
- [Lime] 答复: AD review: draft-ietf-lime-yang-connec… Qin Wu
- Re: [Lime] AD review: draft-ietf-lime-yang-connec… Qin Wu