Re: [Ntp] Yangdoctors early review of draft-ietf-ntp-yang-data-model-03

[Ankit Kumar Sinha <[e-mail]>]

Hi Andy,

Please find our comment in line below

Thanks,
Ankit

On Tue, 9 Oct 2018 at 06:15, Andy Bierman <[e-mail]> wrote:

> Reviewer: Andy Bierman
> Review result: Almost Ready
>
>
>
>
> Overall:
>    - no compiler warnings; passes --ietf check as well
>
> Normative Sections:
>
> sec 1:
>    - YANG version cited should be RFC 7950, not 6020.
>      YANG 1.1 is used in this document.\
>

[Authors]: Updated

>
> sec 4: Relationship with RFC 7317
>   - this section should say how overlapping configuration
>     objects interact. Does setting 1 field (e.g., /system/ntp/enabled)
>     change the other field at the same time  (e.g., /ntp/enabled)
>     It seems the intent is to ignore and deprecate /system/ntp if
>     /ntp is implemented.  This section should explain.
>

[Authors]: Updated

>
> sec 5:
>   - this section is supposed to have citations for every imported
>     YANG module used in the ietf-ntp YANG module
>

[Authors]: In section 1.4, we have a table of all the improted yang modules
with references. We also use reference statement in the yang model in
section 5. All imported modules are mentioned as normative reference. Is
there any other change you would like to see?

>
>
> YANG Module Issues:
>   1 - Indexing used in /ntp/associations list
>      key "address local-mode isConfigured";
>
>      a) will there really be multiple instances per address for
>         different association-modes?  The list description-stmt
>         should explain.
>

[Authors]: It is possible to have different association-modes for same
address. Example is updated in the the document

>
>      b) There can be configured values (isConfigured key) and learned
>         entries for the same address and association-modes?  Why isn't
>         NMDA used instead? (i.e. intended and operational datastores
>         used instead of the isConfigured list key?)
>

[Authors]: Here /ntp/associations/ is read only, which can have learned
entries and configured entries. Configured entries depends on "unicast,
broadcast, multicast and manycast" configurations. Lets take following
examples

1) If RT1 acting as broadcast server,
   and RT2 acting as broadcast client, then RT2
   will form dynamic association with address as RT1,
   local-mode as client and isconfigured as false.

2) When RT2 is configured
   with unicast-server RT1, then RT2 will form
   association with address as RT1, local-mode as client
   and isconfigured as true.

>
>   2 - Purpose of /ntp/access-rules
>       There is no explanation for what it means to configure an entry
>       here that points at an ACL entry in /acls/acl; The description
>       statement needs to specify the details.  Doesn't the ACL module
>       just work? How does the /ntp/access-rules/access-rule list
>       change behavior?
>

[Authors]: We have updated draft with "section 5 Access Rules" with
explanation

>
>   3 - Reinvent Key Management
>       It does not seem like a good idea for every protocol to
>       duplicate key management functionality. The draft should
>       explain why other YANG modules related to this work are not
>       relevant here
>

[Authors]: We have updated draft with "section 6 Key Management" with
explanation

>
>   4 - Reference statements
>       There are no reference statements used in any objects or typedes
>       Definitions which are intended to match a definition in NTP
>       should include a reference-stmt
>

[Authors]: Updated

>
> Minor Issues:
>    - typedef names should be singular
>       - access-mode not access-modes
>       - association-mode not association-modes
>    - grouping comman-attributes should be common-attributes
>    - mixed-case naming should not be used (isConfigured)
>    - indentation used in module needs a lot of corrections
>    - some descriptions have incorrect tense (e.g., "NTP is enable")
>    - port definition should be based on inet:port-number, not uint16
>      OLD:
>          type uint16 {
>            range "123 | 1025..max";
>          }
>      NEW:
>          type inet:port-number {
>            range "123 | 1025..max";
>          }
>

[Authors]: All the above comments are updated

   - /ntp/authentication/trusted-keys
>      Why is this list needed? Seems a leaf (e.g., trusted-key (true/false)
>      added to the authentication-keys list is sufficient
>
[Authors]: trusted-key is removed and leaf istrusted is added to
authentication-key

   - /ntp/clock-state : some leafs should have a units-stmt instead of
>      specifying the units in the description-stmt (could also apply
> elsewhere)
>    - Examples should use line continuation convention
>      e.g.:
>      OLD:
>       <transmit-time>10-10-2017 07:33:55.300 Z+05:30
>          </transmit-time>
>      NEW:
>       <transmit-time>10-10-2017 07:33:55.300 Z+05:30\
>          </transmit-time>
>    - a spell checker should be used; There are many description-stmts
>      with spelling errors
>

[Authors]: All the above comments are updated