Re: [yang-doctors] [Netconf] Yangdoctors last call review of draft-ietf-netconf-nmda-netconf-03

[Martin Bjorklund <[e-mail]>]

Hi,

Ebben Aries <[e-mail]> wrote:
> Hi Martin - see inline...
> 
> On Feb 23 16:43 PM, Martin Bjorklund wrote:
> > Hi,
> > 
> > Thank you for this review.  Comments inline.
> > 
> > Ebben Aries <[e-mail]> wrote:
> > > Reviewer: Ebben Aries
> > > Review result: Almost Ready
> > > 
> > > 1 module in this draft:
> > > - [e-mail]
> > > 
> > > YANG validation errors or warnings (from pyang 1.7.3 and yanglint 0.14.69)
> > > - [e-mail]:171: warning: RFC 6087: 4.10,4.12:
> > >   statement "enum" should have a "description" substatement (From pyang 1.7.3)
> > 
> > Yes, this is a warning b/c it is a SHOULD in 6087.  In this case, the
> > enum is described together with the rest in the main description.  I
> > think it gives a better overall description of the type.
> > 
> 
> I'm fine w/ this - I agree
> 
> > > 0 examples are provided in this draft (section 3.12 of
> > > draft-ietf-netmod-rfc6087bis-18)
> > > Module [e-mail]:
> > > - Note: For the following imports, there are no references to the supporting
> > >   documents as suggested in rfc6087bis however this item is currently under
> > >   discussion on both usefulness/possible formatting
> > >   - import "ietf-yang-types" should reference RFC 6991 per (not as a comment)
> > >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> > >   - import "ietf-inet-types" should reference RFC 6991 per (not as a comment)
> > >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> > >   - import "ietf-datastores" should reference I-D.ietf-netmod-revised-datastores per
> > >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> > >   - import "ietf-origin" should reference I-D.ietf-netmod-revised-datastores per
> > >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> > >   - import "ietf-netconf" should reference RFC 6241 per
> > >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> > >   - import "ietf-netconf-with-defaults" should reference RFC 6243 per
> > >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> > 
> > I think we have to wait for the outcome of that discussion.  I would
> > prefer to add the reference statements.
> > 
> 
> Since we've been actively suggesting to add into the reference
> statements, I suggest to just go ahead w/ the syntax you previously
> described.  If we have a different outcome, then 6087bis can be updated
> as well as modification to modules if time permits.


I have added references for now.

> > > - Module description, revision and feature definition should contain note to
> > >   RFC Ed. to change placeholder reference to RFC when assigned
> > >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23appendix-2DC&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=Bsh4kYXQ9T8sGblwrJnk41asnQCqSuUZjzvla2EgA9E&e=
> > 
> > Ok.
> > 
> > > - Security Considerations looks good and is adjusted to account for NETCONF
> > >   only as well as addressing the additional RPCs introduced
> > > - L82: suggested replacement of text:
> > >   s/, i.e., the filter criteria are logically ANDed/. Multiple filters are
> > >   processed as a logical AND operation/
> > 
> > Hmm.  This misses the "i.e." - the original text explains the sentence
> > just before it.  What do others think?  Maybe leave this to the RFC
> > editor?
> > 
> 
> We can leave this to the RFC editor
> 
> > > - L127: suggested replacement of text:
> > >   s/then the get-data/the get-data/
> > 
> > We have "if ... then ..." in several places.  Maybe also leave this to
> > the RFC editor?
> > 
> 
> We can leave this to the RFC editor
> 
> > > General comments/nits on draft text:
> > > - As mentioned above, there are 0 examples in this draft.  There should be XML
> > >   RPC examples of the 2 newly defined RPCs as well as usage examples of the
> > >   augments of RFC6241 RPCs
> > 
> > This is a SHOULD in 6087.  I don't think that examples for these
> > operations would add much value.
> >
> 
> More examples is generally a good thing and provides clarity to
> implementors.  While it is a SHOULD, RFC6241 carries examples across all
> sections and propose this extension follow the same format.

Ok, done.

