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

[Robert Wilton <[e-mail]>]

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