Subject: Re: [pim] draft-ietf-pim-yang-02 WGLC

From: Michael McBride <[email-masked]>
Date: Wed, 7 Sep 2016 19:14:47 +0000

Hi Xufeng,

Please revise the draft and then we will send to iesg. It may be difficult for people to get motivated to review our yang drafts so we will rely upon your, the other draft authors and the yang doctors, yang expertise for thorough review. 

thanks,
mike

-----Original Message-----
From: Xufeng Liu [mailto:[email-masked]] 
Sent: Monday, August 29, 2016 6:51 AM
To: 'Stig Venaas'; Michael McBride
Cc: [email-masked]
Subject: RE: [pim] draft-ietf-pim-yang-02 WGLC

Hi Stig,

Thanks for the valuable comments. We will check them and revise the draft.

We certainly hope more review comments.

Regards,

- Xufeng

> -----Original Message-----
> From: pim [mailto:[email-masked]] On Behalf Of Stig Venaas
> Sent: Thursday, August 25, 2016 6:17 PM
> To: Michael McBride <[email-masked]>
> Cc: [email-masked]
> Subject: Re: [pim] draft-ietf-pim-yang-02 WGLC
> 
> Here are some quick comments from me.
> 
> There is a TODO list (section 6). Will you do more work on this, or should
the
> section be removed?
> 
> Should BFD be in the model like this? What is the local-multiplier?
> 
> It looks like queue size and overflow statistics are per VRF? Is it normal
that
> implementations do this per VRF? Or it it global?
> 
> Shouldn't there be stats for DF election messages.
> 
> "Grouping" is spelled as "Gouring" or "Gouping" several places.
> 
> Stig
> 
> 
> 
> 
> On Wed, Aug 17, 2016 at 10:19 AM, Stig Venaas <[email-masked]> wrote:
> > Hi
> >
> > I expect the contents of this model is quite important for most
> > vendors that have a pim implementation, and also to a lot of
> > operators. So I hope you all can have a close look at this and see how
> > it matches your implementations and whether it matches operators'
> > needs. It is expected that vendors will need to augment the model in
> > many cases though, as the model tries to address what is common
> > between vendors. I'm hoping that what we have is good as a handful of
> > vendors have been involved in the design team, but we would like you
> > to have a careful look at it.
> >
> > Note that we're doing this last call already now, as there is pressure
> > from operators to get IETF models.
> >
> > Stig
> >
> >
> > On Thu, Aug 11, 2016 at 7:12 PM, Michael McBride
> > <[email-masked]> wrote:
> >> Hello,
> >>
> >>
> >>
> >> Today begins a 2 week WGLC for draft-ietf-pim-yang-02. Please give it
> >> a review and comment about its readiness to be sent to the IESG.
> >>
> >>
> >>
> >> https://www.ietf.org/id/draft-ietf-pim-yang-02.txt
> >>
> >>
> >>
> >> thanks,
> >>
> >> mike
> >>
> >>
> >> _______________________________________________
> >> pim mailing list
> >> [email-masked]
> >> https://www.ietf.org/mailman/listinfo/pim
> >>
> 
> _______________________________________________
> pim mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/pim

---

From: "Xufeng Liu" <[email-masked]>
Date: Wed, 7 Sep 2016 15:45:26 -0400

Hi Mike,

Will do within a couple of weeks. I have been busy in other activities in
the past weeks, but I think that I can get some time to do the revision
within the next couple of weeks. Will let the goup know once done.

Thanks,

- Xufeng

> From: Michael McBride [mailto:[email-masked]]
> Sent: Wednesday, September 7, 2016 3:15 PM
> To: Xufeng Liu <[email-masked]>; 'Stig Venaas'
<[email-masked]>
> Please revise the draft and then we will send to iesg. It may be difficult
for
> people to get motivated to review our yang drafts so we will rely upon
your, the
> other draft authors and the yang doctors, yang expertise for thorough
review.
> > > operators. So I hope you all can have a close look at this and see
> > > how it matches your implementations and whether it matches operators'
> > > between vendors. I'm hoping that what we have is good as a handful
> > > of vendors have been involved in the design team, but we would like
> > > you to have a careful look at it.
> > >
> > > Note that we're doing this last call already now, as there is
> > > pressure from operators to get IETF models.
> > >
> > > Stig
> > >
> > >
> > >> Today begins a 2 week WGLC for draft-ietf-pim-yang-02. Please give
> > >> it a review and comment about its readiness to be sent to the IESG.
> > >>
> > >>
> > >>

---

From: Stig Venaas <[email-masked]>
Date: Wed, 7 Sep 2016 13:22:38 -0700

Hi

I will want to review it in more detail as well, I may have more
comments. I hope others in the group can have a look also.

Stig

---

From: Stig Venaas <[email-masked]>
Date: Fri, 16 Sep 2016 14:27:25 -0700

Hi

