Subject: [Netconf] RGW YANG push -18 LC comments [part 1]

From: Robert Wilton <[email-masked]>
Date: Tue, 4 Sep 2018 15:37:25 +0100

Hi,

I've reviewed the -18 YANG push draft up to section 4 so far . I've 
included my comments below.  I think that they are all minor/cosmetic 
except for the issue regarding the dampened subscriptions which has been 
discussed below and probably does not need to be rehashed again.

In the Abstract:
(1) Is "continuous" the right choice of word here.  E.g. I thought that 
sometimes the stream of updates is not continuous.  Possibly remove 
"continuous"?

In the Introduction:
(2) "remote visibility" in the first sentence is a bit obscure. Perhaps 
replace the first sentence with "Traditional approaches to monitoring 
management data on remote devices have been built upon polling".

(3) The work isn't actually fruitless if polling is the only available 
option, hence perhaps change:

OLD:
"For applications that monitor for changes, many remote polling
  cycles place ultimately fruitless load on the network, devices,
  and applications."

NEW:
   "For applications that monitor for changes, many remote polling
   cycles place unwanted load on the network, devices,
   and applications, particularly if the monitored values are only 
changing infrequently."

(4) In paragraph starting " A more effective alternative", possibly 
suggest changing "publisher" to "server" since the introduction is 
before the definitions/acronyms where "publisher" is formally referenced.

In definitions:

(5) Suggest tying this definition more closely to the terminology 
already defined in RFC 8342, hence:

OLD:
    o  Datastore node: An instance of management information in a
       datastore.  Also known as "object".

NEW:
    o  Datastore node: A node in the instantiated YANG data tree 
associated with a datastore.  In this document, datastore nodes are 
often referred to as "YANG objects" or "objects"

(6) "Datastore node update" definition: Does this update only contain 
the value, or does it also contain the path to the datastore node as 
well?  If this also contains the path then it would be good if that was 
explicit in the definition.

(7) Perhaps the following definition is more precise?  Or by "updates" 
do you actually mean "update records"?  There are other references to 
"updates" in the definitions, do these all refer to "update records"? If 
so then I would suggest moving the definition of "update record" above 
"datastore subscription".

OLD:

"Datastore subscription: A subscription to updates regarding
       contents of a datastore."

NEW:

"Datastore subscription: A subscription to a stream of datastore node 
updates."

(8) Suggest removing "instantiated" from the following definition, and 
using descendant (which is used in 7950) instead of contained .. I also 
suggest moving the definition up, directly below "datastore node":

OLD:

    o Datastore subtree: An instantiated datastore node and the
       datastore nodes that are hierarchically contained within it.

NEW:

    o Datastore subtree: A  datastore node and all its descendant 
datastore nodes."

(9) Sec 3. Solution overview, last sentence.   It is not "YANG objects" 
that are being pushed, but surely "datastore node updates" or perhaps 
"update records" that are pushed, or even "Changes to YANG objects"

Sec 3.1 Subscription model:

(10) Perhaps change"subtrees within a datastore" to "datastore subtrees".

(11) It is not obvious to me from this text what "Anchor time" is, 
although I suspect that it is described in more detail later in the 
document.  Given that the section on-change subscriptions has more 
detail for various parameters, I wonder whether it would be helpful to 
describe anchor time in more detail in this section?

(12) Dampening period: The text states that "The dampening period 
collectively applies to the set of all datastore nodes selected by a 
single subscription and sent to a single receiver."  This would imply 
that if a single subscription had multiple receivers that they would be 
dampened independently, but it wasn't obvious to me that this makes 
sense.  I would assume that for a given subscription that all receivers 
would receive the same update records.

