Re: [Netconf] LC on subscribed-notifications-10

> From: Robert Wilton -X, March 12, 2018 12:41 PM
> 
> Hi,
> 
> Some minor comments on this draft.  I've not read/reviewed all of it yet, so
> some more comments may follow.  But generally I am supportive of publishing
> this draft.

Thanks Robert.   

I have incorporated tweaks as-per below.  These updates can be seen within the working draft -v11 at:
https://github.com/netconf-wg/rfc5277bis/blob/master/draft-ietf-netconf-subscribed-notifications-11.txt 

>    I'm happy to leave it to the authors discretion on whether and how they
> want to address these.
> 
> 1. I noted that neither the title nor abstract mention YANG or network
> configuration.  Nor do they mention that they define a YANG data model.

This is true, and I think ok.     Just like RFC-5277, it is possible to subscribe to non-YANG streams.

> 2. Intro:
>   - Should security be somewhere in the intro section?

Do you see a particular need to emphasize security here?  Not sure why there should be a specific break-out here.

> 3. Section 1.1:
> - Operational counters and instrumentation, it wasn't really clear to me
> whether, or how the draft really addresses this point.

There are counters for the number of events which pass and fail filters for a subscription on a stream.  These are the YANG objects "pushed-notifications" & "excluded-notifications"

I have tweaked the text to "per-subscription operational counters" to cover this.

> - State change notification => "Subscription state change notifications"?

Every time "state change notification" is used, the word subscription is easily matched up.   So it seems to me to be more readable to use a shorter term versus a longer one.

> 4. Sec 1.2:
> - Configuration isn't necessarily persistent across reboots.  E.g. a configured
> subscription over something like I2RS.  Perhaps expand that configured
> subscriptions are normally persistent across reboots, but that depends on the
> behavior of the configuration subsystem.

I made it: 
Configured subscription: A subscription installed via a configuration interface.  Typically a configured subscription is placed into a configuration subsystem which supports persistence across reboots.

> - Steam: Is this really a set, or a sequence.  Perhaps ... "A continuous
> chronologically ordered sequence of events ..."?  If you change this, then
> perhaps grep for usage of "set" in the draft to see if they should be changed to
> sequence.

I like the addition of "chronologically", put that word in.  Regarding "sequence": with "ordered" we likely have that covered sufficiently.  Plus RFC-5277 used  "set", so this is closer match to the original stream definition.

> - I found the description of "Subscription" to be somewhat unclear.
> 
> 5 Sec 1.3:
> 
> - I would have moved section 1.3 to the start of section 2.

Ack.  It has moved around over previous iterations.  I kind of got used to it being in the Intro section.

> - "starts pushing notification messages" -> "starts pushing notification messages
> to the subscriber/receiver".

Made it "back to the subscriber".

> - In paragraph starting "The lifetype or a dynamic" ... "running configuration" ->
> "applied configuration".

Changed

> - In para starting "Configured subscriptions".  Should mention that
> subscriptions can also be killed by someone else with appropriate permissions.

We thought about this.  It seemed to be too much detail this early in the document.  It should be adequately covered in later sections.

> - In para starting "The publisher MAY",  "MAY decide to" => "MAY", in two
> places.

Changed

>  In general I would suggesting checking all the MUST/MAYs and perhaps
> try to term them more explicit things that they MUST/MAY/SHOULD do.
> 
> 6 Sec 2.
> 
> - Does this section need an intro, perhaps if section 1.3 moved here that would
> help?

To me it feels redundant to have that right after the intro 1.3.   If other people feel 1.3 should be moved down, I have no problem with it.

> - Sec 2.1 last paragraph.  Is this right that it must have access to all event
> records? 

Yes

> I wasn't sure how this tied in with YANG push where a client may not
> have access to read some nodes.  In that case, nodes that can't be read are left
> out (e.g. like a NETCONF GET request).  I know that YANG push covers this is
> more detail, but didn't know whether this paragraph correctly stands on its
> own?

Current paragraph is correct.  The idea is you shouldn't allow someone to subscribe to a stream containing any content they are not allowed to see.  Access control is to the stream rather than the content.  This is a reason why yang push is a more flexible approach in many cases.

> 7. 2.3 Qos, para starting "a dscp": "Where provided, this marking" => "Where
> provided, and supported, this marking" ...

As it is a feature, if it is successfully provided, it must be supported.

