Re: [Lime] WGLC: draft-ietf-lime-yang-connectionless-oam-03

Thank Adrian for these valuable comments, please find my reply inline.

Best Regards!
-Michael

-----邮件原件-----
发件人: Lime [mailto:lime-bounces@ietf.org] 代表 Adrian Farrel
发送时间: 2017年1月25日 20:40
收件人: 'Ron Bonica'; lime@ietf.org; draft-ietf-lime-yang-connectionless-oam.all@ietf.org
主题: Re: [Lime] WGLC: draft-ietf-lime-yang-connectionless-oam-03

Hi, 

I read this document as part of the WG last call. 

In my opinion the document is *almost* ready to move forward, but has some small issues that need to be resolved first.

Thanks for the work,
Adrian

===

The use of English could use some work. The RFC Editor will catch this, but there is a risk that they will break something, so if you have the chance to get someone to edit the document first, that would help.

---

There are a number of nits reported at
https://www.ietf.org/tools/idnits?url=https://www.ietf.org/archive/id/draft-ietf
-lime-yang-connectionless-oam-03.txt
[Michael]: Thanks, and we would like to fix them.
---

The datatracker is reporting "Submission Yang Validation returned warnings or errors." No idea about this, but sounds like something that should be fixed.
[Michael]: Thanks, and we would like to fix them.
---

Need to expand "OAM" in the Abstract.
[Michael]: Agree.
---

Abstract s/Based model/Base model/
[Michael]: Thanks, we will fix it.
---

I think the Introduction would benefit from a short paragraph referencing draft-ietf-lime-yang-oam-model and explaining the difference.
[Michael]: It make sense to me, I would like to discuss with my coauthor to add some explaining in next version.
---

Section 1 has tree citations of "[lime retrieval methods]". You need to sort this out.
[Michael]: Yes, we will fix it in next version.
(I suspect idnits catches this)

---

Section 2.2. See my comments on the equivalent section in draft-ietf-lime-yang-oam-model
[Michael]: Agree.
---

Section 3, etc.
Probably best to use single quotes (') around names of data nodes etc.
Thus, for example, 
   At the top of the Model, there is an 'oper' container for session
[Michael]: Agree.
---

In 3.3 I can't make sense of
   Level is provided for scenarios where
   it might be possible to define layering relationship as it can be
   used to stitching fault at related OAM layers.

It seems to me (from the example that follows) that you are not talking about layering of network technologies so much as concatenation of path segments each of which is at a different technology. Maybe rewrite the text as...
   'Level' defines the relative technology level in a sequence of 
   network portions, and is provided to allow correlation of faults in
   related OAM domains.
Or maybe I am only guessing.
[Michael]: I would to discuss with other authors to make it more clearly.
---

3.3
s/thesecond/the second/
[Michael]: Agree.
---

3.3

                  leaf level {
                      type int32 {
                           range "-1..1";
                      }
                      description
                        "Level";
                  }

It's not important, but I did wonder whether you would be better defining "up", "same", and "down" rather than using an int32.
[Michael]: I would to discuss with other authors to decide whether change it.
---

A number of description clauses need to begin with capital letters.
[Michael]: Agree, I will fix it in next version.
---

I am trying to understand how a test point location can be identified by a multicast group address. I think you might want to target some of the OAM tools at a multicast address (e.g., ping a mcast address) but I don't see this as the same as a text point location. Am I missing something?
[Michael]: I will discuss with other authors, and improve it.
---

You define oam-counter32 but then a number of leaf nodes (such as
session-count) are of type uint32. In fact, I don't see oam-counter32 used at all.
[Michael]: Yes, it need to be removed.
---

    grouping session-packet-statistics {
      description "Grouping for per session packet statistics";
      container session-packet-statistics {

        description "Per session packet statistics.";
        leaf rx-packet-count {
          type uint32;
          description "Total received packet count.";
        }
        leaf tx-packet-count {
          type uint32;
          description "Total transmitted packet count.";
        }
        leaf rx-bad-packet {

          type uint32;
          description "Total received bad packet.";
        }
        leaf tx-packet-failed {
          type uint32;
          description "Total send packet failed.";
        }
      }
    }

Please clarify whether this is "Total number of OAM packets..." If it is counting normal data packets, uint32 is not large enough.
[Michael]: Agree, and I will fix it in next verison.
---

    grouping session-path-verification-statistics {
      description "Grouping for per session path verification statistics";
      container session-path-verification-statistics{
        description "OAM per session path verification statistics.";
        leaf verified-count {
          type uint32;
          description "Total number of packets that went through a path as intended.";
        }
        leaf failed-count {
          type uint32;
          description "Total number of packets that went through an unintended path.";
        }
      }
    }

Please clarify whether this is "Total number of OAM packets..." If it is counting normal data packets, uint32 is not large enough.
[Michael]: Agree, and I will fix it in next verison.
---

Not sure why you need to define IP-Multicast-Group-Address since a mcast IP address surely looks a lot like an IP address for which types already exist.
[Michael]: I would to discuss with other authors to decide whether remove it.
---

Quite a few places "IP" is presented as "Ip"
[Michael]: Agree, I will fix it in next version.
---

      container path-verification {
        description "Optional path verification related information.";
        leaf flow-info {
          type string;
          description
            "ACL name that refers to the flow, if any.";
        }
        uses session-path-verification-statistics;
      }

Why "ACL name"? Why does this have anything to do with ACLs?
[Michael]: I would to discuss with other authors to make it more clearly.
 (But if it does, you need to expand ACL.)

---

Superfluous "YANG module of OAM" right at the end of the YANG module
[Michael]: Indeed, it need to be fixed.
---

5.  CL model applicability

Spell out "CL" or use actual model name.
[Michael]: Agree
---

6.  Security Considerations

   TBD.

I don't think that will be good enough :-) You can model on the text in draft-ietf-lime-yang-oam-model
[Michael]: Indeed, I will fix it in next version.
---

Section 8

   The authors of this document would like to thank Greg Mirskey and
   others

Greg spells it "Mirsky".
The others wished to remain anonymous?
[Michael]: Thanks for point it, we will fix it.
---

It is possible that all your references are normative, but I doubt it.
You should go through them and work out which provide information and which are required reading in order to process this document.
[Michael]: Agree, we would like to optimize the reference section.
> -----Original Message-----
> From: Lime [mailto:lime-bounces@ietf.org] On Behalf Of Ron Bonica
> Sent: 19 January 2017 16:12
> To: lime@ietf.org; 
> draft-ietf-lime-yang-connectionless-oam.all@ietf.org
> Subject: [Lime] WGLC: draft-ietf-lime-yang-connectionless-oam-03
> 
> Folks,
> 
> This message begins a Working Group Last Call on draft-ietf-lime-yang- 
> connectionless-oam-03. Please submit comments by February 3, 2017.
> 
>
Ron
> 
> _______________________________________________
> Lime mailing list
> Lime@ietf.org
> https://www.ietf.org/mailman/listinfo/lime

_______________________________________________
Lime mailing list
Lime@ietf.org
https://www.ietf.org/mailman/listinfo/lime