Re: [netmod] [core] WG Last Call on draft-ietf-core-comi-16

[Andy Bierman <[e-mail]>]

Hi,

I looked at the diffs and all are OK.


On Mon, Mar 4, 2024 at 12:18 PM Carsten Bormann <[e-mail]> wrote:

> Hi Andy,
>
> I never got around to answering your feedback in the context of an updated
> document.
> The changes marked here with a “commit” number are both in the repository
> ([1], see [2], where also the issues were created) and in
> draft-ietf-core-comi-17.
>
> [1]: https://github.com/core-wg/comi
> [2]: https://github.com/core-wg/comi/commits/main/
>
> On 2023-09-07, at 22:28, Andy Bierman <[e-mail]> wrote:
> >
> > Hi,
> >
> > I have some minor comments about this draft.
> > I did not find any major issues.  It is almost ready for publication.
> >
> >
> > Andy
> >
> >
> > # comments on draft-ietf-core-comi-16
> >
> > ## sec 2.3
> >
> > Examples of each media type would be helpful here
>
> Indeed.  The current document doesn’t have those; captured as
> https://github.com/core-wg/comi/issues/14
>
>

thanks -- implementors rely on correct examples



> > ## sec 2.4
> >
> > It is not clear how any interoperability would be possible
> > if the server used some other datastore design.
> > Para 2 should be removed or there should be a complete
> > specification how NMDA datastores work in CORECONF.
>
> Indeed.
> This is clarified now both in the definitions at the start of Section 2
> and in Section 2.4 (commit b06f134).
>
> > ## sec 3.1.3
> >
> >     For compactness, indexes of the list instance identifiers
> >     returned by the FETCH response SHOULD be elided, only the
> >     SID is provided.
> >
> > If this encoding is used then the FETCH response will
> > not conform to the application/yang-instances+cbor-seq
> > media-type.
>
> A SID is a valid instance-identifier, so the representation still conforms
> to the media type (which is a sequence of maps from instance-identifiers to
> instance values).
>
> > It could be more clear that the client is responsible
> > for remembering the instance information for each
> > varbind in the request since no key values will be in
> > the response.
>
> Added a sentence to this effect (commit 1f48edf).
>


So there MUST be a response for every varbind (even ones that failed or no
data)
or the response message is malformed?  The position is critical since the
same SID
could be requested for different instances.



>
> > ## FETCH design issues
> >
> > The client must know all of the key values in advance
> > before being able to use the SID of a data node
> > in a FETCH request.  It is difficult and expensive
> > to retrieve the entire datastore contents at once,
> > just to discover the key values in use for the YANG list
> > entries within the datastore.
> >
> > RESTCONF has the 'depth' and 'fields' filter parameters
> > to address this issue. NETCONF has subtree and XPath filtering.
> > NETCONF servers can generally support selection of 1 value or all
> > values, for each key.
> >
> > Perhaps CORECONF will need the 'depth' filter parameter.
>
> I captured this as https://github.com/core-wg/comi/issues/15
>
>

SNMP has 'get-next' for instance discovery, which I would be horrified to
see in a new protocol.
NETCONF and RESTCONF have complex filtering but not really any specific
support at all.
The 'depth' parameter is actually complicated to implement correctly and
almost always includes
more data than is needed for this purpose.

I suggest a more direct approach like "keys-only": a boolean filter
parameter indicating that the
only key leafs (and required ancestor containers/lists) are returned.

This is an optimization. A client can get everything which will include the
keys.
It could be added later if an efficient solution is really needed.


> ## sec 3.2.3
> >
> > It is not clear if there are any server requirements
> > for error handling.  What happens if some (but not all)
> > of the edit varbinds succeed?  Is the datastore left
> > in a state such that YANG validation does not pass?
> > How Are partial edits reported to the client?
> >
> > Most NETCONF and RESTCONF servers implement an all-or-none
> > design so that error-option='rollback-on-error' is always done.
> > Perhaps CORECONF needs a SHOULD apply all-or-none for iPATCH?
>
> All-or-none is indeed the intention.
> Error handling is described in Section 6, but that also seems to miss an
> indication that this is the correct outcome.
>
>

