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

["Eric Voit (evoit)" <[e-mail]>]

Hi Andy,

Thanks for the review.  Some thoughts....

> From: Andy Bierman, January 14, 2019 8:08 PM
> 
> 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.

If you are ok with the following text, I can add the a new clarification bullet to this section:
"A subscription "stop-time" can be specified as part of a notification replay.  This supports an analogous capability to the stopTime parameter of [RFC 5277].  However in this specification, a "stop-time" parameter can also be applied without replay." 

> 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).

The original reason for 'MUST be terminated' was that it was more deterministic / simpler option.  And it seems a reasonable solution considering permissions changes that collide with existing subscription is something which is likely to be a rare corner case.   If an implementation does also want to support suspended, then the implementation will then have to monitor permissions to get that subscription out of suspension.  And adding support for this would add more text, complexity, and NACM integration to the specification.  The best option of course would be to consider NACM implications for subscriptions in flight prior to making permissions changes.  With this, adding the extra complexity of suspension wouldn't be necessary.

>    Should be clear somewhere that
>    suspend is for CPU and other resources, and NACM not considered
>    to be a resource.

If NACM is active, it needs to be followed.  The text we have for NACM is in Section 5.4.  Do you see something else specific to subscription suspension needed here?  (Maybe I am not getting your point.)
 
> 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. 

Agree that they are not on the event stream.  So they do not violate this requirement.  

Additionally there is supporting text in "Section 2.7: subscription state notifications", including...

" Instead, they are inserted (as defined in this section) within the sequence of notification messages sent to a particular receiver."

>     Need to allow ended or suspended to be sent
>     head-of-line whenever state changes

I am not sure that suspended should always be sent head-of-line.   Consider that implementation might want to let the existing queue of filtered event records be sent if is filter complexity causing the CPU issue.   That could be different than if it is a bandwidth issue driving the suspension, and you definitely want the 'subscription-suspended' to be placed at the head of line.

>   -- 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]

See thinking on this table below at ***table thoughts***
 
