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

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

Hi Andy,



Thanks very much for the thorough YANG Doctor review.   I have included the agreed upon comments, and uploaded to:

draft-ietf-netconf-subscribed-notifications-22

a summary of the clarifications made is at the end of the document.  Let me know if there anything else needed to conclude the YANG doctor review of this document.



Also as the result of the ‘error-tag’ discussion with you and Martin, we need to perform the refinement of the ‘error-tag’ mapping within both draft-ietf-netconf-netconf-event-notifications Section 7, and draft-ietf-netconf-restconf-notif Section 3.3.   Directly below is some text and proposed error-tag mappings for those documents.



    o  An "error-tag" node with the value being a string that

       corresponds to an identity associated with the error.  This

       "error-tag" will correspond to the error identities within

       [I-D.draft-ietf-netconf-subscribed-notifications] section

       2.4.6 for general subscription errors:



          error identity         uses error-tag

          ---------------------- --------------

          dscp-unavailable       invalid-value

          encoding-unsupported   invalid-value

          filter-unavailable     invalid-value

         filter-unsupported     invalid-value

          insufficient-resources resource-denied

          no-such-subscription   invalid-value

          replay-unsupported     operation-not-supported

          stream-unavailable     invalid-value

          suspension-timeout     operation-failed

          unsupportable-volume   too-big



       Or this "error-tag" will correspond to the error identities

       within [I-D.ietf-netconf-yang-push] Appendix A.1 for

       subscription errors specific to YANG datastores:



          error identity              uses error-tag

          ----------------------      --------------

          cant-exclude                operation-not-supported

          datastore-not-subscribable  operation-not-supported

          no-such-subscription-resync invalid-value

          on-change-unsupported       operation-not-supported

          on-change-sync-unsupported  operation-not-supported

          period-unsupported          invalid-value

          update-too-big              too-big

          sync-too-big                too-big

          unchanging-selection        operation-failed





Do you (or anyone else in this thread) have any suggestions on the text or proposed mappings?  If this turns out to be ok, Alex will need to remove the NETCONF error-tag specifics from draft-ietf-netconf-yang-push Sections 4.4.1 & 4.4.2



Also Reshad will have to do some work because he is the YANG doctor of netconf-netconf-event-notifications, and he will want to include the same information within draft-ietf-netconf-restconf-notif.



Thanks,

Eric





From: Andy Bierman <[e-mail]>

Sent: Wednesday, January 23, 2019 12:42 PM

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

Cc: Martin Bjorklund <[e-mail]>; [e-mail]; [e-mail]; [e-mail]

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







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

> From: Martin Bjorklund, January 23, 2019 3:32 AM

>

> Hi,

>

> "Eric Voit (evoit)" <mailto:[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?



This is better.

I'm glad no other drafts are creating their own error reporting system for each rpc-stmt.

This is a bad precedent and likely to be skipped in implementations.



Eric



> /martin

>



Andy





>

>

>

> >

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

> > <mailto:[e-mail]<mailto: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