[Netconf] Yangdoctors last call review of draft-ietf-netconf-subscribed-notifications-21

[Andy Bierman <[e-mail]>]

Reviewer: Andy Bierman
Review result: Ready with Issues

based on draft-21
ietf-subscribed-notifications@2018-12-19

General Comments:

  -- the document is well-written and the feature-set is
     very comprehensive. There are a lot of YANG features (10)
     but they are defined to allow for a large set of
     publisher platforms.

  -- the extensibility mechanisms for filters, transport,
     and encoding should be valuable over time. There is
     a risk of proprietary solutions that do not interoperate
     but this base YANG module provides enough functionality.
     Reliance on future augmentations is new but should be OK

  -- advanced use of groupings, refine-stmt and augment-stmt
     within groupings makes the module difficult for humans
     to fully understand without additional tools, and without
     the normative text outside the YANG module.

Issues:

I1) 1.4.  Relationship to RFC 5277

  -- this section should mention that stop-time is not coupled to
     notification replay like RFC 5277. This parameter can be used
     on any stream, even if replay is not supported at all.

I2) sec 2.1 para 5:
   If subscriber permissions change during the lifecycle of a
   subscription and event stream access is no longer permitted, then the
   subscription MUST be terminated.

  -- Why can't server suspend until NACM configured
   (i.e., MUST be terminated or suspended). Should be clear somewhere that
   suspend is for CPU and other resources, and NACM not considered
   to be a resource.

I3) sec 2.1 para 6:
   Event records MUST NOT be delivered to a receiver in a different
   order than they were placed onto an event stream.

  -- does this apply to subscription-state? Think not, they are not events
    placed in event stream. Need to allow ended or suspended to be sent
    head-of-line whenever state changes
  -- Maybe another table or more text should be added listing the
     notifications that indicates
       (a) sent-for-configured
       (b) sent-for-dynamic
       (c) inserted at end of event stream, middle, or sent head-of-line

     replay-completed         Y  Y  end
     subscription-completed   Y  N  end
     subscription-modified    Y  Y  middle [1]
     subscription-resumed     Y  Y  head [2]
     subscription-started     Y  N  head
     subscription-suspended   Y  Y  head
     subscription-terminated  Y  Y  head [3]

  -- [1] not clear where in the event stream subscription-modified is sent.
     Maybe implementation-dependent? e.g. does filter-change inserted
     at slot N means all events N+i are done with new filter?
  -- [2] not clear where in the event stream subscription-resumed is sent.
     There may be events ready to send. Clearly resumed is sent before
     any of these after transition to active state
  -- [3] not clear that subscription terminated right away then all
     events for all receivers are deleted and only subscription-terminated
     is sent to all receivers; sec 2.7.3 says this but not in module
  -- insert point may be specific to each receiver, not each subscription
  -- how long does the server wait for a receiver when sending
     subscription-terminated?

I4) sec 2.4.6: RPC Failures
  -- concern about a subscription-specific error reporting system
     must make sure protocol error reporting system is used correctly

  -- The error-tag value needs to be identified for each 'reason' identity

   2.  "modify-subscription-stream-error-info": This MUST be returned
       with the leaf "reason" populated if an RPC error reason has not
       been placed elsewhere within the transport portion of a failed
       "modify-subscription" RPC response.  This MUST be sent if hints

  -- all 3 paragraphs like this; unclear what "placed elsewhere"
      text means; not appropriate for MUST;  Only 3 fields seem
      to be relevant (error-tag, error-app-tag, error-info).
      Protcol operations are expected to document server requirements
      for these 3 fields, if applicable.  Only the error-tag
      is mandatory-to-use.

  -- the error assignments are extremely specific. e.g., it is not
     possible for <kill-subscription> to fail with an
     'insufficient-resources' error; Do not agree that scoping each
     identity to specific RPC operations is a good idea.

  -- how are errors in these parameters reported for configured
     subscriptions when <edit-config> is the RPC that has the error?
     How are the yang-data structs used for edit-config or commit errors?

I5) sec 2.5

   Multiple configured subscriptions MUST be supportable over
   a single transport session.

  -- why is this a MUST instead of SHOULD? explain harm to
     interoperability if not supported

I6) sec 2.5, para 3:

   On a receiver of a
   configured subscription, support for dynamic subscriptions is
   optional except where replaying missed event records is required.

  -- confusing because text in 1.3:
     Note that there is no mixing-and-matching of dynamic and configured
     operations on a single subscription.  Specifically, a configured
  -- clarify the receiver may have multiple subscriptions here
  -- not clear what "except where replaying..." text means


