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

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

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.

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