Generic YANG Data Model for the Management of Operations, Administration, and Maintenance (OAM) Protocols That Use Connectionless Communications
RFC 8532

Note: This ballot was opened for revision 11 and is now closed.

(Alia Atlas) (was Discuss) Yes

Comment (2017-10-30 for -16)
No email
send info
Thank you very much for handling my Discuss so well and promptly. I am clearing based on the changes in
version -16.  
I've left the comments and Discuss below for clearer history.

====================
I took a quick look through version -15 and it looks like it addresses almost all of my serious Discuss points.
The only Discuss-worthy point is (c) below.  I have a few more points related to the changes that were made;
they are just comments & listed here to be with the original points.

For version 15:

a) In Sec 3.1,  it still says 
" o  Router-id to represent the device or node.
      [I-D.ietf-spring-sr-yang]"

but [I-D.ietf.spring-sr-yang] has nothing to do with the router-id

b) In Section 4, thanks for adding urn:ietf:params:xml:ns:yang:ietf-lime-common-types - but
could it be a meaningful and accurate name like
   ietf-lime-time-types or ietf-time-types  (Benoit would know best structure)  that clearly
 shows its intended scope for reuse and please fix the description for it too.

c)  [I-D.ietf-rtgwg-ni-model] is still listed as informative, but the model defined in there is imported
"import ietf-network-instance {
    prefix ni;
  }"   It needs to be normative

d) I-D.ietf-spring-sr-yang is still listed as informative - but not really correctly used as a reference.

=================


Thank you for your work on this document.  I have a number of serious concerns - but they all amount to fixing up your
references and slight restructuring  for clarity and reuse.

1) In Sec 3.1, the reference is system-id to represent the device or  node.[I-D.ietf-spring-sr-yang]
I believe that should be 
"typedef router-id {
       type yang:dotted-quad;
       description
         "A 32-bit number in the dotted quad format assigned to each
          router. This number uniquely identifies the router within
          an Autonomous System.";
     }"
from draft-ietf-rtgwg-routing-types.
Certainly "[I-D.ietf-spring-sr-yang]" is NOT an informative reference with such a dependency.

I see that this document actually redefines router-id, instead of using it as part of the included import from
 import ietf-routing-types {
   prefix rt;
  }
On p.27, I see "leaf system-id {
          type rt:router-id;
          description
            "System ID assigned to this node.";
        }"
so it is using the routing-yang-types, but renaming it as system-id, there.
Consistency isn't just the hobgoblin of little minds - it's actually useful.

In choice to-location, again "case system-id {
          leaf system-id-location {
            type router-id;
            description
              "System id location";
          }

          description
            "System ID";"
using the locally defined router-id and renaming it instead of using rt:router-id.


2) On p. 13 & 14, there are many identities associated with time and time-stamps.  I cannot believe that the best way
to handle these is by having them as part of an OAM model!   At a minimum, they should be defined as a separate module
and then included, even if it is in the same draft.  Then they will be available for reuse elsewhere.

3) This is extending [I-D.ietf-i2rs-yang-network-topo] - I do not believe this should be merely an informative reference.

4) I cannot tell if I-D.ietf-rtgwg-ni-model is informative or normative; it is not referenced in the draft - though there are fields
that are labeled NI without adequate description.

5) [I-D.ietf-rtgwg-routing-types] is not an informative reference.  Its module is imported and used.  It must be normative.

6) [I-D.ietf-spring-sr-yang] is listed as an informative reference, but if it were actually used as described, it would need to be normative.
Instead, I believe this can be removed as a reference.
==============================
a) Sec 3.8: It is unfortunate that the cc-session-statistics-data structure is not a list of {traffic type, cc-session-statistics}
instead of hardcoded members for IPv4 and IPv6 traffic only.  While it can still be extended for additional traffic types, the
naming may be inconsistent and there's no requirement that the contents are cc-session-statistics.

b) On p.9: " +--:(system-id)
      |                 +--rw system-id-location?      router-id"

Why isn't this just named router-id instead of system-id, for consistency?  This comment applies throughout the draft.

c) The use of "tp" to mean test-point is a bit unfortunate in a model that is building off of the network topology one, which
uses "tp" for termination-point.  

d) On p. 13: "identity address-attribute-types {
    description
      "This is base identity of address
       attribute types which are ip-prefix,
       bgp, tunnel, pwe3, vpls, etc.";
  }"

I haven't a clue what is meant by a bgp address attribute type or a tunnel one.  Can you please expand the 
description to be substantially more meaningful?  How is it used?

On p. 24, I see these defined
" case bgp {
            leaf bgp {
              type inet:ip-prefix;
              description
                "BGP Labeled Prefix ";
            }
          }
          case tunnel {

            leaf tunnel-interface {
              type uint32;
              description
                "VPN Prefix ";
            }
          }
          case pw {
            leaf remote-pe-address {
              type inet:ip-address;
              description
                "Remote pe address.";
            }
"
but unlike the other cases with clear descriptions and references to the relevant RFCs, these are NOT clear and do not even fully expand acronyms.

e) "grouping tp-address-ni "  Please expand what NI is the abbreviation for in the description.

