Re: [Gen-art] Hi, Gen-art Telechat review of draft-ietf-mpls-mldp-node-protection-06 - follow up

[IJsbrand Wijnands]

Thanks Elwyn!

Ice.

> On 16 Sep 2015, at 15:32, Elwyn Davies  wrote:
> 
> Hi Ice.
> 
> I had a quick look through the updates in -07 and that has addressed all the 
> points below.  Definitely good to go now.
> 
> Cheers,
> Elwyn
> 
>> I am the assigned Gen-ART reviewer for this draft. The General Area
>> Review Team (Gen-ART) reviews all IETF documents being processed
>> by the IESG for the IETF Chair. Please wait for direction from your
>> document shepherd or AD before posting a new version of the draft.
>> 
>> For more information, please see the FAQ at
>> 
>> .
>> 
>> Document: draft-ietf-mpls-mldp-node-protection-06.txt
>> Reviewer: Elwyn Davies
>> Review Date: 2015/09/15
>> IETF LC End Date: 2015/09/08
>> IESG Telechat date: 2015/09/17
>> 
>> Summary: Ready for publication on standards track. Thanks for your generous 
>> comments on my review and the updated version -06 which fixes almost all of 
>> the issues. The nits below are mostly suggestions related to the updated 
>> text apart from the last one on s2.3 which got missed.
>> 
>> Major issues:
>> None
>> 
>> Minor issues:
>> None
>> 
>> Nits/editorial comments:
>> s1: The new text referring to the need for capability negotiation is not 
>> easy to parse. Suggested alternative:
>> OLD:
>> In order for a node to be protected, the protecterd node, the PLR and
>> the MPT MUST support the procedures as described in this draft.
>> Detecting the protected node, PLR and MPT support these procedures is
>> done using [RFC5561].
>> NEW:
>> In order to allow a node to be protected against failure, the LSRs providing
>> the PLR and the MPT functionality as well as the protected node MUST
>> support the functionality described in this document. RSVP capability
>> negotiation [RFC5561] is used to signal the availability of the functionality
>> between the participating nodes; these nodes MUST support capability
>> negotiation.
>> END
>> 
>> s2, last para: s/This because/This is because/
>> 
>> s2.1, last para; s2.2, last para: s/Procedures how to setup/The procedures 
>> for setting up/
>> 
>> s2, s2.2 and s3: s/this draft/this document/ (3 places) [A 4th instance is 
>> replaced in the suggested text for s1 above.]
>> 
 s2.2:  If I understand correctly, the bypass LSPs have to be bidirectional 
 (or they could be two unidirectional ones) unlike those in s2.1 which will 
 be unidirectional.  I think this ought to be mentioned, assuming I am 
 right - and presumably one could do a bit of optimisation in setup.  This 
 has some knock-on effects as regards what happens when the node fails.  I 
 wonder if there should be some explanation of what happens in an extra 
 sub-section in s4 - just that the various LSRs need to think about what 
 role they are playing depending on where the incoming packets are coming 
 from, I guess.
>>> Ice: Yes, that is a good observation about unidirectional and bidirectional 
>>> LSPs. I’ll add a node to make that clear.
>> The fixes for that are fine and helpful IMO.
>>> Since the MPT will receive packets with the MPLS label it originally 
>>> expected, it does not really care where the packets are coming from. So I’m 
>>> not sure anything else needs to be added here.
>>> 
>> Probably right. Actually the fact that the bypass LSPs are bidirectional 
>> does sort out the differentiation of roles anyway. Incoming = MPT, Outgoing 
>> = PLR. The note could be extended to mention this.
>> 
>> s2.3:
>>>   Num PLR entry: Element as an unsigned, ***non-zero*** integer followed
>>>   by that number of "PLR entry" fields in the format specified
>>>   below.
>> Per the discussion of my last call comments, the Num PLR entry can be zero.
>> 
> 

___
Gen-art mailing list
Gen-art@ietf.org
https://www.ietf.org/mailman/listinfo/gen-art

Re: [Gen-art] Gen-art LC review of draft-ietf-mpls-mldp-node-protection-05

[IJsbrand Wijnands]

Hi Elwyn,

First of all, thanks for the great comments. See a few responses inline below. 
I’ll submit the new version now, hope it addresses all your comments.


> In particular, there doesn't seem to be any explanation of why the PLRs and 
> MPTs have to be directly connected to the protected node (RFC 6388 is happy 
> to find non-directly connected nodes) and I cannot see why direct connection 
> is vital to this specification.  This may of course because I am not an MPLS 
> afficionado!  There are, however, a considerable number of language