I now took some time to review the draft more carefully. Here are
comments in addition to what I posted earlier for the WGLC.

First some potential issues with the models.

I see for hello-interval, jp-interval etc. the data type is
timer-value which can also be infinity and no-expiry. Isn't
this type to be used for something counting down? These intervals
would always be some number of seconds and won't change unless the
configuration changes.

Also I see the PIM-SM spec calls them periods, like hello-period
and jp-period. So maybe those names would be better?

I also see "expiration" is usually timer-value, but I see for
the multicast-route-attributes we have expiration that is uint32.
2.1

In the BSR model I see:

leaf hash-mask-length{
           type uint8 {
             range "0..32";
           }

The length can be larger for IPv6.

It might be useful to add some text explaining what the multipliers do.
I know some implementations use this concept, but it is not in any IETF
documents that I know. So good to explain it somewhere in this document.

The issues below are more minor.

   The model covers PIM Sparse Mode [RFC4601], including the Source-
   Specfic subset [RFC3569], Dense Mode [RFC3973], and Bi-directional
   PIM [RFC5015].

Typo "Specific"

   The PIM extensions represented in the model include BSR [RFC5059] and
   Anycast RP [RFC4610].

I think it should say "Anycast-RP".

The language in these paragraphs in 2.1 seems to say that this is still
work in progress, and should probably be changed a bit. The last
paragraph is mostly ok, but I think it should say "specified in separate
documents", rather than "future Internet Drafts".

2.4 starts with the words "The current draft". It should perhaps say
"This document".

What to do with section 3? You probably should change the language a
bit, and also not have a TODO list. You could perhaps make it talk about
the scope of the model, limitations, and what is left out?

In 4.1 you need to replace "our current direction" with some other
language. Maybe some of the text can be removed.

In 4.3 it says "ipv6-anycast-rip" in 2 places. Also 2 places it says
"range-poligy".

In 4.5 I see a couple of "offer-multipler".

For all these typos, please search and look for additional occurrences.
I see "multipler" several places in 5.1 too.

In 5.1 it says a couple of places"configuraiont". Also in description
of DR priority, perhaps capitalize DR.

Also I see a "list neighbor-ipv4", where it should be ipv6.

In the descriptions it says "address family" in some places, and "af"
other places. Maybe try to make it consistent. Consider "AF" instead of
"af"?

"The number of bsr messages" should have BSR capitalized.

Some places in 5.2 it says "Gouring".

I think it should say "IPv4" and "IPv6" in the descriptions, rather
than "ipv4 and ipv6". I see both used.

In 5.3, I think you should capitalize RPT, SPT and DR in the
descriptions. Also I see a description where it says "Spars Mode". I
see various ways of writing IPv4 and IPv6 in the descriptions. I think
they should be written like I just did. It says "Gouping" a few places.

In the description of passive it says:
            "Specifies that no PIM messages are sent out of the PIM
             interface, but the interface can be included in a multicast
             forwarding entry.";
Doesn't it also mean that no PIM messages are accepted?

In 5.5 there are two descriptions ending with "..". I also see some of
the same issues as mentioned above. Capitalization and typos like
"gouping" and "multipler".

Section 7
Please try to think if there could be any issues.

Section 8
I'm not sure if there are IANA considerations. If there are none, then
write that.

The 4601 reference should be updated to RFC 7761.

Stig

---

From: "Xufeng Liu" <[email-masked]>
Date: Fri, 23 Sep 2016 10:28:51 -0400

Hi Stig and All,

I'm addressing the following comments. The model changes and draft updates
have been checked into GitHub:
https://github.com/mcallisterjp/pim-yang.

I'm also looking at your other comments from a separate email. Will have the
responses soon.

Thanks,

- Xufeng

the
> section be removed?
[Xufeng] Removed this section.

> Should BFD be in the model like this? What is the local-multiplier?
[Xufeng] The local-multiplier is from [email-masked], with the
following description:
   local-multiplier
           This is the detection time multiplier as defined in
           [RFC5880].

  We currently use a grouping bfd-grouping-base-cfg-parms from the ietf-bfd
model. 
  If agreed, we can use a simpler grouping  bfd-client-base-cfg-parms which
only contain an enable/disable flag.
  Please provide feedbacks about which way we prefer.

that
> implementations do this per VRF? Or it it global?
[Xufeng] Our implementation can do this both per VRF and global. The current
  VRF model is based on [email-masked] and
  [email-masked]. I think that the current VRF
  modeling idea is to model the global properties with a special VRF
  instance, so that the global statistics are also covered by our model.

> "Grouping" is spelled as "Gouring" or "Gouping" several places.
[Xufeng] Fixed, along with some other typos

---

From: Xufeng Liu <[email-masked]>
Date: Fri, 30 Sep 2016 23:29:29 +0000

Hi Stig and All,

Thanks for the comments. I have updated the models and drafts based on these comments. The updated draft and model files are at: https://github.com/mcallisterjp/pim-yang. Any feedbacks will be appreciated.

The update items include the responses to Stig's previous emails, and the comments below.

Thanks,

- Xufeng

> From: Stig Venaas [mailto:[email-masked]]
> Sent: Friday, September 16, 2016 5:27 PM
> To: Xufeng Liu <[email-masked]>
> Cc: Michael McBride <[email-masked]>; [email-masked]
> I now took some time to review the draft more carefully. Here are comments in
> addition to what I posted earlier for the WGLC.
> 
> I see for hello-interval, jp-interval etc. the data type is timer-value which can
> also be infinity and no-expiry. Isn't this type to be used for something counting
> down? These intervals would always be some number of seconds and won't
> change unless the configuration changes.
[Xufeng] The "xxx-interval" leaves are the number of seconds that won't change unless the configuration changes.

> 
> Also I see the PIM-SM spec calls them periods, like hello-period and jp-period. So
> maybe those names would be better?
[Xufeng] RFC7761 uses Hello_Period and Keepalive_Period. It also uses Override_Interval and Assert_Override_Interval.
The description of Hello_Period is "Periodic interval for Hello messages".

Most other models use "interval" instead of "period":
bgp: hold-time, keepalive-interval, minimum-advertisement-interval
bfd: min-interval, 
ietf-routing: max-rtr-adv-interval
isis: hello-interval
ldp: hello-interval, session-ka-interval
te: lsp-install-interval, lsp-cleanup-interval
ospf: hello-interval, restart-interval
rip: update-interval, holddown-interval
vrrp: master-down-interval
igmp: query-interval

> 
> I also see "expiration" is usually timer-value, but I see for the multicast-route-
> attributes we have expiration that is uint32.
[Xufeng] Changed to "timer-value" for consistency.

> The length can be larger for IPv6.
[Xufeng] Changed to 128.

> documents that I know. So good to explain it somewhere in this document.
[Xufeng] Juniper and Alcatel-Lucent use the multiplier to calculate
holdtime, so we have it as an optional feature. Some explanations have
been added.
>    Specfic subset [RFC3569], Dense Mode [RFC3973], and Bi-directional
[Xufeng] Fixed.

> I think it should say "Anycast-RP".
[Xufeng] Changed.

> The language in these paragraphs in 2.1 seems to say that this is still work in
> progress, and should probably be changed a bit. The last paragraph is mostly ok,
> but I think it should say "specified in separate documents", rather than "future
> Internet Drafts".
> 
[Xufeng] Reworded.

> 2.4 starts with the words "The current draft". It should perhaps say "This
> document".
> 
[Xufeng] Fixed.

> What to do with section 3? You probably should change the language a bit, and
> also not have a TODO list. You could perhaps make it talk about the scope of the
> model, limitations, and what is left out?
> 
[Xufeng] Fixed.

> In 4.1 you need to replace "our current direction" with some other language.
> Maybe some of the text can be removed.
> 
[Xufeng] Please check if the modified wording is ok. Please comment if not.

> In 4.3 it says "ipv6-anycast-rip" in 2 places. Also 2 places it says "range-poligy".
> 
[Xufeng] Fixed.

> In 4.5 I see a couple of "offer-multipler".
[Xufeng] Fixed.

> I see "multipler" several places in 5.1 too.
[Xufeng] Checked and fixed.

> In 5.1 it says a couple of places"configuraiont". Also in description of DR priority,
> perhaps capitalize DR.
> 
[Xufeng] Fixed.

> Also I see a "list neighbor-ipv4", where it should be ipv6.
[Xufeng] Fixed.

> other places. Maybe try to make it consistent. Consider "AF" instead of "af"?
> 
[Xufeng] Made them consistent, using "address family" in all places.

> "The number of bsr messages" should have BSR capitalized.
[Xufeng] Fixed.

> Some places in 5.2 it says "Gouring".
[Xufeng] Fixed.

> I think it should say "IPv4" and "IPv6" in the descriptions, rather than "ipv4 and
> ipv6". I see both used.
> 
[Xufeng] Searched and fixed those found.

> In 5.3, I think you should capitalize RPT, SPT and DR in the descriptions. Also I
> see a description where it says "Spars Mode". I see various ways of writing IPv4
> and IPv6 in the descriptions. I think they should be written like I just did. It says
> "Gouping" a few places.
> 
[Xufeng] Fixed.

> Doesn't it also mean that no PIM messages are accepted?
[Xufeng] Right. Clarified the description.

> In 5.5 there are two descriptions ending with "..". I also see some of the same
> issues as mentioned above. Capitalization and typos like "gouping" and
> "multipler".
> 
[Xufeng] Fixed.

> Please try to think if there could be any issues.
[Xufeng] Added some descriptions.

> Section 8
> I'm not sure if there are IANA considerations. If there are none, then write that.
> 
[Xufeng] Added this section.

> The 4601 reference should be updated to RFC 7761.
[Xufeng] Fixed.