Re: [mpls] mpls-rt review of draft-zheng-mpls-lsp-ping-yang-cfg

["Acee Lindem (acee)" <[e-mail]>]

Hi Loa, 

I've reviewed the document and I feel the dynamics of the LSP Ping tests need to be described better. One question I have is what becomes of the configuration once an LSP Ping Test completes? How would one deterministically repeat the test at a later time? Will changing the start-time do this? Are ping results for all completed tests maintained indefinitely? I guess all test results will be maintained as long as the named LSP ping exists? This needs to be specified better. Additionally, did you consider an RPC action to initiate an LSP ping and return results on demand without retaining until the named LSP ping test is deleted? If not, another useful RPC would be one to clear the results for a named LSP ping test.    

Additionally, the model and draft are "rough" and there will be a lot of additions and changes that need to be taken care before WG last call. Also, one concern it is that it could grow substantially due to the statement:   

             "There are extensions for performing LSP ping, for example, over P2MP MPLS LSPs
               [RFC6425] or f1. In section 1, the statement regarding NDMA should refer to the LSP-Ping
               data model.

It would be good to know the scope of the document prior to asking for reviews. 

Here are my comments: 

   1. In section 1, the statement regarding NDMA should refer to the LSP-Ping
        data model.
   2. Why isn't lspping hyphenated at the top level? It is hyphenated in other places and 
        this is more readable. 
   3. Use "scheduling-parameters" rather than "schedule-parameters".
   4. Several indentation problems (fixed in diff).
   5. Add references RFC for imports.
   6. The high-level model "contact" and "description" are woefully lacking.
       Editors are not specified.
   7. Within the model, the multi-line descriptions are hard to read due to the lack
        of indentations subsequent to the first.
   8. Short descriptions sometimes end in period even when they are not
      even close to being sentences.
   9. Why do you need "source-address-type"? The ip-address type implies whether it
        Is IPv4 or IPv6
  10. Use "List of LSP Ping test instances" for top-level description.
  11. Why specify the nexthop?
  12. Which value does the sum-of-squares apply to? One could assume RTT but
         it should be specified.
   13. The "Security Considerations" doesn't conform to the latest template or address
          considerations for specific schema nodes. 
    14. I'd also recommend that one of the native English authors review
          all the description text. There are cases of missing articles and "of" is used in
          prepositional phases where other prepositions would have been a better choice.

See some nits below:
ACEE-M-G2HR:Desktop acee$ diff -c  draft-zheng-mpls-lsp-ping-yang-cfg-09.txt.orig draft-zheng-mpls-lsp-ping-yang-cfg-09.txt
*** draft-zheng-mpls-lsp-ping-yang-cfg-09.txt.orig	2018-11-10 08:20:34.000000000 -0500
--- draft-zheng-mpls-lsp-ping-yang-cfg-09.txt	2018-11-10 21:17:28.000000000 -0500
***************
*** 22,28 ****
     be detected by the MPLS control plane.  RFC 8029 defines a mechanism
     that would enable users to detect such failure and to isolate faults.
     YANG, defined in RFC 6020 and RFC 7950, is a data modeling language
!    used to specify the contents of a conceptual data stores that allows
     networked devices to be managed using NETCONF, as specified in RFC
     6241.  This document defines a YANG data model that can be used to
     configure and manage LSP-Ping.
--- 22,28 ----
     be detected by the MPLS control plane.  RFC 8029 defines a mechanism
     that would enable users to detect such failure and to isolate faults.
     YANG, defined in RFC 6020 and RFC 7950, is a data modeling language
!    used to specify the contents of a conceptual data store that allows
     networked devices to be managed using NETCONF, as specified in RFC
     6241.  This document defines a YANG data model that can be used to
     configure and manage LSP-Ping.
***************
*** 116,122 ****
  
     The rest of this document is organized as follows.  Section 2
     presents the scope of this document.  Section 3 provides the design
!    of the LSP-Ping configuration data model in details by containers.
     Section 4 presents the complete data hierarchy of LSP-Ping YANG
     model.  Section 5 discusses the interaction between LSP-Ping data
     model and other MPLS tools data models.  Section 6 specifies the YANG
