[netconf] Yangdoctors last call review of draft-ietf-netconf-yang-push

Martin Bjorklund <mbj@tail-f.com> Wed, 30 January 2019 15:51 UTC

Return-Path: <mbj@tail-f.com>
X-Original-To: netconf@ietfa.amsl.com
Delivered-To: netconf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 1F091130EB9; Wed, 30 Jan 2019 07:51:28 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.9
X-Spam-Level:
X-Spam-Status: No, score=-1.9 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
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 aKPd4P52dlq7; Wed, 30 Jan 2019 07:51:24 -0800 (PST)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id B2046130F4F; Wed, 30 Jan 2019 07:51:20 -0800 (PST)
Received: from localhost (h-4-215.A165.priv.bahnhof.se [158.174.4.215]) by mail.tail-f.com (Postfix) with ESMTPSA id 77CAE1AE012C; Wed, 30 Jan 2019 16:51:18 +0100 (CET)
Date: Wed, 30 Jan 2019 16:51:18 +0100
Message-Id: <20190130.165118.1684515639662649114.mbj@tail-f.com>
To: yang-doctors@ietf.org, draft-ietf-netconf-yang-push.all@ietf.org
Cc: netconf@ietf.org
From: Martin Bjorklund <mbj@tail-f.com>
X-Mailer: Mew version 6.7 on Emacs 25.2 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Multipart/Mixed; boundary="--Next_Part(Wed_Jan_30_16_51_18_2019_579)--"
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/rDk8FCRIqYyBdyQiK-SJ0uFlULw>
Subject: [netconf] Yangdoctors last call review of draft-ietf-netconf-yang-push
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: NETCONF WG list <netconf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/netconf>, <mailto:netconf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/netconf/>
List-Post: <mailto:netconf@ietf.org>
List-Help: <mailto:netconf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/netconf>, <mailto:netconf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 30 Jan 2019 15:51:29 -0000

Hi,

This is my YANG doctor's review of draft-ietf-netconf-yang-push-21.

Reviewer: Martin Björklund
Review result: Ready with Issues


o  General

  Check the output of idnits.  Specifically, add the 2119
  boilerplate.

  Also, since the YANG module uses 2119 language, add the YANG-ified
  version of the boilerplate to the description statement in the
  module, just before the copyright.


o  General

  The module is inconsistently indented and formatted.  The RFC editor
  will enforce consistent formatting, so in order to make their life
  easier it is better to do this work before it reaches them.

  Attached is a cleaned up version of the YANG module where I have
  done the following changes:

    o  pyang -f yang --keep-comments --yang-line-length 69

       (requires pyang 1.7.8)

    o  added newlines in long descriptions, to make separate
       paragraphs

    o  added extra space after "." in description statement (most of
       them already had two spaces)

    o  formatted some paragraphs (I use fill-paragraph in emacs, manually)

    o  added the 2119 boilerplate

  NOTE: I haven't done any other of the changes that I propose in
  this review.


o  Figure 6

  The tree diagram for push-change-update doesn't match the module.

  OLD:

     notifications:
       +---n push-update
       |  +--ro id?      sn:subscription-id
       |  +--ro datastore-contents?   <anydata>
       |  +--ro incomplete-update?     empty
       +---n push-change-update {on-change}?
          +--ro id?     sn:subscription-id
          +--ro datastore-changes?
          |  +--ro yang-patch
          |     +--ro patch-id        string
          |     +--ro ypatch:comment?    string
          |     +--ro ypatch:edit* [edit-id]
          |        +--ro ypatch:edit-id      string
          |        +--ro ypatch:operation    enumeration
          |        +--ro ypatch:target       target-resource-offset
          |        +--ro ypatch:point?       target-resource-offset
          |        +--ro ypatch:where?       enumeration
          |        +--ro ypatch:value?
          +--ro incomplete-update?    empty

  NEW:

     notifications:
       +---n push-update
       |  +--ro id?                   sn:subscription-id
       |  +--ro datastore-contents?   <anydata>
       |  +--ro incomplete-update?    empty
       +---n push-change-update {on-change}?
          +--ro id?                  sn:subscription-id
          +--ro datastore-changes
          |  +--ro yang-patch
          |     +--ro patch-id    string
          |     +--ro comment?    string
          |     +--ro edit* [edit-id]
          |        +--ro edit-id      string
          |        +--ro operation    enumeration
          |        +--ro target       target-resource-offset
          |        +--ro point?       target-resource-offset
          |        +--ro where?       enumeration
          |        +--ro value?       <anydata>
          +--ro incomplete-update?   empty


