Re: [Netmod-ver-dt] Comments on the guidelines chapter

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Reshad,

Thanks for the updates.  I’ve also had some comments from Martin Bjorklund.  The trivial ones I will just make, the others we can discuss later today.

I’m not sure that we have got the revision-label right for submodules, and should discuss further today:

  *   Banning the revision-label entirely for submodules seems a bit strict
  *   From the meeting last week, the agreement was that in the revision-label is present in a sub-module then it must match the importing parent module.  But I think that this is implies that revision-dates between a module and submodule are allowed to differ, but revision-labels have to be in lock step.  This seems wrong to me.
  *   I tried to write up a stricter version of this stating that the revision-date and revision-label of sub-modules must match the importing module, but Jason indicated that this is too restrictive for their use case.

Hence, I think that we should probably allow a revision-label for a sub-module, but indicate that it has no bearing on the module revision-label, perhaps with the recommendation that if present it should match the including module (for the cross referencing that mention).

Thanks,
Rob


From: Reshad Rahman (rrahman)
Sent: 12 June 2019 20:05
To: Rob Wilton (rwilton) <[e-mail]>; [e-mail]
Subject: Re: Comments on the guidelines chapter

Hi Rob, all,
I’ve made the changes to the guidelines as per the notes below, diff attached.


  1.  Need to add text similar to this (change semver to revision):  Put this into section 2.  Also need to state that labels in submodule revisions, if present, MUST be the same at the including module.
My memory is failing me, can someone remind me why we need revision-labels in submodule revisions? I understand that the extension applies to the revision statement which are used by sub-modules, but shouldn’t we just ignore revision-labels in submodules? Or is the point to be able to “cross-reference” module and submodule revisions?
Also the note says section 2, not sure why, I added it in the guidelines (7.1).

  1.  There is text in 7.1 which says [Moved from section 4.], I think that was added by Rob in last commit. I didn’t remove it in case it was put there for a specific reason.

Comments welcome, feel free to make any changes to the guidelines, I won’t be able to spend much time on this for the rest of the week.

Regards,
Reshad.


From: "Rob Wilton (rwilton)" <[e-mail]<mailto:[e-mail]>>
Date: Thursday, June 6, 2019 at 11:04 AM
To: "Reshad Rahman (rrahman)" <[e-mail]<mailto:[e-mail]>>, "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>
Subject: RE: Comments on the guidelines chapter

One other change (for Rob):
- In 3.3, need to state that the revision label MUST be unique for a given YANG module.  This also needs to be added to the YANG module definition.


From: Reshad Rahman (rrahman)
Sent: 05 June 2019 21:53
To: Rob Wilton (rwilton) <[e-mail]<mailto:[e-mail]>>; [e-mail]<mailto:[e-mail]>
Subject: Re: Comments on the guidelines chapter

Hi Rob,

Yes we can discuss in the meeting. Inline.

From: "Rob Wilton (rwilton)" <[e-mail]<mailto:[e-mail]>>
Date: Wednesday, June 5, 2019 at 12:58 PM
To: "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, "Reshad Rahman (rrahman)" <[e-mail]<mailto:[e-mail]>>
Subject: Comments on the guidelines chapter

Hi Reshad,

Sorry that these have been so late, but here are some comments on the guidelines chapter.  Potentially we can discuss tomorrow, if needed.

>6.1.  Guidelines for YANG module authors

I think that this section should state that is updates RFC8407 sections 4.7 and 4.8, and I think that we should check the text against this existng BCP.
<RR> Ack. I thought I had done that, but missed it.

I think that the text also needs to cover:

(i)               Adding a "rev:nbc-changes;" if and only if there are NBC changes relative to the previous revision.
[RW]
Reshad – will add.

(ii) Fixing RFC8407 section 4.7's "The revision date substatement within the import statement SHOULD be present if any groupings are used from the external module.".
[RW]
Move following text (or similar) to guidelines:

      Using the "revision-date" statement causes overly strict import
      dependencies between modules and SHOULD NOT be used.




