Re: [Bier] WG LC on https://datatracker.ietf.org/doc/draft-ietf-bier-pim-signaling/

["Bidgoli, Hooman (Nokia - CA/Ottawa)" <[e-mail]>]

Thanks Stig!

Inline HB>

Regards

Hooman

-----Original Message-----
From: Stig Venaas <[e-mail]> 
Sent: Tuesday, July 28, 2020 4:56 PM
To: Mankamana Mishra (mankamis) <[e-mail]>
Cc: [e-mail]; Bidgoli, Hooman (Nokia - CA/Ottawa) <[e-mail]>; BIER WG <[e-mail]>; Ijsbrand Wijnands <[e-mail]>; [e-mail]; Jeffrey (Zhaohui) Zhang <[e-mail]>; Antoni Przygienda <[e-mail]>
Subject: Re: [Bier] WG LC on https://datatracker.ietf.org/doc/draft-ietf-bier-pim-signaling/

Hi

It looks like most of the issues I found have been resolved now. But most importantly, the IANA Considerations still say that there are none. This needs to be fixed!

HB> Done, Add it for the join attribute TLV that you got away from in the other draft ;)

I see a couple of places where it says PIM signaling message or PIM signaling packet. I think it should say PIM join/prune message, or PIM join/prune packet.,

HB>
Changed to PIM join/prune in section 3 because this was the PIM domain join/prune. "it will generate a PIM join/prune packet toward its attached PIM domain."
Any where else that the pim signaling is left is actually the BIER signaling the join/prune through the bier network as such I wanted it to be clear that these are signalling packet and not extending the PIM over the BIER domain.

:


Regards,
Stig