(Benoît Claise) Yes

(Ben Campbell) No Objection

Comment (2017-10-25 for -14)
No email
send info
I have some mostly editorial comments:

- I agree with Adam's comments about readability. Please also consider an editing pass for grammar--I noted some missing articles, plural disagreement, etc.

- Please check IDNits. It has some complaints about references. I will leave it to the authors to determine if they are real.

-1, paragraph starting with "The different OAM tools...": The phrase "After the connection is established" is missing an article.

-2.2: "RPC operation - A specific Remote Procedure Call.": That seems like a circular definition.

-3, first paragraph: 'The model augments "/networks/network/node" path...': missing article.

-3.1, 6th paragraph: "... these parameters are not explicit configured": s/explicit/explicitly

-5.2.1.2, 2nd paragraph: s/user/users

-6, third paragraph: " writable/creatable/deletable": Please don't use "/" as a substitute for conjunctions.

Alissa Cooper No Objection

Comment (2017-10-25 for -14)
No email
send info
Update: It seems the Gen-ART comments concerning the summary of the imported modules and their sources and the timestamps still need to be addressed. Sorry for missing this in my original ballot.

(Spencer Dawkins) No Objection

Suresh Krishnan No Objection

Warren Kumari No Objection

Comment (2017-10-26 for -14)
No email
send info
I fully agree with the readability comments - it needs a good editorial pass, and I feel it is unfair to give to the RFC Editor without one.

Mirja Kühlewind No Objection

Alexey Melnikov No Objection

(Kathleen Moriarty) No Objection

Comment (2017-10-23 for -13)
No email
send info
Thanks for your work on this draft and following the template for security considerations.

Also, thanks for responding to the SecDir review:
https://mailarchive.ietf.org/arch/msg/secdir/h1A7I1CVAZme2CvOKapZ6QiqvPQ

(Eric Rescorla) No Objection

Comment (2017-10-24 for -13)
No email
send info
Can you add a sentence or two to the Security Considerations explaining why the nodes that are sensitive are sensitive?

I noticed a number of punctuation, spacing, and grammar errors. These will probably be caught by RFC-Ed but a proofread might help.

Alvaro Retana No Objection

Adam Roach No Objection

Comment (2017-10-25 for -14)
No email
send info
I'd like to update my comment with some fairly mechanical suggestions for improvement that I believe will increase readability of the document greatly. In evaluating this document, I found a number of minor formatting issues that made it somewhat difficult to read.

1. Please ensure that all opening parentheses have a space before them and no space after them.

2. Please ensure that all closing parentheses have a space after them and no space before after them.

3. Please ensure that all quoted terms include both an opening quotation mark and a closing quotation mark.

4. Please ensure that there are no spaces between a quotation mark and the term it is quoting.

5. Please ensure that there *is* a space before an opening quotation mark

6. Please ensure that there *is* a space after a closing quotation mark (unless followed by another punctuation mark)

7. Please ensure that periods at the end of a sentence have no space before them and a space after them.

8. Please break up long paragraphs into separate paragraphs or bullet lists. The third paragraph of section 3 and the paragraph that forms section 3.3 are prime candidates for such an improvement.

9. Please double-check the formatting of the YANG module. The indentation is inconsistent and, in some places, can easily mislead the reader about the level of nesting and association of elements with each other.

My original comments follow.

------------------------------------------------------------

Please expand "EXP", "VPLS", and "LAG" on first use.

Section 3.2 refers to the "lime base model". Please define or expand "lime" or provide a citation that does so.

The id-nits tool reports that there are 6 instances of overly-long lines in the document. Given that these exist in code elements, the authors can probably make better decisions about how to resolve these than the RFC editor can.

Section 3.3 contains the following definition:

                list oam-neighboring-tps {
                  key "index";
                  leaf index {
                     type uint16 {
                        range "0..65536";
                     }

uint16 cannot represent 65536.

----------------------------------------

Later in the model:

 container timestamp-80bit {
 when "derived-from-or-self(../timestamp-type, 'cl-oam:ptp80')"{
         description
          "Only applies when 80bit PTP Timestamp.";
        }
  if-feature ptp-long-format;
      leaf timestamp-sec {
      type uint64 {
      range "0..281474976710656";
      }
      description
        "48bit Timestamp in seconds as per IEEE1588v2.";
       }
      leaf timestamp-nanosec {
      type uint32;
      description
        "Fractional part in nanoseconds as per IEEE1588v2
         or Fractional part in 64-bit NTP timestamp.";
      }
      description
      "Container for 64bit timestamp.";
    }

Issue 1: The 48-bit range should be 0..281474976710655, not 0..281474976710656

Issue 2: The description for this 80-bit timestamp container contains a description of "Container for 64bit timestamp."

----------------------------------------

Similar to issue 2 above, ntp-timestamp-32bit describes itself as a 64-bit timestamp.