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

[Andy Bierman <[e-mail]>]

On Fri, Jan 18, 2019 at 11:53 AM Eric Voit (evoit) <[e-mail]> wrote:

> Hi Andy,
>
> Thanks.  I have incorporated items where there was agreement.  I have
> removed the items below where you were ok.
>
> Remaining below are the open items, with responses.
>
> > >    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?)
>
> The identity which applies here is "stream-unavailable".  This is the same
> identity which would be used if a subscriber had never sufficient
> permissions in the first place.  I don't believe we would want to return an
> identity specific to when NACM when permissions have just been changed.
>
>
OK



> > > 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)
>
> Agree fully with this.  Is there text required in the draft here?
>
>
no - I do not have any text to suggest

...
> > 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
>
> Have put a minor tweak into Section 2.7:
>
> [old]  subscription state change notifications cannot be filtered out
>
> [new] subscription state change notifications cannot be dropped or
> filtered out
>
>
Not sure this is a good addition because the event might get dropped
further down the stack out of the server process that sent the event.


...
> > 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.
>
> I do think this possibility is covered in the text.  For Section 2.7.4
> subscription-suspended the current text is:
>
> "No further notification will be sent until the subscription resumes or is
> terminated."
>
>
OK



> And Section 2.7.5 subscription-resumed says":
> "Subscribed event records generated after the issuance of this
> subscription state change notification may now be sent."
>
> Based on the discussion, I can make it:
>
> "Subscribed event records are again permitted to be sent following this
> subscription state change notification."
>
> Is this sufficient for you?
>
>
old text is OK



> ...
> > > 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.
>
> In draft-ietf-netconf-netconf-event-notifications, section 7, the required
> "error-tag" is identified as "operation-failed".   If we instead placed
> that "error-tag" information in the YANG model, then we have tied the YANG
> model to the RESTCONF and NETCONF transports.
>
>
The subscribed-notifications draft contains rpc-stmts.
Every other RFC with such statements specifies the error handling details
for them.
This document creates a new error reporting system just for 3 or 4 RPC
operations,
and fails to specify even the mandatory fields for the error reporting
system
for 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.
>
> draft-ietf-netconf-restconf-notif Section 3.3 also requires an "error-tag"
> node of "operation-failed".  So we used the transport drafts rather than
> the YANG model to support the same 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
>
> Functionally, everything you ask for is fully covered when you include
> consider draft-ietf-netconf-netconf-event-notifications (section 7) and
> draft-ietf-netconf-restconf-notif (section 3.3).
>
> My read of the issue is that you believe "error-tag" must be specified in
> the YANG model.  I believe that "error-tag" shouldn't be in the YANG model
> because that would tie the model to a transport type.
>
>
The RFC containing the protocol operation defined with the rpc-stmt
needs to document at least the mandatory-to-implement "error-tag" field.

This is a WG or IESG review issue and maybe the IESG will not care at all
about it.



> Any thoughts on how we might close this?    If absolutely required I could
> place a new comment line in the YANG model under
> /* Identities for RPC and Notification errors */
>
> The comment would be something like:
> /* When used with NETCONF and RESTCONF RPCs:
>     "error-type" node to be used is "application"
>      "error-tag" must be "operation-failed".  */
>
> This seems incongruous.  Just throwing it out as a suggestion.
>
>
Usually the normative text contains the error-tag info.
No other operations use a YANG module to define their own error reporting
system


Andy




> > 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.
>
> Instead of implementation specific I would call it application specific.
>  Right now we don't have a catch-all error-identity of 'other-error'.  We
> preferred that error conditions beyond the current ones listed could be
> included by vendors as needed.  Further deployment experience could result
> in new error identities surfacing for standardization should this draft
> catch on.
>
> > >      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?
>
> There is nothing in the specification which requires this.   An
> implementation could choose to place these in some form of SYSLOG.
> ...
> > > 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
>
> Change made.
>
> > > 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.
>
> See next note
>
> > >   -- 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
>
> As a different YANG doctor has required the current text modification, I
> believe this is a blocker.  What is the process for YANG model reviews in
> such a case.  I am happy to accept whatever here.  Any suggestions on next
> steps?
>
> ...
> > >   -- 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
>
> Added
>
> > > 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 ;-)
>
> Thanks.  No action taken.
>
> > > 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
>
> Done
>
> > > 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.
>
> Here is an example from our implementation.  Say you mistype an extra "\"
> to an xpath filter:
> /if:interfaces-state/interface[name="GigabitEthernet0/0"]/oper-status
> As a result, the filter is passed to the publisher is:
> /if:inte\rfaces-state/interface[name="GigabitEthernet0/0"]/oper-status
>
> What we would return in the failure-hint string is:
> Invalid expression: offset(9) in
> '/if:inte\rfaces-state/interface[name="GigabitEthernet0/0"]/oper-status'
>
> Eric
>
> > Andy
>
>