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

["Mehmet Ersue" <[e-mail]>]

Hi Eric,

once again I appreciate your tremendous efforts and engagement to get the notification drafts done.

There is nothing like YANG Doctors approval. The YANG Doctor in charge does the review and suggests changes following RFC6087bis and the YANG language standard. The YANG doctors (reviewers) are actually supposed to help the OPS AD responsible for Network Management to review YANG documents that are destined to be on the IESG agenda.

As  draft-ietf-netconf-subscribed-notifications is a Netconf WG document, I think the WG needs decide on issues which are not very clear and require some discussion on how to change. 

Cheers,
Mehmet

> -----Original Message-----
> From: netconf <[e-mail]> On Behalf Of Eric Voit (evoit)
> Sent: Friday, January 18, 2019 8:53 PM
> To: Andy Bierman <[e-mail]>
> Cc: [e-mail]; [e-mail]; draft-ietf-netconf-subscribed-
> [e-mail]
> Subject: Re: [netconf] Yangdoctors last call review of draft-ietf-netconf-
> subscribed-notifications-21
> 
> 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
> 
> _______________________________________________
> netconf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netconf