(13) Rather than "No Sync on start", it might be better to refer to this 
as  "sync on start", which could be defined as a boolean with default 
true [I've not checked the YANG model]

Sec 3.2. Negotiation of Subscription Policies

(14) "Random guessing at" => "Random guessing of".  Although, I think 
that probably this first sentence, and the "Therfore, " could just be 
deleted.  I.e. starting the paragraph "In order to minimize ..."

(15) Suggest minor rewording of 3.3. third paragraph:

OLD:

In that case, updates are sent only for those objects
    within the scope that do support on-change updates whereas other
    objects are excluded from update records, whether or not their values
    actually change.

NEW:

In that case, updates are sent only for those objects
    within the scope that do support on-change updates, whereas other
    objects are excluded from update records, even if their values change.

(16) Question on 3.3. fifth paragraph (below), it's unclear to me what 
is meant by "which are current at the time":

"Values sent at the end of the
    dampening period are the current values of all changed objects which
    are current at the time the dampening period expires."

(17) I still doubt that all implementations will strictly follow all of 
3.3. paragraph 5, particularly the created and deleted within a 
dampening period because I perceive that it is particularly difficult to 
implement cleanly/efficiently.  However, this issue has been discussed 
previously, and I have no particular desire to slow the RFC progress 
down with further discussion on this.

(18) Section 3.3, conceptual process, step 1 states that access control 
rules are checked to ensure the receiver is authorized to view all 
subscribed datastore nodes, but I think that this should state that the 
access control rules are used to filter the subscribed datastore nodes 
to those that the client has access rights to.

(19) A related question:  When happens if there is an access control 
change, i.e. the receiver had access previously, but then it was 
revoked.  Does it see a delete?  Or does it not get any notification?  
Similarly, if they didn't previously have access, but then access was 
granted before the end of the dampening period?  OK, I see that this is 
described in more detail in section 3.9, paragraph 3.  Perhaps a forward 
reference would be useful here.

(20) Section 3.3, last paragraph, "can be created" => "must be created".

(21) Section 3.4, second paragraph doesn't quite scan.  "This includes
    indicating the fact that an update is incomplete as part of a push-
    update respectively push-change-update notification ..."

(22) Section 3.4, 3rd paragraph, "able fulfill" => "able to fulfill"

(23) Section 3.5.2, 1st paragraph, "not restricted to configuration 
objects only" => "not only restricted to configuration objects".

(24) Section 3.5.2, 2nd paragraph, possible alternative text:

OLD:

    A publisher will indicate a change to the effect that a value of a
    datastore node has been updated by indicating a "replace" operation
    (applied to the datastore node) in the patch.  When a new datastore
    node was created (other than an entry in a user ordered list), a
    publisher will indicate a "create" operation in the patch.  When a
    datastore node was deleted, the publisher indicates this by a
    "delete".  When a new entry in a user-ordered list was created, the
    publisher indicates this by an "insert" operation. When an entry in
    a user-ordered list was moved, the publisher indicates this by a
    "move" operation.

NEW:

   A publisher indicates the type of change to a datastore node using 
the different YANG patch operations:  the "create" operation is used for 
newly created objects (except entries in a user-ordered list), the 
"delete" operation is used for deleted objects (including in 
user-ordered lists), the "replace" operation is used when only the 
object value changes, the "insert" operation is used when a new entry is 
inserted in a list, and the "move" operation is used when an existing 
entry in a user-ordered list is moved.

(25) Section 3.5.2, last paragraph: If you want to allow duplicate 
"create" and "delete" events, then it might be cleaner to encode them as 
"merge" and "remove"?  I.e. it seems that you have created a slightly 
different encoding from vanilla YANG patch because of the way that 
errors are handled differently.  Ideally, it would be better if a client 
could just apply this as regular YANG patch, but this only works with 
the encoding described here if the clients copy of the datastore is in 
sync with the servers old copy of the datastore at the point that the 
patch is generated from.  This seems slightly fragile to unexpected 
inconsistencies between client and server (which I appreciate that you 
try and avoid).

(26) Section 3.6, second paragraph, would it not be easier to allow the 
client to replace an existing filter rather than having to explicitly 
remove the old filter and add the new one?  Or if you mean that any new 
filter replaces the existing filter, then this may be more clearly 
indicated with the text: "An RPC request proposing a new selection 
filter replaces any existing filter. "

(27) Section 3.6, paragraph 6, starting "XPath".  The "boolean" example 
is unclear to me given that a xpath filter always returns a node set so 
can't return a boolean result.  I also suggest changing "passes a 
datastore node" to  "includes a datastore node".

(28) Section 3.6, paragraph 7.  I think that the text must be more 
explicit on when the filter is evaluated for a periodic subscription.  
Am I right in thinking that it is only evaluated once when the 
subscription is created, or the subscription is changed? Specifically, 
if the nodeset evaluated during the lifetime of an Xpath subscription 
changes then does that affect which nodes are returned?

(29) Section 3.7, paragraph 3:  Perhaps make this text conditional to 
dampened subscriptions.  Presumably for non dampened subscriptions all 
changes are reported as separate push-change-update notifications?

OLD:
In cases where multiple changes have occurred and the object
    has not been deleted, the object's most current value is reported.

NEW:
In cases where multiple changes have occurred in a subscription that has 
dampening enabled, and the object
    has not been deleted, the object's most current value is reported.

(30) Section 3.7, paragraph 5, "Ethernet port" => "Ethernet interface"

(31) Section 3.7, last paragraph: Why start at "1" rather than "0". On 
overflow, why overflow to "1" rather than "0", is that deliberate?  If 
you are going to force it to "1" then I would also restrict the upper 
bound to int32 max rather than uint32 max to make life easier in Java 
land ...

(32) Section 3.8, tree diagrams.  I would think that another key metric 
regarding whether a subscription can be satisfied would be object-rate 
and data-rate.  E.g. a publisher implementation may be able to push 100k 
objects/second, so a client has a choice of either monitoring less 
objects, or decreasing the period.  Hence I was wondering whether rates 
should be provided in addition to the object-count-estimate/limit and 
kilobyte-estimate/limit.  Of course, a rate is also implicitly defined 
by including the period hint, so perhaps that is sufficient without 
needing to send more fields back to the client.

(33) Section 3.9, 2nd paragraph, "particular each" => "particular"

(34) Section 3.9, 3rd paragraph:  It was not entirely obvious to me what 
the 3rd paragraph text (reproduced below) actually means.  I think that 
this boils down to:  If, after access control has been applied, there 
are no objects remaining in an update record then only a single empty 
"push-update" notification MAY be sent and empty "push-change-update" 
messages MUST NOT be sent.  This is required to ensure that clients 
cannot surreptitiously monitor objects that they do not have access to 
via carefully crafted selection filters.

"A publisher MUST allow for the possibility that a subscription's
    selection filter references non-existent or access-protected data.
    Such support permits a receiver the ability to monitor the entire
    lifecyle of some datastore tree.  In this case, all "push-update"
    notifications must be sent empty, and no "push-change-update"
    notifications will be sent until some data becomes visible for a
    receiver."

(35) Section 3.10.  This text seems to be very closely related to the 
text in section 3.3, so perhaps section 3.10 would be better moved to 
become section 3.3.1?

(36) Section 3.10, last paragraph.  "will be expected to" => "SHOULD"?

(37) Section 3.11.1 "incomplete-update" flag.  In the YANG definition 
this flag is set in the header of the message (i.e. before the data), 
but for applications that are generating the notifications in a streamed 
way, it would probably be better to put the flag after data.  This 
potentially minimizes the requirement that a publisher has to buffer all 
data to be able to set the incomplete-update flag if necessary.

(38) Section 3.11.1 3rd paragraph and bullet points.  With the text as 
currently written, it is acceptable for a publisher to do nothing even 
if an error is hit.  They can satisfy the first "MUST" by selecting the 
2nd choice, and they can satisfy the "MAY" of the second choice by doing 
nothing at all.  Instead I think that the text should state that the 
publisher MUST set the "incomplete-update" flag, or suspend the 
subscription, or both.

Thanks,
Rob

---

From: Alexander Clemm <[email-masked]>
Date: Wed, 5 Sep 2018 22:28:15 +0000

Hello Rob,

thank you for your comments!  Please find my replies inline, <ALEX>, with corresponding changes to be incorporated into the next revision (-19).

--- Alex

> -----Original Message-----
> From: Netconf [mailto:[email-masked]] On Behalf Of Robert Wilton
> Sent: Tuesday, September 04, 2018 7:37 AM
> To: [email-masked]
> Subject: [Netconf] RGW YANG push -18 LC comments [part 1]
> 
> Hi,
> 
> I've reviewed the -18 YANG push draft up to section 4 so far . I've included my
> comments below.  I think that they are all minor/cosmetic except for the issue
> regarding the dampened subscriptions which has been discussed below and
> probably does not need to be rehashed again.
> 
> In the Abstract:
> (1) Is "continuous" the right choice of word here.  E.g. I thought that sometimes
> the stream of updates is not continuous.  Possibly remove "continuous"?
> 

<ALEX>Would prefer to keep, unless you feel very strongly about this?  What we want to bring across is that this is different from a one-time, on-demand type of update.  "Stream" might imply this already, but this way we would emphasize this a bit further.  
</ALEX>

> 
> In the Introduction:
> (2) "remote visibility" in the first sentence is a bit obscure. Perhaps replace the
> first sentence with "Traditional approaches to monitoring management data on
> remote devices have been built upon polling".

<ALEX> How about changing this to: "Traditional approaches to providing visibility into managed entities from remote have been built on polling. "
[I understand your point, but reason for alternative phrasing is that I don't think we want to restrict this to monitoring (because (a) you can use this also to mirror state for other purposes, e.g. to assess specifics of what needs to be done to provision a service successfully, for troubleshooting, maintenance, validation, etc, and (b) monitoring does include other aspects such as alarm management).  LIkewise, suggest to refer to "managed entities" (not "devices") to not preclude application to things that are not devices (e.g. virtual "things"). ] 
</ALEX> 

> 
> (3) The work isn't actually fruitless if polling is the only available option, hence
> perhaps change:
> 
> OLD:
>    and applications, particularly if the monitored values are only changing
> infrequently."
> 

<ALEX. The "ultimately fruitless" also refers to the fact that many polls will not yield new information (other than "no new information"), as you might e.g. retrieve a lot of stuff to see if it changed only to find that it didn't.  
How about: " For applications that monitor for changes, many remote polling cycles place unwanted and ultimately fruitless load on the network, devices, and applications, particularly when changes occur only infrequently. "
</ALEX>

> (4) In paragraph starting " A more effective alternative", possibly suggest
> changing "publisher" to "server" since the introduction is before the
> definitions/acronyms where "publisher" is formally referenced.
> 
<ALEX> Rewording to "... that enables the server (also referred to as publisher) to push  ... " </ALEX>
> 
> In definitions:
> 
> (5) Suggest tying this definition more closely to the terminology already defined
> in RFC 8342, hence:
> 
> OLD:
>     o  Datastore node: A node in the instantiated YANG data tree associated with
> a datastore.  In this document, datastore nodes are often referred to as "YANG
> objects" or "objects"

<ALEX> OK.  (and removed instance of "YANG object" from the document, as there were only two.) </ALEX>

> 
> (6) "Datastore node update" definition: Does this update only contain the value,
> or does it also contain the path to the datastore node as well?  If this also
> contains the path then it would be good if that was explicit in the definition.
> 

<ALEX> Updated as follows: " A data item containing the current value of a datastore node at the time the datastore node update was created, as well as the path to the datastore node."  </ALEX>

> "updates" in the definitions, do these all refer to "update records"? If so then I
> would suggest moving the definition of "update record" above "datastore
> subscription".
> 
> OLD:
> 
> "Datastore subscription: A subscription to a stream of datastore node updates."

<ALEX> ok, changed </ALEX>

> 
> (8) Suggest removing "instantiated" from the following definition, and using
> descendant (which is used in 7950) instead of contained .. I also suggest moving
> the definition up, directly below "datastore node":
> 
> OLD:
> 
>     o Datastore subtree: A  datastore node and all its descendant datastore
> nodes."
> 

<ALEX> changed </ALEX>

> "update records" that are pushed, or even "Changes to YANG objects"

<ALEX> Changed to "datastore node updates" </ALEX>
> (10) Perhaps change"subtrees within a datastore" to "datastore subtrees".

<ALEX> changed </ALEX>

> describe anchor time in more detail in this section?

<ALEX> Roughly what it is is explained in the same sentence, but how about changing it as follows:
" These parameters are the period interval with which to report updates, and an "anchor time", i.e. a reference point in time that can be used to calculate at which points in time periodic updates need to be assembled and sent. "
</ALEX>

> would receive the same update records.

<ALEX> Yes.  Removed "and sent to a single receiver".  Also clarified further (in a slight rewording) " This means that when there is a change to one or more subscribed objects, an update record containing those objects is created immediately (when no dampening period is in effect) or at the end of a dampening period (when a dampening period is in fact in effect).  "
</ALEX>

> true [I've not checked the YANG model]

<ALEX> it's not defined as a boolean but as a leaf of type empty.  (So, the "default" is in fact true, unless  no-sync-on-start is present).  I don't see a compelling reason to change this and suggest to keep this as-is.  
</ALEX>
> deleted.  I.e. starting the paragraph "In order to minimize ..."

<ALEX> Changed "at" to "of", however, prefer to keep the first sentence as it does provide some explanation.  </ALEX>

>     objects are excluded from update records, even if their values change.
<ALEX> changed </ALEX>

>     are current at the time the dampening period expires."

<ALEX> Rephrase the paragraph as follows, just to make it clear (it's a bit more verbose than it has to, but we want to make sure there is no question as to what value to send - the value of the original change or the value at the time of updating).  
"... .  Values sent at the end of the dampening period are the values that are current at the end of the dampening period of all changed objects.  Changed objects include those which were deleted or newly created during that dampening period.  If an object has returned to its original value (or even has been created and then deleted) during the dampening-period, that value (and not the interim change) will still be sent. ..."
</ALEX> 

> down with further discussion on this.

<ALEX> No change made </ALEX> 

> to those that the client has access rights to.

<ALEX>  I added " (and filter out any nodes for which this is not the case)" to make this clearer; this now reads " Just before a change, or at the start of a dampening period, evaluate any filtering and any access control rules to ensure receiver is authorized to view all subscribed datastore nodes (and filter out any nodes for which this is not the case). "
</ALEX>

> reference would be useful here.

<ALEX> I think this is defined in detail in 3.9, as you note.  I think the spirit of the description here is clear, and I don't want to overly burden the text with cross-references within the same text.  So, I suggest keeping this as-is.  
</ALEX>
> (20) Section 3.3, last paragraph, "can be created" => "must be created".

<ALEX> Actually, I think it is "can".  It is up to the discretion of what the subscriber wants to do.   As an alternative to using the word "can" (not sure if this is really needed), rephrasing it to " In cases where a subscriber wants to have separate dampening periods for different objects, the subscriber has the option to create multiple subscriptions with different selection filters. " </ALEX>
>     update respectively push-change-update notification ..."

<ALEX> I think it does scan, but to make it just slightly more clear, how about updating to " This includes indicating the fact that an update is incomplete as part of a push-update (respectively push-change-update) notification, as well as emitting a subscription-suspended notification as applicable. "
</ALEX>

> (22) Section 3.4, 3rd paragraph, "able fulfill" => "able to fulfill"

<ALEX> changed </ALEX>

> objects only" => "not only restricted to configuration objects".

<ALEX> changing to " not restricted to only configuration objects"  (the "only" refers to the configuration objects, not to the fact that it is restricted) </ALEX>

> entry in a user-ordered list is moved.

<ALEX> OK (I don't feel strongly about this) </ALEX>

> inconsistencies between client and server (which I appreciate that you

<ALEX> Hmm.  While it may be possible to replace "delete" with "remove", I am not so sure about the "merge" part.  At the end of the day, the use here is different from having a client apply a patch operation to the server, and the semantics may be a little clearer by keeping things as they are, so would prefer to keep things as defined now.  
</ALEX>  

> filter replaces any existing filter. "

<ALEX> OK, changing per your suggestion to make it clear that the existing filter is replaced.  </ALEX>

> datastore node" to  "includes a datastore node".

<ALEX> removed the sentence and shortened the paragraph.  It now reads as follows: 
" XPath itself provides powerful filtering constructs and care must be used in filter definition. Consider an XPath filter which only passes a datastore node when an interface is up. It is up to the receiver to understand implications of the presence or absence of objects in each update. "
</ALEX>

> changes then does that affect which nodes are returned?

<ALEX> Not sure what changes you are looking for.  The filter is in effect throughout the subscription until it is changed.  It is up to an implementation to decide whether to apply it once when the subscription is established to determine the datastore nodes (tracking any subtree changes to make sure nodes in the subtrees are included as needed), or whether to apply it at each update anew.    
As far as receiver authorization is concerned (i.e. access control filters), this is explained in section 3.9.  Applying no text changes at this point unless you can be more clear on what you would like to see?
</ALEX>

>     has not been deleted, the object's most current value is reported.

<ALEX> Well, the text states clearly that this concerns any changes since the last notification message.  So, the fact that dampening is enabled is already implied. 
How about rephrasing slightly as follows:
" In cases where multiple changes have occurred over the course of a dampening period and the object has not been deleted, the object's most current value is reported.  "
</ALEX>

> (30) Section 3.7, paragraph 5, "Ethernet port" => "Ethernet interface"

<ALEX> changed </ALEX>

> bound to int32 max rather than uint32 max to make life easier in Java

<ALEX> OK, got it.  Updated the text to reflect start at 0.  
</ALEX> 

> needing to send more fields back to the client.

<ALEX> Would prefer to keep this to the basics, i.e. as-is.  As mentioned, you can compute a rate from this.  </ALEX>

> (33) Section 3.9, 2nd paragraph, "particular each" => "particular"

<ALEX> changed </ALEX>

>     notifications will be sent until some data becomes visible for a

<ALEX> Modified the paragraph already due to another comment in another thread.  Updated this further per your comments.  Now it reads as follows:
" A publisher MUST allow for the possibility that a subscription's selection filter references non-existent data or data that a receiver is not allowed to access.  Such support permits a receiver the ability to monitor the entire lifecyle of some datastore tree without needing to explicitly enumerate every individual datastore node.  If, after access control has been applied, there are no objects remaining in an update record, then (in case of a periodic subscription) only a single empty "push-update" notification MAY be sent. Empty "push-change-update" messages (in case of an on-change subscription) MUST NOT be sent.  This is required to ensure that clients cannot surreptitiously monitor objects that they do not have access to via carefully crafted selection filters."

Also adding the following sentence, to clarify the effect on dampening: " By the same token, changes to objects that are filtered MUST NOT affect any dampening intervals."
</ALEX>

> text in section 3.3, so perhaps section 3.10 would be better moved to

<ALEX>
Don't want to apply major editorial restructuing at this point.
</ALEX>

> (36) Section 3.10, last paragraph.  "will be expected to" => "SHOULD"?

<ALEX>Not sure we want it to sound so normative, but sure - changing it to SHOULD. </ALEX>

> data to be able to set the incomplete-update flag if necessary.

<ALEX> OK, so you are suggesting to move the "incomplete-update" leaf behind the datastore contents (respectively datastore-changes).   Sure, will change that. 
</ALEX>  

> publisher MUST set the "incomplete-update" flag, or suspend the

<ALEX>
OK, I think the problem is with "MUST take on or more of the following actions"  (although I think what is meant is clear).  Rephrasing this as follows:  " In this case, the publisher MUST act as follows."

Also adding the following to the second bullet item: 
" If the publisher does not create an update record at all, it MUST suspend the subscription.  " 
 (This is really the only logical choice; we don't want a publisher to have the option of staying entirely silent.)  

--- Alex
</ALEX>

> Thanks,
> Rob
> 
> _______________________________________________
> Netconf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netconf

---

From: Robert Wilton <[email-masked]>
Date: Thu, 6 Sep 2018 11:56:38 +0100

Hi Alex,

Please see inline ...

> <ALEX>Would prefer to keep, unless you feel very strongly about this?  What we want to bring across is that this is different from a one-time, on-demand type of update.  "Stream" might imply this a…
Keeping is fine if that is what you prefer.

> [I understand your point, but reason for alternative phrasing is that I don't think we want to restrict this to monitoring (because (a) you can use this also to mirror state for other purposes, e.g…
Your phrasing is fine.

> How about: " For applications that monitor for changes, many remote polling cycles place unwanted and ultimately fruitless load on the network, devices, and applications, particularly when changes…
My point is that if the only choice is polling then it is not fruitless 
load, because it serves a useful purpose, that purpose being detecting 
changes in the monitored data.  Hence the load is "unwanted" not 
"fruitless".

But this is a minor consideration, so I'm OK with whatever you decide 
here. :-)

> <ALEX> Rewording to "... that enables the server (also referred to as publisher) to push  ... " </ALEX>
OK.

> <ALEX> OK.  (and removed instance of "YANG object" from the document, as there were only two.) </ALEX>
OK.

> <ALEX> Updated as follows: " A data item containing the current value of a datastore node at the time the datastore node update was created, as well as the path to the datastore node."  </ALEX>
Looks good.

> " These parameters are the period interval with which to report updates, and an "anchor time", i.e. a reference point in time that can be used to calculate at which points in time periodic updates need to be assembled and sent."
> </ALEX>
I think I get this, but didn't find it completely clear until I had 
looked at the model.  I think that you updated text is OK.

Although I have a slight concern (e.g. in section 4.2),

       *  an "anchor-time"; update intervals always fall on the points in
          time that are a multiple of a "period" from an "anchor-time".

I can see implementations will try and keep to this interval, but I 
suspect that the vast majority of implementations will not be able to 
guarantee that it is exact multiples of the given period, given that 
"anchor-time" seems to take an arbitrary sub-section precision, and 
timeticks are in hundreds of a second.  Possibly, weakening the text 
(e.g. as per below, but the YANG model would probably need to be 
updated) might be a good idea?

       *  an "anchor-time"; update intervals always fall as close as possible to the points in
          time that are a multiple of a "period" from an "anchor-time".

> <ALEX> Yes.  Removed "and sent to a single receiver".  Also clarified further (in a slight rewording) " This means that when there is a change to one or more subscribed objects, an update record co…
OK.

> <ALEX> it's not defined as a boolean but as a leaf of type empty.  (So, the "default" is in fact true, unless  no-sync-on-start is present).  I don't see a compelling reason to change this and sugg…
OK.  I think that in YANG, particularly considering NMDA, it is cleaner 
to model this as a boolean with default true, particularly if you do a 
get-data request on the operational state, since it more clearly and 
explicitly indicates whether sync-on-start is enabled.

With the current model, on a get-data request against operation, the 
server returns "no-synch-on-start", it is obvious that it won't perform 
a sync-on-start, however, the reverse if the leaf isn't present isn't 
quite so clear.  If it doesn't return "no-synch-on-start" then that 
either means the device will perform sync-on-start, or perhaps the node 
the server isn't returning any information, or perhaps it has been 
deviated.  So, in summary, I think that flags that turn something off, 
are better modeled with default true, rather than as empty leafs.  Flags 
that turn something on can be modeled equally well either way.

BTW, I also notice that is uses "synch" in various leaf name, but I 
would recommend the abbreviation "sync" instead.  It is shorter, and I 
think more commonly used. 
https://english.stackexchange.com/questions/681/synced-or-synched

> <ALEX> Changed "at" to "of", however, prefer to keep the first sentence as it does provide some explanation.  </ALEX>
OK.

> "... .  Values sent at the end of the dampening period are the values that are current at the end of the dampening period of all changed objects.  Changed objects include those which were deleted o…
Yes, I think that the longer and more explicit text is better.

> <ALEX>  I added " (and filter out any nodes for which this is not the case)" to make this clearer; this now reads " Just before a change, or at the start of a dampening period, evaluate any filteri…
OK, but I would change "and filter" to "filtering".

> <ALEX> I think this is defined in detail in 3.9, as you note.  I think the spirit of the description here is clear, and I don't want to overly burden the text with cross-references within the same…
OK.

> <ALEX> Actually, I think it is "can".  It is up to the discretion of what the subscriber wants to do.   As an alternative to using the word "can" (not sure if this is really needed), rephrasing it…
must is correct, since that is the only available solution to the client 
if they want different dampening periods.  I.e. the clients choice is: 
don't have separate dampening periods or create separate subscriptions.  
But this is a minor point, not worth lengthy discussion, so you can 
decide what to do.

> <ALEX> I think it does scan, but to make it just slightly more clear, how about updating to " This includes indicating the fact that an update is incomplete as part of a push-update (respectively p…
It is the "push-update respectively push-change-update" that doesn't 
make any sense to me.  Do you just mean "or", e.g. "push-update or 
push-change-update"?
> <ALEX> changing to " not restricted to only configuration objects"  (the "only" refers to the configuration objects, not to the fact that it is restricted) </ALEX>
Yes., OK.
> <ALEX> Hmm.  While it may be possible to replace "delete" with "remove", I am not so sure about the "merge" part.  At the end of the day, the use here is different from having a client apply a patc…
I think that is probably right.

I think that it is a slight shame that vanilla YANG patch won't work 
here, but as you say I think that it will need to be applied in a 
slightly different way regardless.

> " XPath itself provides powerful filtering constructs and care must be used in filter definition. Consider an XPath filter which only passes a datastore node when an interface is up. It is up to the receiver to understand implications of the presence or absence of objects in each update."
> </ALEX>
OK.

> As far as receiver authorization is concerned (i.e. access control filters), this is explained in section 3.9.  Applying no text changes at this point unless you can be more clear on what you would…
OK.  Not considering NACM, I think that is already covered, was thinking 
of an Xpath filter.  I am right in thinking that logically the Xpath 
filter is [re-]applied whenever the periodic push notification is 
constructed.

If so, possibly change:

" When the set of selection filtering criteria is applied for a
    periodic subscription, all selected datastore nodes to which a
    receiver has access are provided to that receiver."

To something along the lines of ...

" When the set of selection filtering criteria is applied for a
   periodic subscription, then they are applied whenever a periodic
   update record is constructed, and only datastore nodes that pass
   the filter and to which areceiver has access are provided to
   that receiver."

> " In cases where multiple changes have occurred over the course of a dampening period and the object has not been deleted, the object's most current value is reported."
> </ALEX>
This text is fine.

But this raises another question:

If no dampening is enabled, then is it expected that all changes to an 
object are always notified.  [If this is a requirement, then I think 
that it should probably be stated clearly in the YANG model and perhaps 
in section 3 as well.]

Specifically, say you are monitoring interface state, without dampening, 
and it starts to flap 100 times per second.  Is the publisher required 
to notify of every single change.  Or is the publisher only required to 
notify that the at least one change has occurred, and guarantee that it 
always notifies the latest value.

> <ALEX> Would prefer to keep this to the basics, i.e. as-is.  As mentioned, you can compute a rate from this.  </ALEX>
Yes, that is fine.  Additional fields could be augmented in future if 
needed.

> Also adding the following sentence, to clarify the effect on dampening: " By the same token, changes to objects that are filtered MUST NOT affect any dampening intervals."
Text looks good.  My only question is on the sentence with the MAY. I've 
been told by some old hands that everywhere you see "MAY" in an RFC then 
it is entirely valid to also interpret that as "MAY NOT".

I think that it should perhaps be:

"... then (in case of a periodic subscription) the publisher MUST only 
send single empty "push-update" notification.

or, alternatively (if you don't want MUST):

"... then (in case of a periodic subscription) the publisher is allowed 
to send only a single empty "push-update" notification.

> Don't want to apply major editorial restructuing at this point.
That is fine.

> <ALEX> OK, so you are suggesting to move the "incomplete-update" leaf behind the datastore contents (respectively datastore-changes).   Sure, will change that.
Yes, thanks.

>   (This is really the only logical choice; we don't want a publisher to have the option of staying entirely silent.)
Yes, the change looks good.

Thanks,
Rob

---

From: Alexander Clemm <[email-masked]>
Date: Thu, 6 Sep 2018 19:35:56 +0000

Hi Rob,

thank you for your responses - replies to those in <ALEX-2>

Thanks!
--- Alex

From: Robert Wilton [mailto:[email-masked]]
Sent: Thursday, September 06, 2018 3:57 AM
To: Alexander Clemm <[email-masked]>; [email-masked]
Subject: Re: [Netconf] RGW YANG push -18 LC comments [part 1]

Hi Alex,

Please see inline ...

Hello Rob,

--- Alex

-----Original Message-----

To: [email-masked]<mailto:[email-masked]>

Hi,

In the Abstract:

</ALEX>

<ALEX-2> Thanks </ALEX-2>

In the Introduction:

</ALEX>

Your phrasing is fine.

<ALEX-2> Thanks </ALEX-2>

perhaps change:

OLD:

  and applications."<NEW: >

 <NEW: >

NEW:<NEW: >

   "<NEW: >For applications that monitor for changes, many remote polling

infrequently."

</ALEX>

My point is that if the only choice is polling then it is not fruitless load, because it serves a useful purpose, that purpose being detecting changes in the monitored data.  Hence the load is "unwanted" not "fruitless".

But this is a minor consideration, so I'm OK with whatever you decide here. :-)

<ALEX-2> Let’s change “fruitless” to “wasteful” </ALEX-2>

In definitions:

in RFC 8342, hence:

OLD:

NEW:

objects" or "objects"

subscription".

OLD:

       contents of a datastore."<NEW:>

 <NEW:>

NEW:

 <NEW:>

"<NEW:>Datastore subscription: A subscription to a stream of datastore node updates."

OLD:

NEW:

nodes."

<ALEX> changed </ALEX>

<ALEX> changed </ALEX>

</ALEX>
I think I get this, but didn't find it completely clear until I had looked at the model.  I think that you updated text is OK.

I can see implementations will try and keep to this interval, but I suspect that the vast majority of implementations will not be able to guarantee that it is exact multiples of the given period, given that "anchor-time" seems to take an arbitrary sub-section precision, and timeticks are in hundreds of a second.  Possibly, weakening the text (e.g. as per below, but the YANG model would probably need to be updated) might be a good idea?

<ALEX-2>I don’t like the “as close as possible”.  What is possible may differ between implementations; I feel that this formulation blurs more than it clarifies.  Perhaps we should simply remove the “always” – to read

“update intervals fall on the points in time that are a multiple of a "period" from an "anchor-time" </ALEX-2>

</ALEX>
OK.

</ALEX>

OK.  I think that in YANG, particularly considering NMDA, it is cleaner to model this as a boolean with default true, particularly if you do a get-data request on the operational state, since it more clearly and explicitly indicates whether sync-on-start is enabled.

With the current model, on a get-data request against operation, the server returns "no-synch-on-start", it is obvious that it won't perform a sync-on-start, however, the reverse if the leaf isn't present isn't quite so clear.  If it doesn't return "no-synch-on-start" then that either means the device will perform sync-on-start, or perhaps the node the server isn't returning any information, or perhaps it has been deviated.  So, in summary, I think that flags that turn something off, are better modeled with default true, rather than as empty leafs.  Flags that turn something on can be modeled equally well either way.

<ALEX-2>In terms of instantiation, I find empty leaves more elegant, particularly if there is a default value.  I think <no-sync-on-start/> is more elegant than <no-sync-on-start>True</no-sync-on-start>.

Would you be satisfied if we turn the default around, i.e. change “no-sync-on-start” to “sync-on-start”, but keeping it an empty?

 </ALEX-2>

BTW, I also notice that is uses "synch" in various leaf name, but I would recommend the abbreviation "sync" instead.  It is shorter, and I think more commonly used.  https://english.stackexchange.com/questions/681/synced-or-synched

<ALEX-2>Have changed “synch” (and “resynch”) to sync / resync througout the documentt, also per Rohit’s earlier comment.

</ALEX-2>

OLD:

    actually change.

NEW:

<ALEX> changed </ALEX>

</ALEX>

</ALEX>

<ALEX-2>OK, changed </ALEX-2>

</ALEX>
OK.

must is correct, since that is the only available solution to the client if they want different dampening periods.  I.e. the clients choice is: don't have separate dampening periods or create separate subscriptions.  But this is a minor point, not worth lengthy discussion, so you can decide what to do.

<ALEX-2>OK, keeping it.</ALEX-2>

</ALEX>
It is the "push-update respectively push-change-update" that doesn't make any sense to me.  Do you just mean "or", e.g. "push-update or push-change-update"?

<ALEX-2>OK, changing it to “or” (I guess my nativeness to the German language shines through here, in which there is a finer distinction between two types of “or”, one of which is the term “beziehungsweise” (which would be the proper choice here – yes, some words in German are longer than in English, even the abbreviation “bzw.” is still longer than “or”;-), which translates roughly but perhaps not exactly into “repectively”.)</ALEX-2>

<ALEX> changed </ALEX>

OLD:

    "move" operation.

NEW:

try and avoid).

</ALEX>

I think that it is a slight shame that vanilla YANG patch won't work here, but as you say I think that it will need to be applied in a slightly different way regardless.

<ALEX-2>OK. Keeping it.</ALEX-2>

</ALEX>
OK.

</ALEX>
OK.  Not considering NACM, I think that is already covered, was thinking of an Xpath filter.  I am right in thinking that logically the Xpath filter is [re-]applied whenever the periodic push notification is constructed.

If so, possibly change:

"   When the set of selection filtering criteria is applied for a
  periodic subscription, then they are applied whenever a periodic
  update record is constructed, and only datastore nodes that pass
  the filter and to which a receiver has access are provided to
  that receiver."

<ALEX-2>OK, I am making the change as suggested.  (However, presumably an implementation could choose to apply the filter to select the set of datastore nodes once, and then, assuming the filter and set of datastore nodes don’t change, simply send updates about the nodes form the set, no?) </ALEX-2>

OLD:

NEW:

</ALEX>
This text is fine.

If no dampening is enabled, then is it expected that all changes to an object are always notified.  [If this is a requirement, then I think that it should probably be stated clearly in the YANG model and perhaps in section 3 as well.]

Specifically, say you are monitoring interface state, without dampening, and it starts to flap 100 times per second.  Is the publisher required to notify of every single change.  Or is the publisher only required to notify that the at least one change has occurred, and guarantee that it always notifies the latest value.

<ALEX-2> This is exactly the reason why dampening was introduced.  I think this is well documented.  If a subscriber does not turn on dampening, and thus the server gets overwhelmed, they can suspend or terminate the subscription.  In that case they should give a “period-unsupported” error.
Thinking about it some more, this needs to be more clearly spelled out in the description of “period-unsupported”.

Changing the YANG definition (descriptino statement) as follows:
  identity period-unsupported {
    base sn:establish-subscription-error;
    base sn:modify-subscription-error;
    base sn:subscription-suspended-reason;
    description
      "Requested time period or dampening-period is too short. This
      can be for both periodic and on-change subscriptions (with or
      without dampening.) Hints suggesting alternative periods may
      be returned as supplemental information.";
  }
</ALEX-2>

<ALEX> changed </ALEX>

land ...

</ALEX>

<ALEX> Would prefer to keep this to the basics, i.e. as-is.  As mentioned, you can compute a rate from this.  </ALEX>
Yes, that is fine.  Additional fields could be augmented in future if needed.

<ALEX> changed </ALEX>

    receiver."

</ALEX>
Text looks good.  My only question is on the sentence with the MAY. I've been told by some old hands that everywhere you see "MAY" in an RFC then it is entirely valid to also interpret that as "MAY NOT".

"... then (in case of a periodic subscription) the publisher MUST only send single empty "push-update" notification.

"... then (in case of a periodic subscription) the publisher is allowed to send only a single empty "push-update" notification.

<ALEX-2>
I don’t see how “allowed to” really changes the meaning of “MAY”, so I am taking your suggestion and am replacing the MAY with MUST.
</ALEX-2>

become section 3.3.1?

<ALEX>

</ALEX>
That is fine.

</ALEX>
Yes, thanks.

subscription, or both.

<ALEX>

Thanks,
Rob
<ALEX-2>
Thanks, Rob
--- Alex
</ALEX-2>

--- Alex

</ALEX>

Thanks,

Rob

Netconf mailing list

[email-masked]<mailto:[email-masked]>

---

From: Robert Wilton <[email-masked]>
Date: Fri, 7 Sep 2018 14:46:15 +0100

Hi Alex,

I'm marked my replies RW, in case the indentation isn't working.

On 06/09/2018 20:35, Alexander Clemm wrote:
> *From:*Robert Wilton [mailto:[email-masked]]
> *Sent:* Thursday, September 06, 2018 3:57 AM
> *To:* Alexander Clemm <[email-masked]>; [email-masked]
> *Subject:* Re: [Netconf] RGW YANG push -18 LC comments [part 1]
>
> Hi Alex,
>
> Please see inline ...
>
>         To:[email-masked] <mailto:[email-masked]>
>
>            and applications."<NEW:%C2%A0>
>
>         <NEW:%C2%A0>
>
>         NEW:<NEW:%C2%A0>
>
>           " <NEW:%C2%A0>For applications that monitor for changes, many remote polling
>
>         " <NEW:>Datastore subscription: A subscription to a stream of datastore node updates."
>
> <ALEX-2>I don’t like the “as close as possible”.  What is possible may 
> differ between implementations; I feel that this formulation blurs 
> more than it clarifies.  Perhaps we should simply remove the “always” 
> – to read
RW:
Yes, that is probably OK, the alternative could be "SHOULD fall", but 
even that is probably a bit strong.

What I want to avoid is clients assuming that they will receive updates 
exactly period interval apart, e.g. when calculating rates. Instead, 
they should use the timestamps of the previous and current notification 
to determine the time interval to use when calculating any rates.

> <ALEX-2>In terms of instantiation, I find empty leaves more elegant, 
> particularly if there is a default value.  I think <no-sync-on-start/> 
> is more elegant than <no-sync-on-start>True</no-sync-on-start>.
> Would you be satisfied if we turn the default around, i.e. change 
> “no-sync-on-start” to “sync-on-start”, but keeping it an empty?
>  </ALEX-2>
RW: Yes, changing it to "sync-on-start" keeping the type as empty is fine.

>
> BTW, I also notice that is uses "synch" in various leaf name, but I would recommend the abbreviation "sync" instead.  It is shorter, and I think more commonly used.https://english.stackexchange.com/questions/681/synced-or-synched
>   
>
> <ALEX-2>Have changed “synch” (and “resynch”) to sync / resync 
> througout the documentt, also per Rohit’s earlier comment.
> </ALEX-2>
RW: OK.

> <ALEX-2>OK, keeping it.</ALEX-2>
RW: OK.

>     <ALEX-2>OK, changing it to “or” (I guess my nativeness to the
>     German language shines through here, in which there is a finer
>     distinction between two types of “or”, one of which is the term
>     “beziehungsweise” (which would be the proper choice here – yes,
>     some words in German are longer than in English, even the
>     abbreviation “bzw.” is still longer than “or”;-), which translates
>     roughly but perhaps not exactly into “repectively”.)</ALEX-2>
>
RW: Thanks.

> OK.  Not considering NACM, I think that is already covered, was 
> thinking of an Xpath filter.  I am right in thinking that logically 
> the Xpath filter is [re-]applied whenever the periodic push 
> notification is constructed.
>
> If so, possibly change:
>
>     <ALEX-2>OK, I am making the change as suggested.  (However,
>     presumably an implementation could choose to apply the filter to
>     select the set of datastore nodes once, and then, assuming the
>     filter and set of datastore nodes don’t change, simply send
>     updates about the nodes form the set, no?) </ALEX-2>
>
RW:
If the set of filtered nodes is that same at each periodic update then 
it is equivalent to applying the filter once.

I was more thinking of an XPath example (e.g. inclusion of a node in the 
nodeset depends on whether an interface is up).  Here, the filter 
logically needs to be re-calculated at each periodic interface to ensure 
that the right nodes are returned.

> that it should probably be stated clearly in the YANG model and 
> perhaps in section 3 as well.]
>
> Specifically, say you are monitoring interface state, without 
> dampening, and it starts to flap 100 times per second.  Is the 
> publisher required to notify of every single change.  Or is the 
> publisher only required to notify that the at least one change has 
> occurred, and guarantee that it always notifies the latest value.
>
> <ALEX-2> This is exactly the reason why dampening was introduced. I 
> think this is well documented.  If a subscriber does not turn on 
> dampening, and thus the server gets overwhelmed, they can suspend or 
> terminate the subscription.  In that case they should give a 
> “period-unsupported” error.
>
> Thinking about it some more, this needs to be more clearly spelled out 
> in the description of “period-unsupported”.
>
RW:
OK, but there are other ways of solving this that may be more 
appropriate for some objects like interface states.

E.g. it is quite possible that I can't have a single dampened 
subscription for all interface states because I don't want a down event 
on one interface to prevent fast down notification of another interface 
in the same subscription.  Nor does having a separate subscription for 
each of (10,000+ interfaces appeal).

For interface states, the proper solution is to use a feature like 
carrier delay that limits flaps in a sensible way (fast down, slow up), 
and that is done on a per interface basis outside of YANG. I.e. the 
stream of changes is limited enough that dampening is not required.

I think that a future draft (or drafts) may want to look at other forms 
of dampening.

But for this draft, I still think that it needs to be clear for a 
subscription without dampening if the server MUST notify of *every* change.

> Text looks good.  My only question is on the sentence with the MAY. 
> I've been told by some old hands that everywhere you see "MAY" in an 
> RFC then it is entirely valid to also interpret that as "MAY NOT".
>
> "... then (in case of a periodic subscription) the publisher is 
> allowed to send only a single empty "push-update" notification.
>
>
> <ALEX-2>
>
> I don’t see how “allowed to” really changes the meaning of “MAY”, so I 
> am taking your suggestion and am replacing the MAY with MUST.
>
> </ALEX-2>
>
RW: OK

Thanks,
Rob

>         [email-masked] <mailto:[email-masked]>
>

---

From: Martin Bjorklund <[email-masked]>
Date: Mon, 10 Sep 2018 11:48:58 +0200 (CEST)

Hi,

I have one comment on this thread.

> On 06/09/2018 20:35, Alexander Clemm wrote:

[...]

> >     <ALEX> it's not defined as a boolean but as a leaf of type empty. 
> >     (So, the "default" is in fact true, unless  no-sync-on-start is
> >     present).  I don't see a compelling reason to change this and suggest
> >     to keep this as-is.
> >
> >     </ALEX>
> >
> > OK.  I think that in YANG, particularly considering NMDA, it is
> > cleaner to model this as a boolean with default true, particularly if
> > you do a get-data request on the operational state, since it more
> > clearly and explicitly indicates whether sync-on-start is enabled.
> >
> > server returns "no-synch-on-start", it is obvious that it won't
> > perform a sync-on-start, however, the reverse if the leaf isn't
> > present isn't quite so clear.  If it doesn't return
> > "no-synch-on-start" then that either means the device will perform
> > sync-on-start, or perhaps the node the server isn't returning any
> > information, or perhaps it has been deviated.  So, in summary, I think
> > that flags that turn something off, are better modeled with default
> > true, rather than as empty leafs.  Flags that turn something on can be
> > modeled equally well either way.

But this would change the default behaviour that the WG has accepted.
I don't think this is a good change.  I agree with Robert's initial
comment that this should be modeled as a leaf 'sync-on-start' of type
boolean, with default true.  I prefer the better model before a more
elegant encoding.

/martin