Good. Do not repeat the mistakes of NETCONF "error-option".



> While all-or-none processing could be considered to be implicit in the
> design of CoAP, I added a sentence at the start making this explicit
> (commit 79f35e1).
>
>

This should be a requirement for NETCONF.



> > ## sec 3.5.1
> >
> > IMO there should be a SID file 'item' list shown for
> > the examples.
>
> I found the examples quite readable without such a list.
>
> > IMO it is confusing that the SID assignments
> > do not follow the standard algorithm.
>
> There isn’t really a “standard” algorithm; I’m assuming you mean this
> consideration:
>
> > Since the 'input' or 'output' SID is actually used in
> > protocol messages,
>
>

I noticed this significant change that removed the extra containers.



I need to look at the examples more closely to comment on the rest of the
issues.


Andy



> Actually, the examples should show how we are eliding the input/output
> sids, which is the reason non-contiguous assignment is actually optimal.
>
> > the delta values for the child nodes
> > are not optimal unless the correct path strings are sorted.
> >
> > There is a cur-and-paste error:
> >
> > OLD:
> >
> >      This example invokes the 'reboot' RPC (SID 61000), of the
> >      server instance with name equal to "myserver".
> >
> > NEW:
> >
> >      This example invokes the 'reboot' RPC (SID 61000).
>
> Indeed (commit a0a00cd)
>
> >
> > ## sec 3.5.1
> >
> >     REQ:  POST </c>
> >           (Content-Format: application/yang-instances+cbor-seq)
> >
> >     { 61000:
> >       {
> >         1 : 77
> >       }
> >     }
> >     RES:  2.04 Changed
> >           (Content-Format: application/yang-instances+cbor-seq)
> >
> >     { 61000:
> >       null
> >     }
> >
> > I am not sure 2.04 Changed the the correct status to match '200 OK'
>
> RFC 7252 Section 5.8.2 is quite explicit about this.
>
> > The example RPC does not define any 'output' section
> > so the response should not have any payload at all.
>
> It is not quite clear to me how this would work with a POST that addresses
> multiple nodes.  This is not visible from the example, which attempts to
> stay simple, but the elements of the CBOR map sequences used in requests
> and responses need to match up.
>
> > RESTCONF uses the 'input' and 'output' nodes in the
> > payloads. CORECONF is using the RPC method node instead.
>
> Yes.
>
> > ## sec 3.5.2
> >
> >
> > IMO the extra layer in the encoding for '0' is wrong.
> > It should match the format used for RPC.
>
> I agree.
> I also fixed the yang:date-and-time string in the request and made the
> example more realistic with the response indicating three seconds later.
> commit ec12146
>
> > It should be clear that if an RPC or action does not
> > have any input parameters (either direct or augments),
> > then the payload will not have any input parameters.
> >
> > Is this the correct encoding (should have examples in the draft)
>
> (Examples now https://github.com/core-wg/comi/issues/16)
>
> > RPC:
> >
> >     { 61000:
> >       null
> >     }
> >
> > ACTION:
> >
> >     { [60002, "myserver"]:
> >       null
> >     }
>
> Yes, I think so.
>
> > It should be clear that order of YANG list and leaf-list data nodes
> > are significant if they have the ordered-by 'user' property.
>
> Aren't they always?
> Is this specific to RPC/action?
>
> > Note that this applies only to the entries within a parameter.
> > The RPC or action input/output child nodes do not need to
> > be supplied in order. The text in RFC 7950 (e.g. 7.14.4)
> > applies only to NETCONF.
> >
> > ## Appendix A and B
> >
> > There should be a sentence declaring each appendix to
> > be normative, or move to a numbered section instead.
>
> This is the default in RFCs, but it doesn’t hurt to make this explicit.
> commit c72d08c
> (I also updated the YANG because an email address of an author changed;
> used opportunity to use RFCXML support for markers/name — commit fe6f84b)
>
>
> Thanks again for the input and apologies for the long silence…
>
> Grüße, Carsten
>
>
>