Ice: mLDP consults the RIB in order to find the next LDP neighbor to reach the 
Root node. The RIB only has information about the directly connected nodes and 
does not have visibility beyond that. If mLDP would consult the IGP (OSPF/ISIS) 
database I might be able to figure that out, but that is outside the scope of 
the document. I’ll add a note to make that clear.


> nits which I have documented - another editorial pass by a reviewer with 
> English mother tongue after fixing the nits I found would probably help.
> 
> Major issues:
> None. 
> 
> Minor issues:
> s1:  This specification uses LDP capability negotiation (RFC 5561).  The 
> introduction should be clear that nodes involved in the node protection 
> scheme MUST support RFC 5561 negotiations.

Ice: Fixed.

> 
> s1, para 2:  There is a very dangerous slang phrase here: "most ... should 
> just work"!   Does this have the literal meaning that there are some 
> circumstances in which they might NOT work and/or there are other 
> capabilities that aren't enabled - if so, what are they? Or, I guess more 
> likely, the slang meaning that using this method will allow these 
> capabilities to work without additional effort.  Please use a non-slang 
> phrase and be more precise as to what will/won't work.

Ice: Fixed.

> 
> s2.1:
>>The upstream LSR in figure 1 is LSR1
>>because it is the ***first hop*** along the shortest path to reach the 
>> root
>>address.
>> 
> Does it have to be the 'first hop' (I read this as the first LSR on the path 
> from N towards the root)?
> RFC 6388 s2.4.1.1 allows the upstream to be any LSR along the path according 
> to some local selection policy.  Can't the PLR be any such upstream LSR?

Ice: Added text to explain this.

> 
> s2.2:  Following on from the previous comment, do LSR1..3 *have* to be 
> 'directly connected'?  Can they not be just one from the set of LSRs along 
> each separate LSR emanating from the root?

Ice: It may be possible to support this, but we need more information than we 
can currently find in the RIB. It will significantly complicate the solution. 
I’ve made this outside the scope of the document as at this point I don’t think 
we need that support.

> 
> s2.2:  If I understand correctly, the bypass LSPs have to be bidirectional 
> (or they could be two unidirectional ones) unlike those in s2.1 which will be 
> unidirectional.  I think this ought to be mentioned, assuming I am right - 
> and presumably one could do a bit of optimisation in setup.  This has some 
> knock-on effects as regards what happens when the node fails.  I wonder if 
> there should be some explanation of what happens in an extra sub-section in 
> s4 - just that the various LSRs need to think about what role they are 
> playing depending on where the incoming packets are coming from, I guess.

Ice: Yes, that is a good observation about unidirectional and bidirectional 
LSPs. I’ll add a node to make that clear. Since the MPT will receive packets 
with the MPLS label it originally expected, it does not really care where the 
packets are coming from. So I’m not sure anything else needs to be added here.

> 
> s2.3, next to last para:
>> To remove all PLR addresses belonging to
>>the encoded Address Family, an LSR N MUST encode PLR Status Value
>>Element with no PLR entry and "Num PLR entry" field MUST be set to
>>zero.
>> 
>  This is inconsistent with the statement in the "PLR Status Value Element" 
> figure:
> PLR entry (1 or more)
> I guess this last should be 'zero or more’.

Ice: Good catch!

> 
> Nits/editorial comments:
> == 
> Abstract: Need to expand LSR on first use.

Ice: Fixed.

> 
> Abstract and  s1: s/that has been built by/that have been built by the/ (2 
> places)

Ice: Fixed.

> 
> s1, para 1: Need to expand RSVP-TE and LFA on first use, and give references 
> for them - probably RFC 5420 for RSVP-TE and RFC 5286 for LDP LFA.

Ice: Done.

> 
> s1, para 2: s/Targetted/Targeted/

Ice: Ok.

> 
> s1.2: mLDP probably needs a reference - RFC 6388, I guess

Ice: Ok.

> 
> s1, para 2: the concept 'Target(t)ed LDP session' comes from RFC 7060 - 
> please add a reference.

Ice: Ok.

> 
> s2: It might be helpful to mention that the tLDP sessions are established 
> over the bypass LSPs from PLR to MPT and reiterate the comments in s1 about 
> how these LSPs might be established to avoid the protected node.

Ice: The tLDP 

Re: [Gen-art] Gen-art LC review of draft-ietf-mpls-mldp-in-band-wildcard-encoding-02

[IJsbrand Wijnands]

Hi Jari, Elwyn,