> > > - :with-origin    urn:ietf:params:netconf:capability:with-origin:1.0
> > >   Wouldn't it make more sense to have this URN defined in
> > >   I-D.ietf-netmod-revised-datastores rather?  In addition, should this
> > >   capability and feature be rather defined as 'origin' in the 'ietf-origin'
> > >   module?  As defined today, this feature is used as a clause for
> > >   'origin-filter' as well as 'with-origin' inputs to get-data however these
> > >   inputs appear to be mutually exclusive.
> > >   IMHO, this should be detached from this specific draft which focuses on
> > >   NETCONF specific base extensions for datastores as the returning and
> > >   filtering of origin metadata can be transport independent.
> > 
> > The capability is NETCONF-specific, and should be defined here, imo.
> 
> Understood.  I guess what I would have imagined is rather a capability
> indicating that the operational datastore supports the population of
> 'origin' metadata.  Support of that capability would then indicate
> support of the 'with-origin' parameter which is just a toggle to return
> metadata or not.

Ok.  After some discussion with my co-authors, we suggest that we
remove the ":with-origin" capability, and just use the feature
"origin".  In fact, the YANG module ietf-netconf-nmda already uses
this feature to control both the "with-origin" and "origin-filter"
parameters, and the feature is advertised on servers that support the
"origin" annotation.  The YANG module doesn't even mention the
"with-origin" capability.

> Per draft-ietf-netmod-revised-datastores, it appears that the population
> of origin metadata is mandatory however the wording appears pretty vague
> around such.

The architecure document defines this concept, but I think that
different protocols may or may not expose this; for example, a
protocol for constrained devices might choose to not support this at
all.

> This capability exists in both this draft as well as the RESTCONF draft
> and am trying to understand if origin metadata is mandatory for support
> in <operational> then why is it necessary to have a capability just for
> support of this parameter vs. a capability of origin metadata support as
> a whole?
> 
> If the capability is meant to convey 'origin' metadata support as a
> whole, then I think it should be named 'origin' rather than
> 'with-origin' (which is just the parameter name we are discussing)
> 
> > As for the feature, it is not as clear.  It is not obvious that it
> > should be defined in ietf-origin.  (and since that doc is at the RFC
> > editor, it might be late to change, unless it is really urgent).
> > 
> 
> It seems odd to me that we wouldn't define the feature along side the
> annotation/identity definitions but rather here in a protocol specific
> module.  I think that the population of 'origin' metadata support as a
> whole should be the feature, this should exist in the ietf-origin module
> and 'origin-filter' and 'with-origin' parameters would still be gated by
> this feature that is defined elsewhere.
> 
> As is defined today, the feature description even indicates server
> support for the annotation as a whole.
> 
>   feature origin {
>     description
>       "Indicates that the server supports the 'origin' annotation.";
>     reference
>       "RFC XXXX: Network Management Datastore Architecture";
>   }
> 
> 
> > > - L308: suggested replacement of text:
> > >   s/parameter is optional to support/parameter is optional and dependant off
> > >   of the servers support for the 'origin' feature/ (dependant off the previous
> > >   comment)
> > 
> > Do you mean that if we don't move the origin feature, this text should
> > not change?
> > 
> 
> The suggestion above is merely to add a bit more description around this
> parameter being gated off the server's support for populating origin
> metadata.  If the server supports origin metadata, then it is optional
> and the client can choose to use/not-use.  If the server does not
> support origin metadata, then this parameter is not supported or
> visible.
> 
> > > - L320: "ietf-netconf-nmda" (see Section 4). - This should be an actual
> > >   reference as in line #306
> > 
> > I don't understand this comment.  It is a proper reference...?
> > 
> 
> I would assume reference to [I-D.ietf-netmod-revised-datastores] here
> but there are a number of other spots where this exists as well.  This
> can be left to the RFC editor as well
> 
> > > - L345: "default-operation" parameter of the <edit-config> operation - This
> > >   should at a bare minimum call reference to the appropriate section in
> > >   RFC6241
> > 
> > There is a reference for <edit-config> to RFC 6241 in the first
> > paragraph of this section.  I think that is how references should be
> > done, but I might be wrong.  I am sure the RFC editor will fix this,
> > if needed.
> > 
> 
> We can leave this to the RFC editor
> 
> > > - To the previous point, since there is a large amount of reference and
> > >   comparison to RFC6241 definitions, any references should be tagged
> > >   appropriately
> > 
> > I am not sure I understand what you mean with "tagged appropriately".
> > Could you provide OLD / NEW text for these occurences?
> >
> 
> Same comment as above.  This can be left to the RFC editor and don't
> believe all definitions need to be referenced.



/martin