--- 116,122 ----
  
     The rest of this document is organized as follows.  Section 2
     presents the scope of this document.  Section 3 provides the design
!    of the LSP-Ping configuration data model in detail for each container.
     Section 4 presents the complete data hierarchy of LSP-Ping YANG
     model.  Section 5 discusses the interaction between LSP-Ping data
     model and other MPLS tools data models.  Section 6 specifies the YANG
***************
*** 124,130 ****
     specified in this document.  Finally, security considerations are
     discussed in Section 8.
  
!    This version of the interfaces data model conforms to the Network
     Management Datastore Architecture (NMDA) [RFC8342].
  
  1.1.  Requirements Language
--- 124,130 ----
     specified in this document.  Finally, security considerations are
     discussed in Section 8.
  
!    This version of the LSP-Ping data model conforms to the Network
     Management Datastore Architecture (NMDA) [RFC8342].
  
  1.1.  Requirements Language
***************
*** 137,148 ****
  
  1.2.  Support of Long Running Command with NETCONF
  
!    LSP Ping is one of the examples of what can be described as "long-
     running operation".  Unlike most of the configuration operations that
!    result in single response execution of an LSP Ping triggers multiple
     responses from a node under control.  The question of implementing
     the long-running operation in NETCONF is still open and possible
!    solutions being discussed:
  
     1.  Consecutive Remote Processing Calls (RPC) to poll for results.
  
--- 137,148 ----
  
  1.2.  Support of Long Running Command with NETCONF
  
!    LSP Ping is one of the examples of what can be described as a "long-
     running operation".  Unlike most of the configuration operations that
!    result in single response, execution of an LSP Ping triggers multiple
     responses from a node under control.  The question of implementing
     the long-running operation in NETCONF is still open and possible
!    solutions are being discussed:
  
     1.  Consecutive Remote Processing Calls (RPC) to poll for results.
  
***************
*** 150,167 ****
  
     3.  The one outlined in [I-D.mahesh-netconf-persistent].
  
!    The problem of long-running operation as well can be considered as a
     case of controlling and obtaining results from a Measurement Agent
     (MA) as defined in [RFC7594].
  
  2.  Scope
  
     The fundamental mechanism of LSP-Ping is defined in [RFC8029].
!    Extensions of LSP-Ping has been developed over the years.  There are
     extensions for performing LSP ping, for example, over P2MP MPLS LSPs
!    [RFC6425] or for Segment Routing IGP Prefix and Adjacency SIDs with
     an MPLS data plane [RFC8287].  These extensions will be considered in
!    a later update of this document.
  
  
  
--- 150,167 ----
  
     3.  The one outlined in [I-D.mahesh-netconf-persistent].
  
!    The problem of long-running operations can be considered as a
     case of controlling and obtaining results from a Measurement Agent
     (MA) as defined in [RFC7594].
  
  2.  Scope
  
     The fundamental mechanism of LSP-Ping is defined in [RFC8029].
!    Extensions to LSP-Ping has been developed over the years.  There are
     extensions for performing LSP ping, for example, over P2MP MPLS LSPs
!    [RFC6425] or for Segment Routing IGP Prefixes and Adjacency SIDs with
     an MPLS data plane [RFC8287].  These extensions will be considered in
!    a later version of this document.
  
  
  
***************
*** 175,189 ****
     This YANG data model is defined to be used to configure and manage
     LSP-Ping and it provides the following features:
  
!    1.  The configuration of control information of an LSP-Ping test.
  
!    2.  The configuration of schedule parameters of an LSP-Ping test.
  
!    3.  Display of result information of an LSP-Ping test.
  
!    The top-level container lsp-pings holds the configuration of the
!    control information, schedule parameters and result information for
!    multiple instances of LSP-Ping test.
  
  3.1.  The Configuration of Control Information
  
--- 175,189 ----
     This YANG data model is defined to be used to configure and manage
     LSP-Ping and it provides the following features:
  
!    1.  The configuration of control information for an LSP-Ping test.
  
