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

On 12/03/2018 21:58, Eric Voit (evoit) wrote:
>> 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.
Only to make it clear that it has been considered, and the draft 
provides a solution for it.

>
>> 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.
Yes, that helps clarify.

>
>> - 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.
Ok, so when I first read this I thought that it was talking about the 
changes in state within the data, rather than state changes of the 
subscriptions themselves.  Personally, I still think that this is worth 
clarifying, but I'll leave the decision to the authors.

>
>> 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.
Looks good.

>   
>> - 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 still strongly prefer calling it a sequence, because I think that is 
what it is :-)

I quickly looked up a couple of definitions of stream in computer 
science literature and they all refer to them as sequences not sets.  Is 
the steam being referenced to this draft that same as what a normal 
software engineer would think of as a stream?

Is a stream ever allowed to contain duplicate values, e.g. on a replay 
it is possible to put the same events into the stream?  If events are 
always unique then set is fine, otherwise set just seems like the 
incorrect term.

>   
>> - 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.
OK.

>   
>> 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.
OK, I think that I'm still struggling to create a good mental picture of 
how everything fits together.

I assumed that subscriptions only subscribe to event streams (e.g. from 
first sentence in section 1).  Hence, I was assuming that in the case of 
YANG push, that the change notifications to a datastore would 
effectively be a new event stream that could be subscribed to.  Are you 
saying that it is something else?  This might need to be clarified 
somewhere.

>   
>> - 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.
I think that I find it confusing as to what is an external event coming 
from a subscriber or receiver vs internal state changes.

OK, so I think that I have finally figured out what this diagram is 
meant to be showing :-)

Hence, I think that it might help to explicitly list the external events 
below the diagram:

- External events from a subscriber: establish-subscription, 
modify-subscription, ...

- Internal states: start, receiver-ACTIVE, receiver-SUSPENDED, end.

>   
>> - 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.
OK.
>
>> 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.
OK.

>   
>> 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.
OK.

>   
>> - 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.
The first sentence of that paragraph already states that it is on the 
same transport session.  My interpretation of the current text is that a 
publisher can arbitrarily choose not to cancel a subscription.  I think 
that the text would be better if it was more explicit.

E.g. perhaps replace: " If the publisher rejects the request, the
    request has no impact whatsoever on any subscription.

with something like:

"If the delete request does not match a known subscription id of a 
subscription established on the same transport session, then it MUST be 
rejected with no changes to the system".

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

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