o  Figure 6

  The purpose of including tree diagrams is to give a easy-to-read
  overview to the reader.  When the diagram is spread out over
  several pages, it becomes more difficult to grasp.

  In this case, I would suggest that you split the diagram into
  smaller pieces, and place them together with the text that describe
  them.

  For example, in 4.2 you can have the first part of the tree that
  deals with subscription config; 4.3.1 the augmented notifs etc.


o  Figure 9

  This example is difficult to read.  I suggest:

  OLD:

   <netconf:rpc message-id="101"
     xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
     <establish-subscription
     xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
     xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
       <yp:datastore
       xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
         ds:operational
       </yp:datastore>
       <yp:datastore-xpath-filter
       xmlns:ex="http://example.com/sample-data/1.0">
         /ex:foo
       </yp:datastore-xpath-filter>
       <yp:on-change>
         <yp:dampening-period>100</yp:dampening-period>
       </yp:on-change>
     </establish-subscription>
   </netconf:rpc>

  NEW:

   <rpc message-id="101"
        xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
     <establish-subscription
         xmlns=
           "urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
         xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
       <yp:datastore
           xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
         ds:operational
       </yp:datastore>
       <yp:datastore-xpath-filter
           xmlns:ex="http://example.com/sample-data/1.0">
         /ex:foo
       </yp:datastore-xpath-filter>
       <yp:on-change>
         <yp:dampening-period>100</yp:dampening-period>
       </yp:on-change>
     </establish-subscription>
   </rpc>


o  Figure 11

  Similar in this example:

  OLD:

   <netconf:rpc message-id="102"
      xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
      <modify-subscription
      xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
      xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
       <id>1011</id>
       <yp:datastore
       xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
         ds:operational
       </yp:datastore>
       <yp:datastore-xpath-filter
         xmlns:ex="http://example.com/sample-data/1.0">
         /ex:bar
       </yp:datastore-xpath-filter>
       <yp:periodic>
         <yp:period>250</yp:period>
       </yp:periodic>
      </modify-subscription>
   </netconf:rpc>

  NEW:

   <rpc message-id="102"
        xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
     <modify-subscription
         xmlns=
           "urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
         xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
       <id>1011</id>
       <yp:datastore
           xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
         ds:operational
       </yp:datastore>
       <yp:datastore-xpath-filter
           xmlns:ex="http://example.com/sample-data/1.0">
         /ex:bar
       </yp:datastore-xpath-filter>
       <yp:periodic>
         <yp:period>250</yp:period>
       </yp:periodic>
      </modify-subscription>
   </rpc>