>   -- [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?

Yes.  The text which indicates this is in Section 2.5.1...

"A "subscription-modified" subscription state change notification will
   be sent to all active receivers, immediately followed by notification
   messages conforming to the new parameters."

There is also text in the YANG model "notification subscription-modified" saying

      "Notification messages sent from this point on will 
       conform to the modified terms of the subscription."

Beyond that it is up to the implementation to decide if some un-transmitted queue of event records should be flushed and reprocessed based on the modification.  I do not expect this would popular, as a replay subscription could accomplish this same functional need.

>   -- [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.

Yes.  The text on this is in Section 2.7.5 should help.  There is also text in the YANG module...

   "Subscribed event records generated after the issuance of this
   subscription state change notification may now be sent."

Thinking more on your point, it might be worth tweaking a couple words to allow for head-of-line placement of "subscription-suspended".

   "Subscribed event records queued for sending after the issuance of this
   subscription state change notification may now be sent."

Are you good with this suggested change?

>   -- [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

I am not sure if the module is the best place to embed this.  My thinking was the module should defining the meaning of the notification and its relationship to other model elements, and the behavior of the publisher prior to the issuance of the notification can be included in the specification text.  As a result beyond Section 2.7.3, there is also related text in Section 2.5.4...

   "Immediately after a subscription is successfully deleted, the
   publisher sends to all receivers of that subscription a subscription
   state change notification stating the subscription has ended (i.e.,
   "subscription-terminated")."

>   -- insert point may be specific to each receiver, not each subscription

This is true, and is a natural function of the text is Section 2.5.4.

>   -- how long does the server wait for a receiver when sending
>      subscription-terminated?

Per Section 2.5.4. there is no wait.

***table thoughts*** The table does provide an interesting summary view.   And do I appreciate you putting it together.  Based on the notes for [1], [2], and [3], my perception is that the requirements are covered within the existing text.  Is it an absolute requirement from the YANG Doctors that this summary table be embedded as a new section of the draft (perhaps as a Section 2.7.8?)  I am hoping to avoid major new text additions at this point.

> 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

Yes.  We have done our best to integrate with the embedded NETCONF and RESTCONF mechanisms.  There is much additional information in the transport drafts here.
 
>   -- The error-tag value needs to be identified for each 'reason' identity

This is done in the transport drafts.   E.g., see draft-ietf-netconf-netconf-event-notifications Section 7
 
>    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;  

Instead of "placed elsewhere", how about: "placed in subscription transport document defined object".  Would this be sufficient?

In any case, the -v21 wording results from the attempted balancing the WG requests for:
* merging with transport protocol error mechanisms
* WG leadership guidance to provide requirements for transport documents

>      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.

Hopefully these are covered sufficiently when this document is coupled with the NETCONF and RESTCONF Notif transport documents.   For other transports, the tags you identify about would not be applicable.
 
>   -- the error assignments are extremely specific. e.g., it is not
>      possible for <kill-subscription> to fail with an
>      'insufficient-resources' error; 

This is the intent of the base specification, e.g., we don't believe a kill-subscription should fail for an insufficient-resources reason.   But vendors might desire more specificity.  As a result is certainly ok for vendor implementations to add new error identities.

>      Do not agree that scoping each
>      identity to specific RPC operations is a good idea.

This level of specificity was not the author's original plans.  Nor was this level of specificity part of earlier draft versions up through -v08.   However members of the WG made it clear that such specificity was necessary for draft progression.
 
>   -- 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?

None of these yang-data structures are specified for use with <edit-config> operations.   For <edit-config>, the change to a configured subscription would be written to the datastore if it were semantically valid.   At this point the subscription enters the [evaluate] points of Figure 8.  Issues from this point out would be reported with a vendor specific construct such as SYSLOG.
 
> 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

From the receiver side, a receiver must be able to differentiate between state notifications for different subscriptions.  If a publisher can't support more than one, it doesn't actually hurt interoperability with a receiver.  It just doesn't meet the minimum criteria set for feature support for the capability as defined in this document.
 
> 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

How about the following tweak:

"On a receiver of a configured subscription, support for dynamic subscriptions is optional.  However if replaying missed event records is required for a configured subscription, support for dynamic subscription is highly recommended.  In this case, a separate dynamic subscription can be established to retransmit the missing event records."

> 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

This text was proposed by Martin as a result of the "xpath expressions in JSON" thread last October in NETMOD.

I am happy to incorporate whatever text is appropriate.  I was hoping that the suggested text was sufficient for now.  Kent has already incorporated this as an issue for yang-next
https://github.com/netmod-wg/yang-next/issues/55 
So hopefully there is no final precedent being claimed. 

>   -- 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

I am happy to adopt whatever text meets YANG doctor approval.   Can you suggest? 

> 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

This constraint was suggested by Martin last June in the thread:
https://www.ietf.org/mail-archive/web/netconf/current/msg14402.html 
more details can be seen at:
https://mailarchive.ietf.org/arch/msg/netconf/wHQjZuJnh7asJvpE1nIovPDnESM 

>   -- there should be an example of a configurable encoding provided

I am happy to enhance the definition YANG model's identity definition of "configurable-encoding".  I could do this by adding the following additional text to the description: "An example of a configurable encoding might be a new identity such as 'encode-cbor'.  Such an identity could use 'configurable-encoding' as its base.  This would allow a dynamic subscription encoded in JSON [RFC-8259] to request notification messages be encoded via CBOR [RFC-7049].  Further details for any specific configurable encoding would be explored in a transport document based on this specification."   Does this meet your ask?

>   -- 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

Dynamic subscription is possible.  This is the intent of drafts like 
draft-birkholz-yang-core-telemetry-01

>   -- 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?

I don't think the encoding leaf of establish-subscription needs to note that it cannot be modified.  There are other objects which cannot be modified as well, they are not explicitly called out.   In the end, the current model matches the intended behavior per the text above.  And it is only possible to request an encoding for delivered event records be different from that of the RPC during the establish subscription.

> 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)

Thinking about this some, it certainly should be there.  But does not need to be mandatory.  So I have changed the leaf description so that it is no longer mandatory.

> 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'?

I have no problem supporting your request for refinement/specificity.  As a result, I have tweaked the definition as follows...

                "A subscription is inactive as it has hit a stop time, 
                it no longer has receivers in the 'receiver active' or 
                'receiver suspended' state, but not yet been 
                removed from configuration.";

> 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.

This was the subject of significant debate in the WG.  The authors did not want this restriction either.  

To be allowed to progress the document, we inserted the document.  If this really is mandatory-to-remove from a YANG doctor point-of-view, what is the process for quick closure on this issue between WG leadership and the YANG doctors?
 
> 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.

Done
 
> 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

There is no issue with removing this second sentence completely.  If I did that, would this address your concern?

> 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

Per your thoughts on rpc-error...   For NETCONF and RESTCONF, you point to error structures which historically been used with those transports.  Of course we were looking to have all subscription hints supportable across transports via a single portable YANG data structure.  So the value is that a single string object exists so to transport whatever the vendor thinks would be useful as a hint in this case.  I.e., there has been no attempt to standardize the contents of this string.  If operational experiences drive a desire for such structuring, this could provide the basis for a new draft building off of this starting point.  

> 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

A "replay-completed" might have already been sent if the subscription started with replay, but the replay concluded before the "stop-time".  Based on this, are you good with the following clarification sentence at the end of the current description?....

       "If this is a replay subscription, and a 'replay-completed' subscription state notification has not been sent, the 'replay-completed' must be sent prior to the 'subscription-completed'." 

> I16) leaf replay-previous-event-time {
> 
>   -- It is not clear why this leaf is needed

The purpose of the leaf is included in the second paragraph of the description

            If a receiver previously received event records for this 
            configured subscription, it can compare this time to the
            last event record previously received.  If the two are not 
            the same (perhaps due to a reboot), then a dynamic replay 
            can be initiated to acquire any missing event records."

More on the leaf is also described in Section 2.5.6., paragraph 4.    

>      How does this relate to replay-log-creation-time
>      and replay-log-aged-time?

It does not relate directly to those objects.  But indirectly if the replay log is empty (or timed out), then the first paragraph of the description applies...

            "If there is at least one event in the replay buffer prior 
            to 'replay-start-time', this gives the time of the event 
            generated immediately prior to the 'replay-start-time'.

If it helps, I could add one more sentence to the first paragraph:

             "If there is no prior event in the replay buffer, this object MUST NOT be populated."

But this seems like over-specifying to me. 

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

Fixed

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

Are you ok if I tweak the text to:
"These RPCs have been designed extensibly so that they may be augmented for use beyond event streams."

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

Fixed to:
"RFC XXXX:Subscription to YANG Event Notifications"

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

Done

> refine "target/stream/replay-start-time" {
>   description
>     "Indicates the time that a replay using for the streaming of
> 
>  -- sentence seems mangled; needs repair

Made it:
"replay is using"

Thanks again for doing a very complete YANG Doctor review.

Eric