I missed Elwyn’s response during my travel the last couple of weeks, sorry 
about that.

I think Elwyn and I are in sync on the error handling. I’ll make a small 
modification that Elwyn suggest below. There are a few minor updates I like to 
do, will try to do that by the end of the week.

Thx,

Ice.

On 25 Nov 2014, at 16:12, Jari Arkko jari.ar...@ericsson.com wrote:

 Thanks for the review, Elwyn. Ice, is a new version upcoming? (The telechat 
 is in a moment… just wanted to check that we’ve not forgotten anything, it is 
 of course fine to collect various changes to a draft revision that you’ll 
 post after the call as well.)
 
 Jari
 
 On 14 Nov 2014, at 20:07, Elwyn Davies elw...@dial.pipex.com wrote:
 
 Hi, Ijsbrand/Ice,
 
 Thanks for the response.It seems there isn't much that can be done about the 
 error handling.  I shall escalate this problem as a generic check that we 
 need to make sure that unused extension systems defined in base protocols 
 have adequate error handling/reporting for when the extensibility gets used.
 
 Are you intending to generate a new version before the IESG review?  I see 
 this is now scheduled for 30 November, and it would be useful to know 
 whether there will be a new version to check out for the IESG review report.
 
 Cheers,
 Elwyn
 
 On 04/11/14 22:03, IJsbrand Wijnands wrote:
 Hi Elwyn,
 
 Summary: Almost ready.  There are a couple of clarifications around
 how IPv4 and IPv6 trees can or cannot be merged on a single MP-LSP
 that would be advantageous.  Also the error handling in the parent
 RFCs (6388, 6826) is a bit sketchy resulting in messy handling if
 an LSR that does not support the wildcard encoding is accidentally
 sent the TLVs from this draft.  I am not sure if this can be
 cleaned up (and maybe there is no thought to be a problem - I am
 not sufficiently expert in LDP signalling).
 
 I don’t think that is a problem, the wildcard encoding i.e. ‘all
 zero’s’, is an invalid value in implementation that don’t support it.
 So these routers would just drop the label mapping when this
 happens.
 
 Minor issues: IPv4 and IPv6 Multicast: I think it would be useful
 to add a short discussion of the fact that there are both IPv4 and
 IPv6 multicast trees.  Presumably an MP-LSP can only carry one or
 the other at a time, but I am unclear about whether this is the
 case. Also a note in s3.1 that it is either the IPv4 or IPv6
 address fields that are relevant and they are all zeroes in either
 case would clarify the usage further.
 
 We are not changing anything about how an LSP is used, either for
 IPv4 or IPv6. We’re only allowing *all* sources within a IPv4 or IPv6
 group to be forwarded down the LSP. I don’t think we should add
 anything here since we are not changing the default behaviour.
 
 I understand that.  To a novice reading the document it would be useful to 
 add a sentence after the first three paras of s3.2, something like:
  The wildcard scheme allows multicast traffic for multiple sources or
  multiple trees for either IPv4 or IPv6 traffic, but not both, to
  share a single MP-LSP according to the type of TLV used.
 
 RFC 5918 Typed Wildcard:  Is there anything special that has to be
 done if the Typed Wildcard is used?
 
 No, a typed wildcard does not encode any mLDP Source or Group address
 fields, so this draft does not apply.
 
 OK
 
 
 s3.3:  Is it possible to specify the error behaviour more
 concretely (i.e., what might  happen) in case an unadapted Ingress
 LSR gets a wildcard TLV?  It appears that RFC 6826 is rather
 underspecified as regards error handling - but I am not an expert
 on this area - it appears that the MP-LSP would get set up but to
 no good purpose which seems inappropriate.  Could an error status
 not be returned?
 
 For the Opaque in-band TLV's that mLDP FEC’s carry there is no error
 signalling defined in the base RFC. If you receive an opaque type for
 an in-band TLV and you don’t support it, you’ll just drop the label
 mapping. I agree this would have been useful to add a LDP capability
 in the base RPF 6826. Since this draft is just a minor update to the
 functionality as defined in 6826, it does make sense to do it now.
 
 
 Pity.  Do I understand correctly that the egress LSR will be none the wiser 
 that its request has been effectively silently ignored?  There doesn't seem 
 to be much that can be done at this stage.
 
 *** NOT SPECIFIC TO THIS DRAFT: There is a generic point here that when 
 extensible capabilities are put in place in protocols but not actually used 
 in the base protocol, we need to be sure that adequate error signalling is 
 put in place so that a later (mis)use of the extensible capabilities can be 
 detected and reported appropriately.  This is the second instance I have 
 seen in the recent past where there was inadequate error reporting 
 capability to  handle an extension scheme.
 
 
 Nits/editorial comments: s2, Definition of 'in-band signalling':
 Should also allow

