Re: [netmod] AD review of draft-ietf-netmod-nmda-diff-07

[Andy Bierman <[e-mail]>]

On Thu, Oct 29, 2020 at 6:09 PM joel jaeggli <[e-mail]> wrote:

> Rob,
>
> These seem like reasonable suggestions.
>
> Lets see what the authors say.
>
> Thanks for this
> joel
>
> On Thu, Oct 29, 2020 at 6:47 AM Rob Wilton (rwilton) <[e-mail]>
> wrote:
>
>> Hi,
>>
>> Here is my AD review for draft-ietf-netmod-nmda-diff-07.  Apologies for
>> the delay.
>>
>> Thank you for writing this document, I think that it is useful, and looks
>> like it is in good shape.
>>
>>
>> Main comments:
>>
>> 1. Should there be any text about how to find out what datastores are
>> supported by a device?  E.g., pointing them to either YANG library, or
>> protocol specific mechanisms in the case of RESTCONF.
>>
>>
Do you have a section in mind and suggested text?



> 2. It might be helpful to add a comment about potential issues that could
>> arise by comparing <running> to <operational>, i.e., additional differences
>> could be reported due to inactive configuration and template processing
>> between <running> and <operational>.
>>
>>
Do you have a section in mind and suggested text?
You mean if there are differences between <running> and <intended>
then a diff between <running> and <operational> will not be the same
as a diff between <intended> and <operational>.?


3. I would prefer if 'exclude=origin' was in the reverse sense and perhaps
>> called 'report-origin' instead.  With the reverse sense it seems to be
>> safer if new datastores are defined, where otherwise the behaviour could
>> end being under specified.
>>
>

IMO the WG already designed the features so if the functional requirements
have changed
then the draft should go back to the WG for changes and new WG consensus
calls.




>> 4. Should there be an option to filter on origin metadata?  E.g., only
>> include values that come from intended.  Otherwise, things like IP
>> addresses learned from DHCP may always turn up as differences.
>>
>
IMO the WG already designed the features so if the functional requirements
have changedthen the draft should go back to the WG for changes and new WG
consensus calls.



>> 5. I'm not that keen on the "Possible Future Extensions" section of an
>> RFC.  Personally, I would prefer that this section is deleted, but if you
>> wish to retain it, then please can you move it to an appendix.
>>
>>
OK with me to remove it



Andy



>
>> I've also included some minor comments inline below, and some nits at the
>> end:
>>
>>     Abstract
>>
>>        This document defines an RPC operation to compare management
>>        datastores that comply with the NMDA architecture.
>>
>> The abstract is perhaps somewhat terse.  Perhaps:
>>
>>     This document defines a YANG RPC operation to compare the
>>     contents of network management datastores that comply with
>>     the NMDA architecture and return the differences in the
>>     YANG-Patch format.
>>
>>
>>     1.  Introduction
>>
>>        The revised Network Management Datastore Architecture (NMDA)
>>        [RFC8342] introduces a set of new datastores that each hold YANG-
>>        defined data [RFC7950] and represent a different "viewpoint" on the
>>        data that is maintained by a server.  New YANG datastores that are
>>        introduced include <intended>, which contains validated
>> configuration
>>        data that a client application intends to be in effect, and
>>        <operational>, which contains at least conceptually operational
>> state
>>        data (such as statistics) as well as configuration data that is
>>        actually in effect.
>>
>> I would suggest deleting "at least conceptually", since the <operational>
>> datastore does contain all operational state, but it may be implemented
>> as a virtual construct that spans multiple nodes (e.g., linecards) and
>> processes.
>>
>>
>>        NMDA introduces in effect a concept of "lifecycle" for management
>>        data, allowing to clearly distinguish between data that is part of
>> a
>>        configuration that was supplied by a user, configuration data that
>>        has actually been successfully applied and that is part of the
>>        operational state, and overall operational state that includes both
>>        applied configuration data as well as status and statistics.
>>
>> "allowing to clearly distinguish" => distinguishing"
>> "status and statistics" => "status information and statistics"
>>
>>
>>        As a result, data from the same management model can be reflected
>> in
>>        multiple datastores.  Clients need to specify the target datastore
>> to
>>        be specific about which viewpoint of the data they want to access.
>>        This way, an application can differentiate whether they are (for
>>        example) interested in the configuration that has been applied and
>> is
>>        actually in effect, or in the configuration that was supplied by a
>>        client and that is supposed to be in effect.
>>
>> Perhaps reword the last sentence to match the logical data flow in the
>> server:
>>
>>    For example, a client application can differentiate whether they are
>>    interested in the configuration supplied to a server and that is
>>    supposed to be in effect, or the configuration that has been applied
>> and is
>>    actually in effect on the server.
>>
>>
>>        When configuration that is in effect is different from
>> configuration
>>        that was applied, many issues can result.  It becomes more
>> difficult
>>        to operate the network properly due to limited visibility of actual
>>        status which makes it more difficult to analyze and understand what
>>        is going on in the network.  Services may be negatively affected
>> (for
>>        example, breaking a service instance resulting in service is not
>>        properly delivered to a customer) and network resources be
>>        misallocated.
>>
>> Perhaps change "actual status" to "actual operational status".
>>
>> I also suggest changing the last sentence to:
>>
>>     Services may be negatively affected (e.g., degrading or breaking a
>> customer service) or network resources may be misallocated.
>>
>>
>>         3. Definitions:
>>
>> It should probably define that <intended>, <operational>, (and perhaps
>> <running>) are used to indicate names of datastores.
>>
>> It should also explain that <compare> is used as the name of a YANG RPC.
>>
>>
>>     4.  Data Model Overview
>>
>>        At the core of the solution is a new management operation,
>> <compare>,
>>        that allows to compare two datastores for the same data.
>>
>> Suggest rewording this first sentence to:
>>
>>   The core of the solution is a new management operation, <compare>,
>>   that compares the data tree contents of two datastores.
>>
>>        o  target: The target identifies the datastore to compare against
>> the
>>           source.
>>
>> Suggest adding an example ", e.g., <operational>."
>>
>>        o  filter-spec: This is a choice between different filter
>> constructs
>>           to identify the portions of the datastore to be retrieved.  It
>>           acts as a node selector that specifies which data nodes are
>> within
>>           the scope of the comparison and which nodes are outside the
>> scope
>
>