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

[Andy Bierman <[e-mail]>]

On Wed, Jan 16, 2019 at 10:35 AM Eric Voit (evoit) <[e-mail]> wrote:

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

OK



> > 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.
>
>
OK to leave as-is. Agree it is more complex.


> >    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.)
>
>
No -- OK to leave NACM as terminate-if-loss-of-rights
(Is there an error identity for this event?)



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

It is up to the publisher to decide when to stop sending events on a
subscription.
Obviously the publisher cannot wait until the subscription is idle.
The reason it is getting suspended is it is far from idle

So also up to the publisher wrt/ what to do with any events that have not
been delivered yet on a subscription.  Could delete them or save them for
when more bandwidth available (for example)



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

OK



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

Agreed that an implementation can drop at any time and increment the
appropriate counters. It will try to to do this, but no requirements except
maybe subscription events like 'replay-completed' cannot be dropped



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


> 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?
>
>
Not sure -- it needs to be clear that subscription-suspended is the
last event sent before suspending and subscription-resumed is
the first event sent after transition from suspended to active.
The next event could also be subscription-terminated.

Implementation details do not need to be mentioned



> >   -- [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...
>
>
OK to leave as-is


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


> >   -- how long does the server wait for a receiver when sending
> >      subscription-terminated?
>
> Per Section 2.5.4. there is no wait.
>
>
OK


> ***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.
>
>
there is no need to add a new table


> > 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
>
>
I do not agree this is a good idea.
Each error identity should simply state the required "error-tag"
that is associated with the error.  This is expected of protocol operations
that are added to NETCONF and RESTCONF.

Both NETCONF and RESTCONF use a compatible error reporting data structure.
The "error-tag" is used in both of them.  IMO client developers do not
want a different set of error codes for the same error conditions.

I agree that transport drafts could define their own error identities,
which would document the expected error-tag there.


>    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?
>
>
No -- NETCONF and RESTCONF have well-defined error reporting.
The server requirements for this error reporting must be documented.

I agree with the following approach:
  - each operation MUST identify the error-tags that are expected for
    various error conditions (such s is done in RFC 6241)
  - the server MUST return the specified error-tags. If a condition not
explicitly
    defined then the server MUST pick the appropriate error-tag from RFC
6241
 - the server MAY include the specified rc:yang-data in the <error-info>
data structure
 - the server MUST use the appropriate rc:yang-data to report hints
 - for protocols other than NETCONF and RESTCONF, they can map error-tag or
ignore it,
   but the document defining the protocol operation MUST provide

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.
>
>
IMO anything can fail for insufficient resources. That is very
implementation-specific.



> >      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.
>
>
So how are hints reported for configured subscriptions?



> > 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.
>
>
OK - the vendor can use deviations to change it if they want



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


> > 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.
>
>
I do not agree that this YANG module should define a new way to encode XPath
into XML instance documents. This will require significant changes to server
implementations.  YANG module prefixes are not even required to be unique
so the set of prefixes used by the server in XML instance documents may be
different,
since it must be unique.




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

Remove all text about YANG prefixes and continue using XML encoding without
modification


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


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

OK



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



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



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

OK



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

The YANG language makes no restrictions about exporting statements.
I guess I missed that debate so I will just say OK and wonder what problem
this is supposed to solve. I guess the WG wants to give YANG Doctors more
things to check. (This is what we called a CLR in SNMP-land ;-)



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



> > 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.
>
>
I guess I do not consider NETCONF and RESTCONF "historic" quite yet.
There are many implementations using the rpc-error reporting with no intent
to replace it with something else.

I was just asking for an example, since I have no idea what an implementor
would put in this leaf.




> > 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?....
>
>
OK -- no changes needed



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



> >      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.
>
>
OK -- leave unchanged



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


Andy