!    2.  The configuration of scheduling parameters for an LSP-Ping test.
  
!    3.  Display of result information from an LSP-Ping test.
  
!    The top-level container, lsp-pings, holds the configuration of the
!    control information, scheduling parameters, and result information for
!    multiple LSP-Ping test instances.
  
  3.1.  The Configuration of Control Information
  
***************
*** 191,197 ****
     parameters which control an LSP-Ping test.  Examples are the target-
     fec-type/target-fec of the echo request packet and the reply mode of
     the echo reply packet.  Values of some parameters may be auto-
!    assigned by the system, but in several cases, there is a requirement
     for configuration of these parameters.  Examples of such parameters
     are source address and outgoing interface.
  
--- 191,197 ----
     parameters which control an LSP-Ping test.  Examples are the target-
     fec-type/target-fec of the echo request packet and the reply mode of
     the echo reply packet.  Values of some parameters may be auto-
!    assigned by the system. However, in several cases, there is a requirement
     for configuration of these parameters.  Examples of such parameters
     are source address and outgoing interface.
  
***************
*** 271,279 ****
     Container lsp-pings:lsp-ping:schedule-parameters defines the schedule
     parameters of an LSP-Ping test, which describes when to start and
     when to end the test.  Four start modes and three end modes are
!    defined respectively.  To be noted that, the configuration of
     "interval" and "probe-count" parameter defined in container lsp-
!    pings:lsp-ping:control-info could also determine when the test ends
  
  
  
--- 271,279 ----
     Container lsp-pings:lsp-ping:schedule-parameters defines the schedule
     parameters of an LSP-Ping test, which describes when to start and
     when to end the test.  Four start modes and three end modes are
!    defined respectively.  Note that the configuration of
     "interval" and "probe-count" parameter defined in container lsp-
!    pings:lsp-ping:control-info could also implicitly determine when the
  
  
  
***************
*** 282,296 ****
  Internet-Draft                LSP-Ping YANG                 October 2018
  
  
!    implicitly.  All these three parameters are optional.If the user does
     not configure either "interval" or "probe-count" parameter, then the
     default values will be used by the system.  If the user configures
     "end-test", then the actual end time of the LSP-Ping test is the
!    smaller one between the configuration value of "end-test" and the
     time implicitly determined by the configuration value of
     "interval"/"probe-count".
  
!    The data hierarchy for schedule information configuration is
     presented below:
  
     module: ietf-lspping
--- 282,296 ----
  Internet-Draft                LSP-Ping YANG                 October 2018
  
  
!    test ends.  All three of these parameters are optional. If the user does
     not configure either "interval" or "probe-count" parameter, then the
     default values will be used by the system.  If the user configures
     "end-test", then the actual end time of the LSP-Ping test is the
!    sooner one between the configuration value of "end-test" and the
     time implicitly determined by the configuration value of
     "interval"/"probe-count".
  
!    The data hierarchy for scheduling information configuration is
     presented below:
  
     module: ietf-lspping
***************
*** 324,331 ****
  3.3.  Display of Result Information
  
     Container lsp-pings:lsp-ping:result-info shows the result of the
!    current LSP-Ping test.  Both the statistical result e.g. min-rtt, max
!    rtt, and per test probe result e.g. return code, return subcode, are
     shown.
  
     The data hierarchy for display of result information is presented
--- 324,331 ----
  3.3.  Display of Result Information
  
     Container lsp-pings:lsp-ping:result-info shows the result of the
!    current LSP-Ping test.  Both the statistical result, e.g., min-rtt, max
!    rtt, and per test probe result, e.g., return code, return subcode, are
     shown.
  
     The data hierarchy for display of result information is presented
***************
*** 383,389 ****
  
  4.  Data Hierarchy
  
!    The complete data hierarchy of LSP-Ping YANG model is presented
     below.
  
  
--- 383,389 ----
  
  4.  Data Hierarchy
  
!    The complete data hierarchy of the LSP-Ping YANG model is presented
     below.
  
  
***************
*** 519,528 ****
    prefix "lspping";
  
    import ietf-inet-types {
!   prefix inet;
    }
