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

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

> From: Martin Bjorklund, January 23, 2019 3:32 AM
> 
> Hi,
> 
> "Eric Voit (evoit)" <[e-mail]> wrote:
> > Hi Andy,
> >
> > Looking at your proposal...  My reading is that it takes the transport
> > specific error info contained in
> > draft-ietf-netconf-netconf-event-notifications section 7, and then
> > replicates that info within 12 separate description objects of the
> > transport independent ietf-subscribed-notifications.yang.  The value
> > you are asserting is that RFCs containing YANG models containing the
> > rpc-stmt have traditionally document the mandatory-to-implement
> > "error-tag" field within the model.  And presumably you are concerned
> > that developers should not have to look elsewhere for this
> > information.
> 
> I think that maybe there are two separate issues here.
> 
> The first issue is that for each error identity defined, there needs to be a
> mapping to the protocol-specific error handling.  Andy suggests that this info is
> added to this document, but currently this information is available in the
> protcol-mapping documents (netconf-notif and restconf-notif).  Personally, I
> think that the current split of text between documents is fine.
> 
> The second issue is that currently, both netconf-notif and restconf-notif say
> that *all* these errors use the error-tag "operation-failed".  Essentially it means
> that we bypass the error handling in the protocols.  As Andy points out below,
> the error "insufficient-resources" should be mapped to "resource-denied" in
> NETCONF and RESTCONF (they mean the same thing).  So it might make sense
> to carefully go through the list of errors and map them to the correct error-tag
> (but specifiy this in the transport drafts).

I am completely good with this.   Does this work for you Andy?

Eric

> /martin
> 
> 
> 
> 
> >
> > If the YANG doctors require this, it can be inserted.  A similar text
> > change would be needed for quite a few error identities within YANG
> > Push.  Personally I don’t like that YANG models should be required to
> > embed this information.  But I will make the change if you really want
> > this, and nobody else objects.
> >
> > Other than that, I am not aware of any other open issues in the YANG
> > Doctor review.  Do you know of anything else?
> >
> > Eric
> >
> >
> > From: Andy Bierman, January 21, 2019 2:26 PM
> >
> > Hi,
> >
> > I think the error-tag issue can be resolved by including 1 extra
> > sentence in each error identity.
> > I know this is NETCONF and RESTCONF centric but those are the only 2
> > standard protocols supported for the YANG language right now.
> >
> >        If the 'error-tag' field is used in error reporting,
> >        then the value '<correct error-tag>' MUST be used.
> >
> > For example:
> >
> >
> > OLD:
> >
> >   identity insufficient-resources {
> >     base establish-subscription-error;
> >     base modify-subscription-error;
> >     base subscription-suspended-reason;
> >     description
> >       "The publisher has insufficient resources to support the
> >        requested subscription.  An example might be that allocated CPU
> >        is too limited to generate the desired set of notification
> >        messages.";
> >   }
> >
> >
> > NEW:
> >
> >   identity insufficient-resources {
> >     base establish-subscription-error;
> >     base modify-subscription-error;
> >     base subscription-suspended-reason;
> >     description
> >       "The publisher has insufficient resources to support the
> >        requested subscription.  An example might be that allocated CPU
> >        is too limited to generate the desired set of notification
> >        messages. If the 'error-tag' field is used in error reporting,
> >        then the value 'resource-denied' MUST be used.";
> >   }
> >
> >
> > Andy
> >
> >
> > On Fri, Jan 18, 2019 at 11:53 AM Eric Voit (evoit)
> > <[e-mail]<mailto:[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.
> >
> > > > 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?
> >
> > ...
> > > 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
> >
> > ...
> > > 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."
> >
> > 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?
> >
> > ...
> > > > 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.
> >
> > > 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.
> >
> > 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.
> >
> > > 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