o  Section 4.4.4

  (The figure in this example doesn't have a number)

  OLD:

 <netconf:rpc message-id="103"
 xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
   <resync-subscription
   xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push"
   xmlns:sn="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications">
     <id>1011</id>
   </resync-subscription>
 </netconf:rpc>

  NEW:

   <rpc message-id="103"
        xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
     <resync-subscription
         xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push"
       <id>1011</id>
     </resync-subscription>
    </netconf:rpc>

o  Section 5.

  In order to get the RFC references right,  follow the pattern in
  other documents with YANG modules; e.g. RFC 8343.  Specifically,
  all references in RFCs must be found in the main text; this means
  that you need to add something like:

   This YANG module imports typedefs from [RFC6991].


o  import yang-patch

  OLD:

    reference
      "RFC 8072: YANG Patch";

  NEW:

    reference
      "RFC 8072: YANG Patch Media Type";


o  typedef change-type

  The "reference" statement contains text that should be in the
  "description" statement (this comment has been raised before on the
  ML).

  I suggest:

  OLD:

    description
      "Specifies different types of datastore changes.";
    reference
      "RFC 8072 section 2.5, with a delta that it is valid for a
       receiver to process an update record which performs a create
       operation on a datastore node the receiver believes exists,
       or to process a delete on a datastore node the receiver
       believes is missing.";

   NEW:

    description
      "Specifies different types of datastore changes.

       This type is based on the edit operations defined for YANG
       Patch, with the difference that it is valid for a receiver to
       process an update record which performs a create operation on
       a datastore node the receiver believes exists, or to process a
       delete on a datastore node the receiver believes is missing.";
    reference
      "RFC 8072: YANG Patch Media Type, section 2.5";


o  wrong type used (timeticks)

  This was discussed on the ML, but apparently the change wasn't
  introduced yet.

  A couple of 'period' nodes use the type yang:timeticks, but it is
  not an appropriate type.  RFC 6991 describes it as:

         The timeticks type represents a non-negative integer that
         represents the time, modulo 2^32 (4294967296 decimal), in
         hundredths of a second between two epochs.  When a schema
         node is defined that uses this type, the description of
         the schema node identifies both of the reference epochs.

  As Juergen pointed out, it seems you want a TimeInterval - but this
  type is not present in RFC 6991.  So add:

    type centiseconds {
      type uint32;
      description
        "A period of time, measured in units of 0.01 seconds.";
    }

  and use that instead.


o  container datastore-changes

  OLD:

    container datastore-changes {
      description
        "This contains the set of datastore changes of the target
         datastore starting at the time of the previous update, per
         the terms of the subscription.  The datastore changes are
         encoded per RFC 8027 (YANG Patch).";
      uses ypatch:yang-patch;

  NEW:

    container datastore-changes {
      description
        "This contains the set of datastore changes of the target
         datastore starting at the time of the previous update, per
         the terms of the subscription.";
      uses ypatch:yang-patch;

   (The reference was not correct, and it isn't really necessary
   anymore, since you use the grouping from yang patch.)


o  leaf datastore-xpath-filter

  This leaf has the same problem that stream-xpath-filter had; it
  doesn't work for RESTCONF.  We need to apply the same solution as
  for stream-xpath-filter:

  OLD:

       leaf datastore-xpath-filter {
         if-feature "sn:xpath";
         type yang:xpath1.0;
         description
           "This parameter contains an XPath expression identifying
           the portions of the target datastore to retrieve.
           If the expression returns a node-set, all nodes in the
           node-set are selected by the filter.  Otherwise, if the
           expression does not return a node-set, the filter
           doesn't select any nodes.
           The expression is evaluated in the following XPath
           context:
            o  The set of namespace declarations are those in scope
               on the 'datastore-xpath-filter' leaf element.
            o  The set of variable bindings is empty.
            o  The function library is the core function library, and
               the XPath functions defined in section 10 in RFC 7950.
            o  The context node is the root node of the target
               datastore.";
       }

  NEW:

       leaf datastore-xpath-filter {
         if-feature "sn:xpath";
         type yang:xpath1.0;
         description
           "This parameter contains an XPath expression identifying
           the portions of the target datastore to retrieve.

           If the expression returns a node-set, all nodes in the
           node-set are selected by the filter.  Otherwise, if the
           expression does not return a node-set, the filter
           doesn't select any nodes.

           The expression is evaluated in the following XPath
           context:

             o   The set of namespace declarations is the set of prefix
                 and namespace pairs for all YANG modules implemented
                 by the server, where the prefix is the YANG module
                 name and the namespace is as defined by the
                 'namespace' statement in the YANG module.

                 If the leaf is encoded in XML, all namespace
                 declarations in scope on the 'stream-xpath-filter'
                 leaf element are added to the set of namespace
                 declarations.  If a prefix found in the XML is
                 already present in the set of namespace declarations,
                 the namespace in the XML is used.

             o  The set of variable bindings is empty.

             o  The function library is the core function library, and
                the XPath functions defined in section 10 in RFC 7950.

             o  The context node is the root node of the target
                datastore.";
       }


o  Section A.1

  I have trouble parsing this paragraph:

   References to specific identities within the either the subscribed-
   notifications YANG model or the yang-push YANG model may be returned
   as part of the error responses resulting from failed attempts at
   datastore subscription.  Following are valid errors per RPC (note:
   throughout this section the prefix 'sn' indicates an item imported
   from the subscribed-notifications.yang model):

  "the either the" needs to be fixed.

  Suggest you spell ou the real names of the YANG modules you refer
  to (s/subscribed-notifications.yang
  model/"ietf-subscribed-notifications" YANG module/  etc)


o  Section A.1

  I don't think this document should repeat what's already specified
  in the SN draft.  Specifically, I think this section should list
  the *additional* errors that this document introduces.  I also note
  that the list in this document is not the same as in the SN draft...

  So I suggest:

  OLD:

   establish-subscription         modify-subscription
   ----------------------         -------------------
    cant-exclude                   sn:filter-unsupported
    datastore-not-subscribable     sn:insufficient-resources
    sn:dscp-unavailable            sn:no-such-subscription
    sn:filter-unsupported          period-unsupported
    sn:insufficient-resources      update-too-big
    on-change-unsupported          sync-too-big
    on-change-sync-unsupported     unchanging-selection
    period-unsupported
    update-too-big                resync-subscription
    sync-too-big                  --------------------
    unchanging-selection           no-such-subscription-resync
                                   sync-too-big

   delete-subscription            kill-subscription
   ----------------------         -----------------
    sn:no-such-subscription        sn:no-such-subscription

  NEW:

   establish-subscription         modify-subscription
   ----------------------         -------------------
    cant-exclude                   period-unsupported
    datastore-not-subscribable     update-too-big
    on-change-unsupported          sync-too-big
    on-change-sync-unsupported     unchanging-selection
    period-unsupported
    update-too-big                resync-subscription
    sync-too-big                  --------------------
    unchanging-selection           no-such-subscription-resync
                                   sync-too-big



o  Section A.2

  Similar to A.1, suggest you remove the sn: errors that are already
  covered by SN.




/martin