!   import ietf-yang-types{
!   prefix yang;
    }
  
    organization "IETF Multiprotocol Label Switching Working Group";
--- 519,528 ----
    prefix "lspping";
  
    import ietf-inet-types {
!     prefix inet;
    }
!   import ietf-yang-types {
!     prefix yang;
    }
  
    organization "IETF Multiprotocol Label Switching Working Group";
***************
*** 626,632 ****
      type enumeration {
        enum enabled {
          value "1";
!         description "The Test is active.";
        }
        enum disabled {
          value "2";
--- 626,632 ----
      type enumeration {
        enum enabled {
          value "1";
!         description "The test is active.";
        }
        enum disabled {
          value "2";
***************
*** 710,720 ****
            case vpn {
              leaf vrf-name {
                type uint32;
!               description "Layer3 VPN Name.";
              }
              leaf vpn-ip-address {
                type inet:ip-address;
!               description "Layer3 VPN IPv4 Prefix.";
              }
            }
            case pw {
--- 710,720 ----
            case vpn {
              leaf vrf-name {
                type uint32;
!               description "Layer-3 VPN Name.";
              }
              leaf vpn-ip-address {
                type inet:ip-address;
!               description "Layer-3 VPN IPv4 Prefix.";
              }
            }
            case pw {
***************
*** 759,766 ****
          leaf interval {
            type uint32;
            default 1;
!           description "Specifies the interval to send an LSP Ping
!           echo request packet(probe) as part of one LSP Ping test.";
          }
          leaf interval-units {
            type units;
--- 759,766 ----
          leaf interval {
            type uint32;
            default 1;
!           description "Specifies the interval at which to send LSP Ping
!           echo request packets, i.e., probes, as part of one LSP Ping test.";
          }
          leaf interval-units {
            type units;
***************
*** 795,801 ****
            portion of a probe packet.";
          }
          leaf description {
!           type string{
                length "1..31";
            }
            description "A descriptive name of the LSP Ping test.";
--- 795,801 ----
            portion of a probe packet.";
          }
          leaf description {
!           type string {
                length "1..31";
            }
            description "A descriptive name of the LSP Ping test.";
***************
*** 822,828 ****
                description "Specifies the outgoing interface.";
              }
            }
!           case nexthop{
              leaf nexthop {
                type inet:ip-address;
                description "Specifies the nexthop.";
--- 822,828 ----
                description "Specifies the outgoing interface.";
              }
            }
!           case nexthop {
              leaf nexthop {
                type inet:ip-address;
                description "Specifies the nexthop.";
***************
*** 877,885 ****
            start delay(3), start daily(4).";
          }
  
!         choice end-test{
            case at {
!             leaf end-test-at{
                type yang:date-and-time;
                description "End test at a specific time.";
              }
--- 877,885 ----
            start delay(3), start daily(4).";
          }
  
!         choice end-test {
            case at {
!             leaf end-test-at {
                type yang:date-and-time;
                description "End test at a specific time.";
              }
***************
*** 969,979 ****
            case vpn {
              leaf vrf-name {
                type uint32;
!               description "Layer3 VPN Name.";
              }
              leaf vpn-ip-address {
                type inet:ip-address;
!               description "Layer3 VPN IPv4 Prefix.";
              }
            }
            case pw {
--- 969,979 ----
            case vpn {
              leaf vrf-name {
                type uint32;
!               description "Layer-3 VPN Name.";
              }
              leaf vpn-ip-address {
                type inet:ip-address;
!               description "Layer-3 VPN IPv4 Prefix.";
              }
            }
            case pw {
***************
*** 1010,1016 ****
  Internet-Draft                LSP-Ping YANG                 October 2018
  
  
!           description "The current average LSP Ping round-trip-time
            (RTT).";
          }
          leaf probe-responses {
--- 1010,1016 ----
  Internet-Draft                LSP-Ping YANG                 October 2018
  
  
!           description "The average LSP Ping round-trip-time
            (RTT).";
          }
          leaf probe-responses {
ACEE-M-G2HR:Desktop acee$
Thanks,
Acee