Hi all,
Rob, thank you very much for your AD review! We have just posted a new
revision -08 taking your comments into account. Please find attached
and below my reponses to your comments (inline, delimited <AC>;
apologies for having taken so long).
Thanks
--- Alex
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.
<AC> Reply: Note sure this is needed, and where we would even say it in
the text. Perhaps in the Introduction, where we introduce NMDA, a
sentence of the sort: "To identify which datastores are supported by a
given device, ...".
If we wanted to this, one question is, how is this actually done? RFC
8342 makes no statement about this; it defines identities and typedefs
but no capabilities or data model that would indicate the capabilities.
</AC>
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>.
<AC> Reply: I modified the last sentence in the fourth paragraph in the
Introduction as follows:
"This can be the case due to certain conditions not being met, certain
parts of the configuration not propagating because considered inactive,
resource dependencies not being resolved, or even implementation errors
in corner conditions."
I am not sure I understand the template processing issue; can you please
elaborate?
</AC>
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.
<AC> Reply: Updated it per discussion in the mail thread.
</AC>
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.
<AC> Reply: accepting proposed change per the email discussion
Changed "exclude-origin" to "report-origin", with new description as
follows:
leaf report-origin {
type empty;
description
"When this leaf is provided, origin metadata is
included as part of RPC output. When this leaf is
omitted, origin metadata in comparisons that involve
<operational> is by default omitted.";
}
Analogous change in the data model overview:
"report-origin: When set, this parameter indicates that origin metadata
should be included as part of RPC output. When this parameter is
omitted, origin metadata in comparisons that involve <operational> is by
default omitted."
Updated also the output parameter description of "differences" accordingly:
Previous:
When the target datastore is <operational>, "origin" metadata is
included as part of the patch.
New:
When the target datastore is <operational> and the input parameter
"report-origin" is set, "origin" metadata is included as part of the patch.
Also updated the examples:
New RPC request (NETCONF):
<rpc message-id="101"
xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
<compare xmlns="urn:ietf:params:xml:ns:yang:ietf-nmda-compare"
xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
<source>ds:operational</source>
<target>ds:intended</target>
<report-origin/>
<xpath-filter
xmlns:if="urn:ietf:params:xml:ns:yang:ietf-interfaces">
/if:interfaces
</xpath-filter>
</compare>
</rpc>
(RPC reply remains the same)
New RPC request (RESTCONF):
POST /restconf/operations/ietf-nmda-compare:compare HTTP/1.1
Host: example.com
Content-Type: application/yang-data+json
Accept: application/yang-d
{ "ietf-nmda-compare:input" {
"source" : "ietf-datastores:operational",
"target" : "ietf-datastores:intended",
"report-origin" : null,
"xpath-filter" : "/ietf-interfaces:interfaces"
}
}
(again, RPC reply remains the same)
</AC>
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.
<AC> Moved section 8 to an appendix. If there are objections to that, we
can also delete it entirely, but my preference would be for it to remain
as I think the discussion is useful as it also outlines some practical
considerations.
</AC>
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.
<AC> Changed
</AC>
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"
<AC> Changed
</AC>
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.
<AC> Changed
</AC>
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".
<AC> Changed
</AC>
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.
<AC> Changed
</AC>
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.
<AC> I am not sure we need to define the datastores, as they have
already been defined in RFC 8342. Also, the fact that <compare> is used
as the name of the YANG RPC is evident from the data model and would
feel redundant. Unless you feel strongly it should be added, I would
prefer to keep this as-is.
</AC>
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.
<AC> Changed
</AC>
o target: The target identifies the datastore to compare against the
source.
Suggest adding an example ", e.g., <operational>."
<AC> Changed
</AC>
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.
This allows a comparison operation to be applied only to a
specific portion of the datastore that is of interest, such as a
particular subtree. (The filter dow not contain expressions that
would match values data nodes, as this is not required by most use
cases and would complicate the scheme, from implementation to
dealing with race conditions.)
Perhaps "parts/part" rather than "portions/portion".
<AC> Changed
</AC>
Suggest rewording the last sentence to:
Note, the filter does not allow expressions that match against data
node values since that may incur implementation difficulties and is not
required for normal use cases.
<AC> Changed
</AC>
o ... When the target datastore is <operational>, "origin"
metadata is included as part of the patch. Including origin
metadata can help in some cases explain the cause of a difference,
for example when a data node is part of <intended> but the origin
of the same data node in <operational> is reported as "system".
I think that this test needs to refer back to the 'exclude-origin' or
'report-origin' options since the origin metadata isn't always included.
<AC> Yes, this is updated, per earlier comment.
</AC>
5. YANG Data Model
The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL', 'SHALL
NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED', 'NOT RECOMMENDED',
'MAY', and 'OPTIONAL' in this document are to be interpreted as
described in BCP 14 (RFC 2119) (RFC 8174) when, and only when,
they appear in all capitals, as shown here.
I couldn't see that RFC 2119 language is actually used in the YANG
module, so perhaps this can be deleted?
<AC> I thought this was boilerplate, but I will be happy to delete it -
removed
</AC>
rpc compare {
description
"NMDA compare operation.";
Perhaps "NMDA datastore compare operation."
<AC> Changed
</AC>
anydata source-value {
when "../operation = 'delete'"
+ "or ../operation = 'merge'"
+ "or ../operation = 'move'"
+ "or ../operation = 'replace'"
+ "or ../operation = 'remove'";
description
"The anydata 'value' is only used for 'delete',
'move', 'merge', 'replace', and 'remove'
operations.";
I'm not convinced how useful the when statement really is in this case,
since 'source-value' isn't marked as mandatory, a server is allowed to
omit it when it doesn't apply anyway.
<AC> No change made. The reason why this is specified is that in case
of a "create" operation, it would not make sense to include a source
value. Sure, this is supplied by the server, but still I do think there
is value to retain the statement - there is a client application
somewhere that would be very surprised if a server were to provide a
source value here...
</AC>
6. Example
<operational> does not contain object "description" that is contained
in <intended>. Another object, "enabled", has differences in values,
being "true" in <operational> and "false" in <intended>. A third
object, "name", is the same in both cases. The origin of the objects
in <operational> is "learned", which may help explain the
discrepancies.
I think that we should probably refrain from calling them objects,
perhaps "leaf instance" would be better? Rather than "differences in
values", perhaps "different values".
E.g.,
<operational> does not contain an instance for leaf "description"
that
is contained in <intended>. Another leaf, "enabled", has different
values in the two datastores, being "true" in <operational> and
"false" in <intended>. A third leaf, "name", has the same instance
value in both datastores. The origin of the leaf instances in
<operational> is "learned", which may help explain the discrepancies.
<AC> Changed. Although I think it would be fair to call them objects
also; RFC 8342 talks about objects too. </AC>
//OPERATIONAL
<interfaces
xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces"
xmlns:or="urn:ietf:params:xml:ns:yang:ietf-origin">
<interface or:origin="or:learned">
<name>eth0</name>
<enabled>true</enabled>
</interface>
</interfaces>
There is an extra line, and dodgy indentation for "</interface>".
<AC> Changed, formatted now as follows:
//OPERATIONAL
<interfaces
xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces"
xmlns:or="urn:ietf:params:xml:ns:yang:ietf-origin">
<interface or:origin="or:learned">
<name>eth0</name>
<enabled>true</enabled>
</interface>
</interfaces>
</AC>
Nits:
"possibly for" => "possible for"
"reference for the" => "the reference data tree for the"
"is basis" => "is the basis"
<AC> Changed, except "as reference for the" => "as the reference for
the" (I think data tree would sound a bit odd in the context; also
datastore would perhaps be more appropriate than data tree here).
</AC>
Thanks,
Rob
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.
<AC> Reply: Note sure this is needed, and where we would even say it in the
text. Perhaps in the Introduction, where we introduce NMDA, a sentence of the
sort: "To identify which datastores are supported by a given device, ...".
If we wanted to this, one question is, how is this actually done? RFC 8342
makes no statement about this; it defines identities and typedefs but no
capabilities or data model that would indicate the capabilities.
</AC>
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>.
<AC> Reply: I modified the last sentence in the fourth paragraph in the
Introduction as follows:
"This can be the case due to certain conditions not being met, certain parts of
the configuration not propagating because considered inactive, resource
dependencies not being resolved, or even implementation errors in corner
conditions."
I am not sure I understand the template processing issue; can you please
elaborate?
</AC>
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.
<AC> Reply: Updated it per discussion in the mail thread.
</AC>
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.
<AC> Reply: accepting proposed change per the email discussion
Changed "exclude-origin" to "report-origin", with new description as follows:
leaf report-origin {
type empty;
description
"When this leaf is provided, origin metadata is
included as part of RPC output. When this leaf is
omitted, origin metadata in comparisons that involve
<operational> is by default omitted.";
}
Analogous change in the data model overview:
"report-origin: When set, this parameter indicates that origin metadata should
be included as part of RPC output. When this parameter is omitted, origin
metadata in comparisons that involve <operational> is by default omitted."
Updated also the output parameter description of "differences" accordingly:
Previous:
When the target datastore is <operational>, "origin" metadata is included as
part of the patch.
New:
When the target datastore is <operational> and the input parameter
"report-origin" is set, "origin" metadata is included as part of the patch.
Also updated the examples:
New RPC request (NETCONF):
<rpc message-id="101"
xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
<compare xmlns="urn:ietf:params:xml:ns:yang:ietf-nmda-compare"
xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
<source>ds:operational</source>
<target>ds:intended</target>
<report-origin/>
<xpath-filter
xmlns:if="urn:ietf:params:xml:ns:yang:ietf-interfaces">
/if:interfaces
</xpath-filter>
</compare>
</rpc>
(RPC reply remains the same)
New RPC request (RESTCONF):
POST /restconf/operations/ietf-nmda-compare:compare HTTP/1.1
Host: example.com
Content-Type: application/yang-data+json
Accept: application/yang-d
{ "ietf-nmda-compare:input" {
"source" : "ietf-datastores:operational",
"target" : "ietf-datastores:intended",
"report-origin" : null,
"xpath-filter" : "/ietf-interfaces:interfaces"
}
}
(again, RPC reply remains the same)
</AC>
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.
<AC> Moved section 8 to an appendix. If there are objections to that, we can
also delete it entirely, but my preference would be for it to remain as I think
the discussion is useful as it also outlines some practical considerations.
</AC>
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.
<AC> Changed
</AC>
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"
<AC> Changed
</AC>
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.
<AC> Changed
</AC>
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".
<AC> Changed
</AC>
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.
<AC> Changed
</AC>
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.
<AC> I am not sure we need to define the datastores, as they have already been
defined in RFC 8342. Also, the fact that <compare> is used as the name of the
YANG RPC is evident from the data model and would feel redundant. Unless you
feel strongly it should be added, I would prefer to keep this as-is.
</AC>
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.
<AC> Changed
</AC>
o target: The target identifies the datastore to compare against the
source.
Suggest adding an example ", e.g., <operational>."
<AC> Changed
</AC>
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.
This allows a comparison operation to be applied only to a
specific portion of the datastore that is of interest, such as a
particular subtree. (The filter dow not contain expressions that
would match values data nodes, as this is not required by most use
cases and would complicate the scheme, from implementation to
dealing with race conditions.)
Perhaps "parts/part" rather than "portions/portion".
<AC> Changed
</AC>
Suggest rewording the last sentence to:
Note, the filter does not allow expressions that match against data node
values since that may incur implementation difficulties and is not required for
normal use cases.
<AC> Changed
</AC>
o ... When the target datastore is <operational>, "origin"
metadata is included as part of the patch. Including origin
metadata can help in some cases explain the cause of a difference,
for example when a data node is part of <intended> but the origin
of the same data node in <operational> is reported as "system".
I think that this test needs to refer back to the 'exclude-origin' or
'report-origin' options since the origin metadata isn't always included.
<AC> Yes, this is updated, per earlier comment.
</AC>
5. YANG Data Model
The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL', 'SHALL
NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED', 'NOT RECOMMENDED',
'MAY', and 'OPTIONAL' in this document are to be interpreted as
described in BCP 14 (RFC 2119) (RFC 8174) when, and only when,
they appear in all capitals, as shown here.
I couldn't see that RFC 2119 language is actually used in the YANG module, so
perhaps this can be deleted?
<AC> I thought this was boilerplate, but I will be happy to delete it - removed
</AC>
rpc compare {
description
"NMDA compare operation.";
Perhaps "NMDA datastore compare operation."
<AC> Changed
</AC>
anydata source-value {
when "../operation = 'delete'"
+ "or ../operation = 'merge'"
+ "or ../operation = 'move'"
+ "or ../operation = 'replace'"
+ "or ../operation = 'remove'";
description
"The anydata 'value' is only used for 'delete',
'move', 'merge', 'replace', and 'remove'
operations.";
I'm not convinced how useful the when statement really is in this case, since
'source-value' isn't marked as mandatory, a server is allowed to omit it when
it doesn't apply anyway.
<AC> No change made. The reason why this is specified is that in case of a
"create" operation, it would not make sense to include a source value. Sure,
this is supplied by the server, but still I do think there is value to retain
the statement - there is a client application somewhere that would be very
surprised if a server were to provide a source value here...
</AC>
6. Example
<operational> does not contain object "description" that is contained
in <intended>. Another object, "enabled", has differences in values,
being "true" in <operational> and "false" in <intended>. A third
object, "name", is the same in both cases. The origin of the objects
in <operational> is "learned", which may help explain the
discrepancies.
I think that we should probably refrain from calling them objects, perhaps
"leaf instance" would be better? Rather than "differences in values", perhaps
"different values".
E.g.,
<operational> does not contain an instance for leaf "description" that
is contained in <intended>. Another leaf, "enabled", has different
values in the two datastores, being "true" in <operational> and
"false" in <intended>. A third leaf, "name", has the same instance
value in both datastores. The origin of the leaf instances in
<operational> is "learned", which may help explain the discrepancies.
<AC> Changed. Although I think it would be fair to call them objects also; RFC
8342 talks about objects too. </AC>
//OPERATIONAL
<interfaces
xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces"
xmlns:or="urn:ietf:params:xml:ns:yang:ietf-origin">
<interface or:origin="or:learned">
<name>eth0</name>
<enabled>true</enabled>
</interface>
</interfaces>
There is an extra line, and dodgy indentation for "</interface>".
<AC> Changed, formatted now as follows:
//OPERATIONAL
<interfaces
xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces"
xmlns:or="urn:ietf:params:xml:ns:yang:ietf-origin">
<interface or:origin="or:learned">
<name>eth0</name>
<enabled>true</enabled>
</interface>
</interfaces>
</AC>
Nits:
"possibly for" => "possible for"
"reference for the" => "the reference data tree for the"
"is basis" => "is the basis"
<AC> Changed, except "as reference for the" => "as the reference for the" (I
think data tree would sound a bit odd in the context; also datastore would
perhaps be more appropriate than data tree here).
</AC>
Thanks,
Rob
_______________________________________________
netmod mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/netmod
