Re: [bess] Shepherd review on draft-ietf-bess-mvpn-mib-01

Mach,

Thank you very much for the review. Please see zzh> below.

> -----Original Message-----
> From: BESS [mailto:bess-bounces@ietf.org] On Behalf Of Mach Chen
> Sent: Tuesday, December 22, 2015 3:18 AM
> To: draft-ietf-bess-mvpn-mib@tools.ietf.org
> Cc: bess-chairs@ietf.org; bess@ietf.org
> Subject: [bess] Shepherd review on draft-ietf-bess-mvpn-mib-01
> 
> Hi Authors,
> 
> I am requested (by the WG chairs) to shepherd this draft, here are my
> shepherd review comments on this document.
> 
> 
> 1. To make the document more readable, expect for the well-known
> abbreviations, there are a lot of abbreviations (e.g., MVRF, I-/S-PMSI,
> etc.) that should be expanded when first use.

Zzh> Will do.

> 
> 2. Abstract and introduction section
> 
> s/ multicast in MPLS/BGP-based Layer-3 VPN (MVPN)/ Multicast in BGP/MPLS
> in IP VPN (MVPN),  to align with the definition in RFC6513

Zzh> will do.

> 
> 3.
> Section 2.1
> 
>    - mvpnIpmsiTable/Entry
> 
>      This table contains all advertised or received intra-as I-PMSIs.
>      With PIM-MVPN, it is applicable only when BGP-Based Autodiscovery
>      of MVPN Membership is used.
> 
>    - mvpnInterAsIpmsiTable/Entry
> 
>      This table contains all advertised or received inter-as I-PMSIs.
>      With PIM-MVPN, it is applicable only when BGP-Based Autodiscovery
>      of MVPN Membership is used.
> 1) For each table, why is there a "/Entry" followed?  To avoid confusion,
> I'd suggest to remove them if there is no special intention or meaning.

Zzh> It is meant to say that xxxEntry are xxxTable are associated and their usage is explained in the paragraphs. I will remove the "/Entry"

> 
> 2 )In addition, from the above text, I have the feeling that a
> mvpnIpmsiTable or mvpnInterAsIpmsiTable can only contain advertised or
> received I-PMSIs, but cannot contain both. Is this the intention?

Zzh> Both. I'll change "or" to "and".

> 
> 
> 4. Section 2.2, the LAST-UPDATED, ORGANIZATION, CONTACT-INFO and the
> Revision history should be updated to reflect the latest status.

Zzh> I had expected this to be updated at the last moment (e.g. at publication), but I am not sure. Will do whatever is the right procedure.

> 
> 5. mvpnMvrfNumber OBJECT-TYPE
>      SYNTAX        Unsigned32
>      MAX-ACCESS    read-only
>      STATUS        current
>      DESCRIPTION
>          "The total number of MVRFs for IPv4 or IPv6 or mLDP
>           C-Multicast that are present in this device."
> 
> Should the "or" be changed to "and"?

Zzh> It's for any of those types. Someone said we should use "or". I am open to suggestions.

> 
> 6.
> There are several places in draft say the following or similar:
> "An entry in this table is created for every MVRF in the device."
> I'd suggest to replace the "every" as "each".

Zzh> Will do.

> 
> 7.
> Page 10:
> mvpnGenCmcastRouteProtocol OBJECT-TYPE
>      SYNTAX        INTEGER { pim (1),
>                              bgp (2)
>                            }
>      MAX-ACCESS    read-write
>      STATUS        current
>      DESCRIPTION
>          "Protocol used to signal C-multicast states across the
>           provider core.
> 
> s/ Protocol /The protocol is

Zzh> Will do.

> 
> 8.
> Page 13:
> mvpnBgpGenVrfRtImport OBJECT-TYPE
>      SYNTAX             MplsL3VpnRouteDistinguisher
>      MAX-ACCESS         read-write
>      STATUS             current
>      DESCRIPTION
>          "The VRF Route Import Extended Community that this device
>           adds to unicast vpn routes that it advertises for this mvpn."
>      ::= { mvpnBgpGeneralEntry 2}
> 
>   mvpnBgpGenSrcAs      OBJECT-TYPE
>      SYNTAX            Unsigned32
>      MAX-ACCESS        read-only
>      STATUS            current
>      DESCRIPTION
>          "The Source AS number in Source AS Extended Community that this
>           device adds to the unicast vpn routes that it advertises for
>           this mvpn."
> 
> Why should the "Source", "Extended", and "Community" be upper case? If
> there is no special intention, suggest to change to lower case. You may
> need to look through the whole document to do a text clean up.

Zzh> That's per RFC 6513.

> 
> 9.
> Section 3, Security consideration.
> 
> Given that the document introduces some read-write objects, I don't think
> that the current statement of "This document does not introduce new
> security risks." will pass the IESG review. I'd suggest the authors to
> enhance the security consideration section. For the mib security
> consideration, you may refer to some existing mid documents (e.g,
> https://tools.ietf.org/html/draft-ietf-mpls-tp-oam-id-mib-11) that have
> already passed the IESG review. Also, you may refer to some history
> discussions on a mib security consideration (e.g.,
> https://datatracker.ietf.org/doc/draft-ietf-trill-oam-mib/history/ ).

Zzh> This seems to be no different from CLI configuration/monitoring? But I will study those past discussions.

> 
> 10.Please make sure that the MIB Modules are compiled cleanly.

Zzh> I did use http://www.simpleweb.org/ietf/mibs/validate/. Did you notice any problem?

> 
> 11. BTW, I saw the WG chairs had issued the IPR poll, and only Jeffery
> replied, to progress this draft, the authors and contributors have to
> respond to the IPR poll.

Zzh> Working on that.

> 
> 
> Happy Holidays!

Zzh> Thanks! Part of the reason for this late response :-)

Jeffrey

> 
> Best regards,
> Mach
> 
> 
> 
> 
> 
> _______________________________________________
> BESS mailing list
> BESS@ietf.org
> https://www.ietf.org/mailman/listinfo/bess