> 8 2.4. Dynamic subscriptions
> 
> - Would writing "event streams" be better than "targets"?

As dynamic subscriptions also apply to datastores (as augmented by yang-push), it is better to leave the more general term.

> - Diagram in 2.4.1, I found this diagram a bit tricky to read/interpret.  One
> particular question: Can you modify a subscription for a suspended receiver.

There is a line from receiver suspended for this, so yes.

> - In para starting "Failed modify", 'modify' => '"modify subscription"'.

Changed

> 9 2.4.2. Establishing a subscription:
>   "It MUST be possible to support" => "A publisher MUST support". I would
> suggest changing both cases.

Done

> 10. In para starting "an optional start time", perhaps also describe what
> happens if no start time has been provided.  Or perhaps just keep the first
> sentence and refer to 2.4.2.1 for more details.

Added: Where there is no start time, the subscription starts immediately.

> 11. In para starting "In the publisher", "it provides an identifier" => "it replies
> with an identifier"?

Changed

> 12. Sec 2.4.2.1, first paragraph:
>    "then be followed by event records" => "then be followed by a replay
> complete event and then by event records"?

Made it:
The end of these historical event records is identified via a "replay-completed" state change notification.  Any event records generated since the subscription establishment may then follow.

> 13. Sec 2.4.3, modify description tree output.  Identifier is optional, should be
> marked as mandatory?  Or otherwise what happens if it is not provided?

Good catch.  Re-made it mandatory.  Not sure where that got dropped.

> 14. Sec 2.4.4.
> - I would have called this "ending a subscription, or terminating a subscription"
> rather than "delete" because the subscription wasn't created, it was
> established.

The only reason we couldn't use "create" was that it was already used in RFC-5277.   Delete should be fine.

> - The text mentions the publisher rejecting a subscription, when can this
> happen?

Actually it says "any" not "a".  The implication is that no other effects anywhere based on a rejected subscription request.  E.g., don't try to guess which subscription to delete.

But there is still a case for "a".  What if a subscriber (with identical credentials) established the referenced subscription on a different transport session?  The delete still should not work unless it comes on the original transport.

>  Presumably only if the subscription id is not valid, or if it doesn't own
> it.  Perhaps check that this is consistent with how the text describes killing a
> subscription.

I think it works.

> 15 Sec 2.4.6
> - Perhaps tweak the wording for "specifically ... specific".

Tweaked

> 16. Sec 2.5
> "persistence across reboots" => "persistence across publisher reboots"?

Done

> Now skipping to Section 5.3 Security considerations!
> - "all RPC requests" => "all subscriber RPC requests"?

Should be ok per the earlier words in the sentence.

> - "MUST have the ability" => Something along the lines => Publishers MUST
> ensure that they have sufficient resources to fulfill a dynamic subscription
> request or otherwise reject the request, and provide ...

Incorporated something similar.

> - "excessive use of system resources" => "excessive use of system resources on
> publisher or receiver".

Updated

> - "No notification messages SHOULD be sent" => "Notification messages
> SHOULD NOT be sent".  Doesn't => Does not.

Updated

> - Last paragraph, "it SHOULD NOT be assumed" => it cannot be assumed.

Updated

> 
> I will try and provide further comments between sec 2.5 and 5.3, but I'm
> somewhat short of time.

Appreciated, Thanks Again!
Eric

> Thanks,
> Rob
> 
> 
> On 28/02/2018 23:09, Kent Watsen wrote:
> > WG,
> >
> > The authors of netconf-event-notifications have indicated privately that they
> believe this document is now ready for Last Call.
> >
> > This starts a 2.5-week Last Call on
> > draft-ietf-netconf-subscribed-notifications-10 [1] The LC will end on March
> 17, or when active threads peter out.
> >
> > Please send your comments on this thread. Reviews of the document, and
> statement of support, are particularly helpful to the authors.  If you have
> concerns about the document, please state those too.
> >
> > Authors please indicate if you are aware of any IPR on the document.
> >
> >
> > [1]
> > https://tools.ietf.org/html/draft-ietf-netconf-subscribed-notification
> > s-10
> >
> >
> > Kent & Mahesh
> >
> >
> >
> >
> > _______________________________________________
> > Netconf mailing list
> > Netconf@ietf.org
> > https://www.ietf.org/mailman/listinfo/netconf
> > .
> >