Re: [Gen-art] Gen-art LC review of draft-ietf-mpls-mldp-in-band-wildcard-encoding-02

[IJsbrand Wijnands]

Hi Elwyn,

 Summary:
 Almost ready.  There are a couple of clarifications around how IPv4 and IPv6 
 trees can or cannot be merged on a single MP-LSP that would be advantageous.  
 Also the error handling in the parent RFCs
 (6388, 6826) is a bit sketchy resulting in messy handling if an LSR that does 
 not support the wildcard
 encoding is accidentally sent the TLVs from this draft.  I am not sure if 
 this can be cleaned up (and maybe there is no thought to be a problem - I am 
 not sufficiently expert in LDP signalling).

I don’t think that is a problem, the wildcard encoding i.e. ‘all zero’s’, is an 
invalid value in implementation that don’t support it. So these routers would 
just drop the label mapping when this happens.

 Minor issues:
 IPv4 and IPv6 Multicast: I think it would be useful to add a short discussion 
 of the fact that there are both IPv4 and IPv6 multicast trees.  Presumably an 
 MP-LSP can only carry one or the other at a time, but I am unclear about 
 whether this is the case. Also a note in s3.1 that it is either the IPv4 or 
 IPv6 address fields that are relevant and they are all zeroes in either case 
 would clarify the usage further.

We are not changing anything about how an LSP is used, either for IPv4 or IPv6. 
We’re only allowing *all* sources within a IPv4 or IPv6 group to be forwarded 
down the LSP. I don’t think we should add anything here since we are not 
changing the default behaviour.

 RFC 5918 Typed Wildcard:  Is there anything special that has to be done if 
 the Typed Wildcard is used?

No, a typed wildcard does not encode any mLDP Source or Group address fields, 
so this draft does not apply.

 
 s3.3:  Is it possible to specify the error behaviour more concretely (i.e., 
 what might  happen) in case an unadapted Ingress LSR gets a wildcard TLV?  It 
 appears that RFC 6826 is rather underspecified as regards error handling - 
 but I am not an expert on this area - it appears that the MP-LSP would get 
 set up but to no good purpose which seems inappropriate.  Could an error 
 status not be returned?

For the Opaque in-band TLV's that mLDP FEC’s carry there is no error signalling 
defined in the base RFC. If you receive an opaque type for an in-band TLV and 
you don’t support it, you’ll just drop the label mapping. I agree this would 
have been useful to add a LDP capability in the base RPF 6826. Since this draft 
is just a minor update to the functionality as defined in 6826, it does make 
sense to do it now.

 
 
 Nits/editorial comments:
 s2, Definition of 'in-band signalling': Should also allow for (S,*) - and 
 possibly (*,*), I believe.

Yes, I’ll change this.

 
 s3.4: s/PIM ASM/PIM-ASM/

Ok, 

Thanks for your review,

Ice.


___
Gen-art mailing list
Gen-art@ietf.org
https://www.ietf.org/mailman/listinfo/gen-art

Re: [Gen-art] review of draft-ietf-l3vpn-mldp-vrf-in-band-signaling-03.txt

[IJsbrand Wijnands]

Hi Francis,

I'll resolve the editorial nits.

Regarding the co-author list, you say (soft) limit, what does (soft) mean here?

Does one co-author need to drop of, or will it pass?

Thx,

Ice.

On 17 Feb 2014, at 16:12, Francis Dupont francis.dup...@fdupont.fr wrote:

 I am the assigned Gen-ART reviewer for this draft. For background on
 Gen-ART, please see the FAQ at
 
 http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq.
 
 Please resolve these comments along with any other Last Call comments
 you may receive.
 
 Document: draft-ietf-l3vpn-mldp-vrf-in-band-signaling-03.txt
 Reviewer: Francis Dupont
 Review Date: 20140211
 IETF LC End Date: 20140212
 IESG Telechat date: unknown
 
 Summary: Ready
 
 Major issues: None
 
 Minor issues: None
 
 Nits/editorial comments:
 - the number of authors is greater than the (soft) limit
 
 - 1 page 5: too many 'o' in 'co-oordination'
 
 - 1.2 pages 5  6: IMHO you should describe more terms in the Terminology
  section, e.g., UMH, RPA and even RD (as VRF is already in it)
 
 Regards 
 
 francis.dup...@fdupont.fr