I7) leaf stream-xpath-filter: [multiple uses]

           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.

  -- This prefix processing is not done anywhere else in NETCONF
     or RESTCONF.  IMO a bad precedent.  Only the XML prefixes
     should be required for processing of XML encoding.  YANG
     module prefixes are not required to be unique, unlike
     the prefix mappings in XML
  -- NMDA allows the same module to appear in multiple module-sets
     and different in each datastore. This text about "implemented by
     the server" does not work for NMDA


I8) leaf /subscriptions/subscription/encoding {
      when 'not(../transport) or derived-from(../transport,
      "sn:configurable-encoding")';
      type encoding;

  -- when-stmt is odd; there are no configurable-encodings specified
  -- there should be an example of a configurable encoding provided
  -- maybe add some text noting this is not the "encoding" leaf in
     establish-subscription.  Text is confusing since not clear how
     a dynamic subscription would use this leaf inside subscription-policy
     grouping
  -- it is only clear in the YANG tree that modify-subscription does
     not allow encoding to be changed.  Is this worth mentioning in
     the establish-subscription/input/encoding leaf?

I9) leaf /streams/stream/description {
        type string;
        mandatory true;

  -- it is odd to see an admin-string be mandatory. should add explanation
     why this is mandatory (in config false even more odd)

I10) leaf /subscriptions/subscription/configured-subscription-state
        if-feature "configured";
          enum concluded {
            value 3;
              description
                "A subscription is inactive as it has hit a stop time,
                but not yet been removed from configuration.";
          }

  -- it is also possible for stop-time to be reached but not all
     events delivered to all receivers on the subscription.
     Is this a different state or part of 'concluded'?

I11) extension subscription-state-notification {

       This statement is not for use
       outside of this YANG module.";

  -- this text should be removed. There is no value in limiting
     the scope of this extension.  It prevents even this WG from
     creating a module that uses the extension again.

I12)   notification replay-completed {

    description
      "This notification is sent to indicate that all of the replay
        notifications have been sent. It must not be sent for any other
       reason.";

  -- 2nd sentence should be removed. It is implied that the notification
     is only sent for its defined purpose.  No other notifications
     have this type of text.

I13)   notification subscription-started {
    sn:subscription-state-notification;
    if-feature "configured";
    description
      "This notification indicates that a subscription has started and
        notifications are beginning to be sent. This notification shall
       only be sent to receivers of a subscription; it does not
       constitute a general-purpose notification.";

  -- 2nd sentence is confusing; all notifications are sent to
     receivers of a subscription. last part is redundant since
     the sn:subscription-state-notification extension is used

I14)   rc:yang-data modify-subscription-stream-error-info {

      leaf filter-failure-hint {
        type string;
          description
            "Information describing where and/or why a provided filter
             was unsupportable for a subscription.";
      }

  -- rpc-error already allows more precise error reporting
     It uses error-tag, error-path, error-string, and error-info extensions
     to identify which parameters/conditions caused the RPC to be rejected.
     This error reporting will continue to be used, Not sure this failure-hint
     has any standards value. Perhaps real-use example can be added

I15)  notification subscription-completed {
    sn:subscription-state-notification;
    if-feature "configured";
    description
      "This notification is sent to indicate that a subscription has
       finished passing event records, as the 'stop-time' has been
       reached.";

  -- Could be more clear:
     A) replay in use and stop-time has past:
        * send replay-completed
        * send subscription-completed
     B) no replay and stop-time has past:
        * send subscription-completed

I16) leaf replay-previous-event-time {

  -- It is not clear why this leaf is needed
     How does this relate to replay-log-creation-time
     and replay-log-aged-time?

Nits:

2.3:
This document provide for several QoS parameters
  -- s/provide/provides

2.4, para 1:
  -- term 'target' usage is confusing, inconsistent with sec 1.2

reference-stmt:
    "RFC XXXX:Customized Subscriptions to a Publisher's Event Streams";
 -- Does not match document title

feature configured:
      "This feature indicates that configuration of subscription is
      supported.";
 -- s/subscription/subscriptions/

refine "target/stream/replay-start-time" {
  description
    "Indicates the time that a replay using for the streaming of

 -- sentence seems mangled; needs repair