On Tue, Sep 24, 2019 at 10:51 AM Stig Venaas <[e-mail]> wrote:
>
> Hi
>
> This is good work and I generally support the document. There are 
> however some issues that need to be addressed. I also think it would 
> be good to get this reviewed in the pim WG, perhaps you can ask for 
> reviews on the pim list.
>
> I mostly have editorial comments that are easy to address. But at a 
> high level I think some more text about pim is needed.
>
> PIM relies on hello messages to know what capabilities a router has, 
> e.g. whether it support Join attributes, whether it support BIDIR etc. 
> I think you need to point out what capabilities are assumed to be 
> present. Obviously it is assumed that Join attributes are supported. 
> It may also be good to point out that there is no J/P suppression as 
> the J/P is only sent to the target, and not to other routers. Also 
> point out that only J/P messages are used, and that there is no assert 
> processing.
>
>
> Below are the editorial comments. Please also check the "nits"
> tool. It complains about missing references, and it cannot find the 
> "Authors' Addresses" section. The heading is missing.
>
> Also Section 7 must be updated. There is an IANA action, but it says 
> there are none!
>
> The abstract is rather long. Suggest removing some of the text that 
> explains the general operation of BIER.
>
> Some of the references might need some work. In section 2 it says "RFC 
> 2119 [RFC2119]". Should it be just "[RFC2119]"? Also the reference is 
> missing.  In 2.1 it says "[I-D. rfc8279]" which also needs to be 
> fixed. Most of the other references look fine.
> In 3.1 there seems to be a reference to 8279 without brackets though.
>
> In 2.1, BFR definition. Missing space after ".".
>
> BFIR definition: It says "insert the BM into the packet", but I think 
> it might be better to say that it performs BIER encapsulation. The 
> term BM is not defined. It says "plain" instead of "plane".
>
> BFER defintion: Do we have to say that BFER is a BFR? In that case, we 
> should also say that BFIR is a BFR. It says "plain" instead of "plane".
>
> IBBR and EBBR, might it be good to include "signaling" in the name, 
> like Ingress Signaling BIER Router and Egress Signaling BIER Router? I 
> want it to be very clear that ingress and egress are not related to data.
>
> In "Figure 1" I think "bfir" and "bfer" should be upper case.
>
> In 3.1:
> "weather" should be "whether".
> "located on" should probably be "located in".
>
> In 3.1 there is this paragraph that I think need some changes.
>
>    After discovering the EBBR and its BFR-ID (flooded via IGP BIER
>    extension), the IBBR will construct a PIM Join Attribute encoded as
>
> It says that BFR-ID is flooded via IGP BIER extension. That isn't 
> necessarily true, do we need to explain how the BFR-ID is discovered?
>
>    TLVs into the Source Address field of the PIM Join Message as per
>
> Isn't it just one TLV? I think you can skip saying that it is a TLV in 
> the source address field. Just say that it includes a new PIM Join 
> Attribute in the Join/Prune message. Also note that we should really 
> say Join/Prune, not just Join.
>
>    [RFC5384] and include it in PIM signaling message. Two new "BIER
>
> I think it is better to be clear and say PIM Join/Prune message.
>
>    IBBR" attributes are define and explained in upcoming section. The 
> s/define/defined
>
>    PIM Join Attribute is used on EBBR to obtain necessary bier 
> s/bier/BIER
>    information to build its multicast states. In addition the IBBR will
>    change the PIM signaling packet source IP address to its BIER prefix
>    address (standard PIM procedure). It will also keep the destination
>    address as the well known multicast IP address. It then will
>    construct the BIER header. The signaling packet, in this case the PIM
>    join/prune packet, is encapsulated in the BIER header and transported
>    through BIER domain to EBBR.
>
> The language at the end here makes it sound like the PIM J/P is being 
> forwarded and that the J/P is being modified. But J/P messages are 
> sent hop-by-hop. Each router originates a new J/P. For instance, a 
> router may receive a J/P for multiple sources. These sources may have 
> different RPF neighbors on the receiving router and hence be split 
> into separate join messages. This could happen on the IBBR as well.
>
> The lasts paragraph in section 3 says:
>    The IBBR will track all the PIM interfaces on the attached PIM domain
>    which are interested in a certain (S,G). It creates multicast states
>    for arriving (S,G)s from PIM domain, with incoming interface as BIER
>    "tunnel" interface and outgoing interface as the PIM domain
>    interface(s) on which PIM Join(s) were received on.
>
> The IBBR is a PIM router and what you are describing is standard PIM 
> behavior, so I think this is a bit redundant. It might be goot to 
> stress that OIFs are adding according to standard PIM behavior. The 
> only special here is the RPF interface.
>
> Section 3.1.4:
> In heading Pim/PIM
> bier/BIER
>
> It says "PIM Join Attribute [RFC5384] is used." I think it might be 
> good to say that "a new PIM Join Attribute is used". And then also 
> this sentence "The PIM Join Attribute format is as follow:" should 
> perhaps be The new PIM Join Attribute format is defined as follows:".
>
> s/Ipv4/IPv4
> s/Ipv6/IPv6
>
> In the format I think it might be good to not just show "BIER info", 
> but show the formatting of prefix, SD and BFR-ID explicitly. Also, it 
> should state clearly that these are the prefix, SD and BFR-id of the IBBR.
>
> In 3.1.4.1 why not say PIM Join/Prune packet instead of PIM signaling?
>
> In 3.3.:
>    After receiving the BIER packet and determining this packet is a
>    signaling packet, EBBR will remove the BIER header from PIM packet.
>    The Received PIM join/prune Signaling packet is processed as if it
>    were received from neighbors on a virtual interface, (i.e. as if the
>    pim adjacency was presents, regardless of the fact there is no
>    adjacency)
> Wouldn't the router remove the BIER header simply because the BFR-id 
> in the BIER header matches its own BFR-id? There are some grammar 
> issues and a missing period in this paragraph.
>
> In this paragraph:
>    With same token the EBBR creates a multicast state with incoming
>    interface as same interface that PIM join packet was forwarded and
>    outgoing interfaces of BIER tunnel to BFER identified with BFIR-id
>    and its corresponding Sub-Domain obtained from the BIER header or via
>    PIM Join Attributes added to the PIM signaling packet by the IBBR.
>
> I'm not sure what "With same token" means here. Also, it not should 
> say that PIM join packet was forwarded. I would say that state is 
> created according to the PIM specification, and just describe the BIER OIF state.
> The specific OIF state may be implementation specific though. Perhaps 
> this is sufficiently described in the paragraph that comes right 
> after? It is also well described in 4.1.
>
> In section 5: s/LEAFs/leaves? Should it be EBBRs?
>
> In section 6:
> s/vrf/VRF
> "it is determine" should be "it is determined".
> Replace "PIM signaling message" with "PIM Join/Prune message"?
>
> Section 8:
> The 4601 reference should be to RFC 7761.
>
> Appendix A:
> The header seems to not be formatted correctly.
> "This section" should be "This appendix"?
>
> In A.1: "is consist" should be "consists".
>
> In Figure 2: s/bier/BIER
> Also in the text in A.2.2.
>
> IN A.3.1
> s/Bier/BIER
> s/bier/BIER
> s/tlv/TLV
>
> Stig