(ii)              Fixing RFC8407 section 4.7's "The revision date substatement within the include statement SHOULD be present if any groupings are used from the external submodule." - I think that we want this to be a MUST otherwise the module doesn't have a stable definition because the the module revision does not uniquely identify its content.

[RW]

Need to add text similar to this (change semver to revision):  Put this into section 2.  Also need to state that labels in submodule revisions, if present, MUST be the same at the including module.


   The YANG semantic versioning scheme applies only to YANG modules.

   YANG submodules are not independently versioned by the YANG semantic

   versioning scheme.  Instead, if a versioned module includes one or

   more submodules then those submodules are implicitly versioned as

   part of the module's 'semver:version' statements, and all the

   module's 'include' statements MUST specify the revision-date for each

   of the included submodules.

Probably also want a guideline statement as well:
The revision date substatement within the include statement MUST be present if any groupings are used from the external submodule.


(iii)            Some text stating that removing old revision statements from a modules revision history could break import by revision, and hence it might be better to retain them, unless it is known that all depencencies have been updated.  An alternative solution, if the revision section is too long, would be remove, or curtail, the older description statements associated with old historical revisions.

[RW]
OK, Reshad to add text along these lines.


(v) Some guidance about whether updating the revision in import revision-or-derived is a BC or NBC change.  I think that it is NBC is the imported module has been changed in an NBC way between the revision dates.

[RW]
Leave out this text for now, and resolve via the open issues.


>
>   NBC changes to YANG modules may cause problems to clients, who are
>   consumers of YANG models, and SHOULD be avoided.  YANG module authors
>   are recommended to minimize NBC changes and keep changes BC whenever
>   possible.

I think "SHOULD be avoided" is too strong.  Perhaps:

NBC changes to YANG modules may cause problems to clients, who are
consumers of YANG models, and hence YANG module authors are
RECOMMENDED to minimize NBC changes and keep changes BC whenever
possible.

<RR> Good with me.

My counter argument is that there are somecases (e.g. bugfixes), where an NBC change makes sense because it should not break any clients.

<RR> Ack.

>
>   When NBC changes are introduced, consideration should be given to the
>   impact on clients and YANG module authors SHOULD try to mitigate that
>   impact.

OK.


>
>6.1.1.  Making non-backwards-compatible changes to a YANG module
>
>   There are various valid situations where a YANG module has to be
>   modified in a non-backwards-compatible way.  Here are the different
>   ways in which this can be done:
>
>   o  NBC changes can be done incrementally using the 'deprecated'
>      status to provide clients time to adapt to NBC changes.

I would suggest caveating this to:

  o  NBC changes can sometimes be done incrementally using the 'deprecated'
     status to provide clients time to adapt to NBC changes.
<RR> OK.

E.g. I'm still to be convinced that in the mainline case that configuration can be fixed incrementally.
<RR> I think the issue is supporting both at the same time. E.g. in the vpn-id example where the type is changed from integer to string,  it can be done incrementally but supporting both at the same time on a server is an issue (mapping values etc).

>
>   o  NBC changes are done at once, i.e. without using 'status'
>      statements.  This has a big impact on clients.

"This has a big impact on clients." => "Depending on the change, this may have a big impact on clients."

<RR> Ack.

>
>   o  If the server can support multiple versions of the YANG module or
>      of YANG packages(as specified in
>      [I-D.rwilton-netmod-yang-packages]), and allows the client to
>      select the version (as per
>      [I-D.wilton-netmod-yang-ver-selection]), then NBC changes MAY be
>      done without using 'status' statements.  Clients would be required
>      to select the version which they support and the NBC change would
>      have no impact on them

OK.


>
>   Here are some guidelines on how non-backwards compatible changes can
>   be made incrementally:

Perhaps, alter this to something like:

Here are some guidelines on how non-backwards compatible changes can be made incrementally, with the assumption that deprecated nodes are implemented by the srever, and obselete nodes are not:
<RR> Good with me.

