Re: [Netconf] Shepherd review of draft-ietf-netconf-nmda-restconf

[Mahesh Jethanandani <[e-mail]>]

> On Apr 18, 2018, at 10:41 PM, Martin Bjorklund <[e-mail]> wrote:
> 
> Mahesh Jethanandani <[e-mail]> wrote:
>> 
>> 
>>> On Apr 12, 2018, at 3:53 AM, Martin Bjorklund <[e-mail]> wrote:
>>> 
>>> Hi,
>>> 
>>> 
>>> Mahesh Jethanandani <[e-mail]> wrote:
>>>> I have reviewed -03 version of draft-ietf-netconf-nmda-restconf. 
>>>> 
>>>> The document is well written, and is easy to read, but has issues
>>>> which should be addressed before the document is sent to IESG.
>>>> 
>>>> Nits.
>>>> 
>>>> Abstract:
>>>> 
>>>> s/REF Editor/RFC Editor/
>>> 
>>> Fixed.
>>> 
>>>> 1.0 Introduction:
>>>> 
>>>> The first comma in this sentence seems to be misplaced. 
>>>> 
>>>> “supported in each datastore and, finally,” ??
>>> 
>>> I removed the "finally,".
>>> 
>>>> 2. Datastore and YANG Library Requirements
>>>> 
>>>> Can we consolidate the RFC Editor instructions in one place, instead
>>>> of splitting them between here and Abstract.
>>> 
>>> I don't think this matters; these are just instructions to the RFC
>>> editor that will be removed.  If the RFC editor has a preference for
>>> either style I'd be happy to follow that.
>> 
>> The RFC editors have indicated that they would prefer RFC instructions
>> for the entire draft appear in one place.
>> 
>>> 
>>>> Detailed comments:
>>>> 
>>>> Introduction:
>>>> 
>>>> The document states:
>>>> 
>>>> "This document updates [RFC8040 <https://tools.ietf.org/html/rfc8040>]
>>>> in order to enable RESTCONF clients to discover which datastores are
>>>> supported by the RESTCONF server, as well as determine which modules
>>>> are supported in each datastore and, finally, to interact with all the
>>>> datastores supported by the NMDA. Specifically, the update introduces
>>>> new datastore resources, adds a new query parameter, and requires the
>>>> usage of [I-D.ietf-netconf-rfc7895bis
>>>> <https://tools.ietf.org/html/draft-ietf-netconf-nmda-restconf-03.html#ref-I-D.ietf-netconf-rfc7895bis>]
>>>> by RESTCONF servers implementing the NMDA."
>>>> 
>>>> 
>>>> Questions:
>>>> 
>>>> 1) How does this draft enable RESTCONF clients to discover which
>>>> datastores are supported?
>>>> 
>>>> Is it the definition of the new datastores resource? If so, can that
>>>> be made explicit, by saying something like this in Section 3.1
>>> 
>>> No, it is the inclusion of the new YANG library that makes this
>>> possible.  I suggest we add a paragraph at the end of section 2:
>>> 
>>> A RESTCONF client can discover which datastores and YANG modules the
>>> server supports by reading the YANG library information from the
>>> operational state datastore.
>> 
>> Ok.
>> 
>>> 
>>> 
>>>> OLD:
>>>> This document defines a set of new resources representing datastores
>>>> as defined in [I-D.ietf-netmod-revised-datastores
>>>> <https://tools.ietf.org/html/draft-ietf-netconf-nmda-restconf-03.html#ref-I-D.ietf-netmod-revised-datastores>].
>>>> 
>>>> NEW:
>>>> This document defines a set of new resources representing datastores
>>>> as defined in [I-D.ietf-netmod-revised-datastores
>>>> <https://tools.ietf.org/html/draft-ietf-netconf-nmda-restconf-03.html#ref-I-D.ietf-netmod-revised-datastores>]. This
>>>> resource
>>>> can be used to discover the datastores that are supported by the
>>>> server.
>>>> 
>>>> On the question of discovery of datastores itself, would the clients
>>>> use the HEAD or GET methods to query what datastores are supported? If
>>>> so, an example would be nice. Specifically, something that explains
>>>> what response and errors to expect if the datastore is supported, and
>>>> what response and errors to expect if the queried datastore is not
>>>> supported.
>>>> 
>>>> 2) How does the draft help determine which modules are supported by
>>>> each datastore?
>>> 
>>> See above, this is also covered by my proposed text.
>>> 
>>>> I went looking for text in the draft that talks about module discovery
>>>> under each datastore, but did not find any. Am I missing something?
>> 
>> I do not see this issue addressed. The document states in the
>> Introduction that:
>> 
>> "as well as determine which modules are supported in each datastore"
>> 
>> Where in the draft are we talking about module discovery in each
>> datastore? If this is covered somewhere else, we should either remove
>> this sentence or refer to the draft that covers it.
> 
> The text I proposed was:
> 
>  A RESTCONF client can discover which datastores and YANG modules the
>  server supports by reading the YANG library information from the
>  operational state datastore.
> 
> Do you think this answers the question which modules are supported?

It does, and sorry I missed that the first time around.

I think that satisfies all my queries. If you could post an updated draft with the changes we agreed, I will do the write-up.

Thanks.

> 
> 
> /martin
> 
> 
>> 
>> Thanks.
>> 
>>>> 
>>>> 3) The part of the sentence “to interact with all the datastores
>>>> supported by the NMDA” seems incomplete.
>>> 
>>>> What interaction are we referring to? Is it the protocol operations on
>>>> the datastore? Is it the query parameters on the datastores?
>>> 
>>> Yes, both.  A client can interact by reading or writing the
>>> datastores.
>>> 
>>>> Section 3.2.1
>>>> 
>>>> What happens if the capability is not announced, but a “with-defaults”
>>>> query is issued?
>>> 
>>> If the server doesn't understand the query, it will reply as defined
>>> by RFC 8040, section 4.8.  This document doesn't change that behavior.
>>> 
>>>> Finally, what are the implications of explicit discovery of datastores
>>>> supported by a RESTCONF server? For example, if the above
>>>> “with-defaults” query is issued with the “content” query parameter set
>>>> to “nonconfig", default values from which datastore are returned?
>>> 
>>> I don't understand this question.  Neither of these query parameter
>>> controls from which datastore data is returned.
>>> 
>>>> Also, if the query parameter “content” is not present, RFC 8040 says
>>>> that the default value is “all”. Does “all” include operational
>>>> datastore data nodes, with or without the new capability? In other
>>>> words, do we need to redefine “all”?
>>> 
>>> No, RFC 8040 says that "all" means: "Return all descendant data
>>> nodes".  This document doesn't change that.
>>> 
>>> 
>>> /martin
>> 
>> Mahesh Jethanandani
>> [e-mail]
>> 

Mahesh Jethanandani
[e-mail]