___
Gen-art mailing list
Gen-art@ietf.org
https://www.ietf.org/mailman/listinfo/gen-art

Re: [Gen-art] Review: draft-ietf-mpls-ldp-p2mp-14

[IJsbrand Wijnands]

Hi Joel,

Thanks for the comments.

 Major issues:
As I read the document, status codes (and stauts TLVs) are for reporting 
 on the status of LSPs.  They are not for negotiating behaviors.  Thus, I find 
 it very strange that make-before-break behavior (section 8) is requested (by 
 a downstream device sending a request to an upstream device) by including a 
 specific status code in the request. Has this technique been used in MPLS LDP 
 before?

Well, the status codes and TLVs are not restricted by RFC 5036 for any 
particular purpose. The MBB procedures requires to signal information about the 
LSP which is not part of the FEC. The status TLV seems a good way to do that. 
Also, the status of the LSP can be viewed as 'waiting on upstream LDP neighbor 
to respond for MBB purpose'. So I think its ok.

 
 Minor issues:
The definition (section 1.2)  of MP2MP LSPs should indicate that even 
 though all nodes are allowed to send on the LSP, there is still a 
 distinguished root node.

Ok, I will add that.

 
The LDP MP Opaque Value Element extended type (section 2.3, second 
 definition) seems to be gratuitous complexity, adding extra matching cases in 
 the LDP processing path for no stated value.  Is there really believed to be 
 a need for more than 254 types of Opaque values?  If so, explain it in the 
 document.

The opaque type was defined as a non-extensible one-octet field. This may be 
enough for the future, but you never know. I guess there are many examples in 
the IETF where fields seemed to be large enough but proved to be too small over 
time. So why not just document it now.

 
Section 3.3.1.3 begins by describing two methods for installing the 
 upstream path of a MP2MPLSP.  After carefully describing both, it says to 
 only and always use the second method.  Would it not be better to describe 
 the constraint (that the upstream path must be in place all the way to the 
 root before it claims to be established), and then describe the one method 
 that meets taht.  Just don't describe a method that is not to be used.

That is a good comment, let me think about this for a bit.

Thx,

Ice.


 
 Nits/editorial comments:
 ___
 Ietf mailing list
 i...@ietf.org
 https://www.ietf.org/mailman/listinfo/ietf

___
Gen-art mailing list
Gen-art@ietf.org
https://www.ietf.org/mailman/listinfo/gen-art

Re: [Gen-art] Gen-ART Last Call Review of draft-ietf-pim-rpf-vector-06

[IJsbrand Wijnands]

Hi Ben,

Sorry for the delay..



Substantive Comments:

-- It is not clear to me why this is to be an informational RFC. It  
seems to be defining protocol. If that protocol is not intended to  
be a standard, then it would help to have an applicability  
statement to that effect.


Good point, we feel this document should be on standard track. We're  
working with the PIM WG chairs currently to get in on standards track.




-- Section 4:

The IANA considerations section needs a little more information. Is  
this attribute to be added to an existing registry? Is a new  
registry needed?


To an existing registry, we'll make this clear.



-- Section 5:

The security considerations section implies that adding this new  
Vector creates no new security considerations beyond those in  
RFC4601. I am not qualified to hold an opinion whether this is true  
or not--has the working group explicitly thought about it?


Well, it has passed WG last-call, so as far as I can tell there is  
consensus..




Editorial Comments:

-- IDNITS reports that there is no RFC 2119 reference or  
boilerplate, but there is at least one use of normative language  
(2.3.4).


Will fix that.



-- There are a number of acronyms that should be expanded on first  
use. I would not worry about expanding acronyms that are well known  
to the entire IETF community (e.g. TCP), but acronyms that are not  
widely known outside the BGP community probably should be.


Will fix that.



-- Section 2, first sentence:

Who is the we in this context? A edge router? (This is not a  
complaint about 2nd person language in general so much as a concern  
about the actor being obscured.)  The pattern of saying we or  
our referring to a network element taking some particular action  
occurs a few more times in the document. It would be better to  
simply name the element.


Ok, will adjust the text to make it clear.



-- Section 2.3.4, first paragraph:

s/depending/dependent

-- IDNITS reports that the reference to draft-ietf-pim-join- 
attributes-03 is outdated. There is an 06 as of the time of this  
review.


This is an RFC now, will update it.

Thanks for the review!

Ice.


___
Gen-art mailing list
Gen-art@ietf.org
https://www.ietf.org/mailman/listinfo/gen-art