>
>   1.  The changes should be made gradually, e.g. a data node's status
>       SHOULD NOT be changed directly from "current" to "obsolete" (see
>       Section 4.7 of [RFC8407]), instead the status SHOULD first be
>       marked "deprecated" and then when support is removed its status
>       MUST be changed to "obsolete".  Instead of using the "obsolete"
>       status, the data node MAY be removed from the model but this has
>       the risk of breaking modules which import the modified module.
>
>   2.  A node with status "deprecated" MUST be supported for the
>       solution described here to function properly.

As above, I think that this could be merged into the first sentence.
<RR> So just take bullet 2 out?

>
>   3.  A node with status "deprecated" SHOULD be available for at least
>       one year before its status is changed to "obsolete", see
>       Section 4.7 of [RFC8407].

I'm not sure whether we need to even state this one (since it is already in RFC8407).
<RR> Yes, and I’d like to avoid this statement since it’s contentious.


>
>   4.  Support for a node which is "obsolete" is indicated by the node
>       "obsolete-nodes-present, see Section 4.

As above, I think that this coudl be merged into the first sentence.
>
>
>
>Claise, et al.          Expires December 1, 2019               [Page 15]
>

>Internet-Draft           YANG Module Versioning                 May 2019
>
>
>   5.  The new extension "status-description" SHOULD be used for nodes
>       which are "obsolete" or "deprecated".

Should we indicate what information should be included:
- Reason for deprecation (if interesting)
- Replacement node (if known)
- Release/time that the node is anticipated to be obsoleted.

<RR> The last 2 points are  mentioned in the following bullets. I’ll add the 1st point.

Should we also indicate that "status-description" can be used with "status current" to provide forewarning that it could be deprecated, or does that mean that the node is already deprecated?

>
>   6.  For status "deprecated", the "status-description" SHOULD also
>       indicate until when support for the node is guaranteed.  If there
>       is a replacement data node, rpc, action or notification for the
>       deprecated node, this SHOULD be stated in the "status-
>       description".

I think that the first "SHOULD" should be a "MAY" because it is hard to foretell the future.  Alternatively, or perhaps in addition, it should be caveated with an "if known".
<RR> if known.

I think that this paragraph should also cover "reason for deprecated (if interesting)", and some of the status-description information also applies, and should be preserved when it becomes status-obsolete.


>
>   7.  When obsoleting or deprecating data nodes, the "deprecated" or
>       "obsolete" status SHOULD be applied at the highest possible level
>       in the data tree.  For example, when deprecating all data nodes
>       in a container, the "deprecated" status SHOULD be applied to the
>       container.  For clarity, the status MAY be added in all the
>       affected nodes but the status-description SHOULD be added only at
>       the highest level in the tree.

I would suggest rephasing to something like:

   7.  When obsoleting or deprecating data nodes, the "deprecated" or
       "obsolete" status SHOULD be applied at the highest possible level
       in the data tree with an appropriate status-description.  For
       clarity, status statement SHOULD also be applied to all descendent
       data nodes, but status-description does not need to be repeated
       if it does not introduce any additional information.
<RR> Good with me.

>
>   See Appendix A.2 for examples on how non-backwards-compatible changes
>   can be made.
>
>6.2.  Versioning Considerations for Clients
>
>   Guidelines for clients of modules using the new module revision
>   update procedure:
>
>   o  Clients SHOULD be liberal when processing data received from a
>      server.  For example, the server may have increased the range of
>      an operational node causing the client to receive a value which is
>      outside the range of the YANG model revision it was coded against.
>
>   o  Clients SHOULD monitor changes to published YANG modules through
>      their revision history, and use appropriate tooling to understand
>      the specific changes between module revision.  In particular,
>      clients SHOULD NOT migrate to NBC revisions of a module without
>      considering the specific NBC changes.

considering -> "understanding any potential impact of"
<RR> Ack.

Regards,
Reshad.

>
>   o  Clients SHOULD plan to make changes to match published status
>      changes.  When a node's status changes from "current" to
>      "deprecated", clients SHOULD plan to stop using that node in a
>      timely fashion.  When a node's status changes to "obsolete",
>      clients MUST stop using that node.

Thanks,
Rob