Subject: [mpls] Ben Campbell's No Objection on draft-ietf-mpls-mldp-node-protection-05: (with COMMENT)

From: "Ben Campbell" <[email-masked]>
Date: Mon, 14 Sep 2015 15:03:31 -0700

Ben Campbell has entered the following ballot position for
draft-ietf-mpls-mldp-node-protection-05: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-mpls-mldp-node-protection/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

The shepherd writup says that there has been no explicit discussion of
the two IPR disclosures. It's probably too late to worry about that for
this draft, and both disclosures have the fairly common "we won't assert
if you don't" terms. But I think we should encourage working groups to
have more explicit discussion for IPR disclosures.

-- 2.3, last paragraph: "Along with the PLR MP Status a MP FEC TLV MUST
be included"
Does that mean that both MUST be included, or if the first is included,
the second MUST also be?

-- 4.1.3, last paragraph:
Just “recommended”? Is link flapping a minor enough that it doesn't
justify a MUST?

-- 6:
It would be nice to show your work a bit more in the security
considerations. This draft adds new protocol elements and procedures. If
the working group has determined that those new bits add no new security
concerns, it would be good to say why.

Editorial and Nits:

A (probably first) paragraph in the intro that defined exactly what the
draft means by "protection" would be helpful. (The existing first
paragraph talks about how you provide protection, but one must infer what
this protects _against_.

-- 1, 2nd paragraph: Lots of the terms here could use (informative)
citations.

-- 2.1, first paragraph: Consider s/"we are describing"/"we describe"
-- 2.1, last paragraph, 2nd to last sentence, "See section 5":
 unbalanced parentheses.

-- 2.2, 1st paragraph:
s/"we are describing"/"we describe"

-- 2.2, last paragraph: 
"protection mechanism don’t" -- Noun/verb disagreement (singular/plural)
s/ help restoring/help restore

-- 2.3, 2nd to last paragraph:
I suggest the “A node N” phrase be moved to the first mention of N in
this paragraph.

"Removing a PLR address is likely due to a link failure, see the
procedures as documented in Section 4.1. ":
Comma splice. Consider a semicolon.

"MUST encode PLR Status Value Element": Missing article.

-- 3, 2nd paragraph:
"Ln that was assigned to N via the normal mLDP procedures, and Label Lpx
that was assigned for PLR (LSR1) for the purpose of node protecting MP
LSP via node N."
I can’t parse this sentence. (Incomplete sentence?)

-- 3, 3rd paragraph: "For this reason, the FEC Label Mapping (FEC <R,X> :
Lpx) sent by the MPT over the tLDP session to the PLR MUST include a
Status TLV with
   MP Status including a new LDP MP status Value Element called the
"Protected Node Status Value Element"."
Convoluted sentence. Consider breaking into multiple, simpler sentences.

-- 5, 1st sentence:
s/we are organizing/we organize/

---

From: IJsbrand Wijnands <[email-masked]>
Date: Wed, 16 Sep 2015 12:04:10 +0200

Hi Ben,

> Does that mean that both MUST be included, or if the first is included,

Both MUST be included.

> Just “recommended”? Is link flapping a minor enough that it doesn't

Well, its really an implementation choice to save resources being using in deleting and re-creating the tLDP session. If the implementation has means to deal with this or has other mechanism to solve this problem, that is fine too. 

> concerns, it would be good to say why.

Yep, that is done.

> Comma splice. Consider a semicolon.

Ok, I think I fixed the above.

> "MUST encode PLR Status Value Element": Missing article.

What do you mean here?

> I can’t parse this sentence. (Incomplete sentence?)

Ok, updated it, hope its better now.

> s/we are organizing/we organize/

Ok,

Thx!

Ice.

---

From: IJsbrand Wijnands <[email-masked]>
Date: Wed, 16 Sep 2015 12:04:10 +0200

Hi Ben,

Both MUST be included.

Yep, that is done.

What do you mean here?

Ok,

Thx!

Ice.

---

From: "Ben Campbell" <[email-masked]>
Date: Wed, 16 Sep 2015 19:42:55 -0500

>> Does that mean that both MUST be included, or if the first is 

Then I suggest something like "Both the PRL MP Status and an MP FEC TLV 
MUST..."

> Well, its really an implementation choice to save resources being 
> using in deleting and re-creating the tLDP session. If the 
> implementation has means to deal with this or has other mechanism to 
> solve this problem, that is fine too.

Okay, A few words to that effect might be helpful.

[...]

>> "MUST encode PLR Status Value Element": Missing article.

"MUST encode _the_ PLR Status Value Element". (or possibly _a_ PRL 
Status Value Element)

Thanks!

Ben.

---

From: IJsbrand Wijnands <[email-masked]>
Date: Mon, 28 Sep 2015 21:40:37 +0200

Hi Ben,

>>> 
>> 
>> Both MUST be included.
> 
> Then I suggest something like "Both the PRL MP Status and an MP FEC TLV MUST…"

Ok, I’ll make that change.

> Okay, A few words to that effect might be helpful.

Well, in that section I’m already making the reader aware of the fact that a tLDP session may be flapping due to link flapping, and what solution can be applied to mitigate it. I don’t think more words are necessary here. Are you ok if we keep it like it is?

> "MUST encode _the_ PLR Status Value Element". (or possibly _a_ PRL Status Value Element)

Got it ;-)

Thx,

Ice.

---

From: "Ben Campbell" <[email-masked]>
Date: Mon, 28 Sep 2015 14:47:34 -0500

On 28 Sep 2015, at 14:40, IJsbrand Wijnands wrote:

> Well, in that section I’m already making the reader aware of the 
> fact that a tLDP session may be flapping due to link flapping, and 
> what solution can be applied to mitigate it. I don’t think more 
> words are necessary here. Are you ok if we keep it like it is?

I meant a few words to explain why this is not a MUST, not the fact that 
the sessions may flap. For example something to the effect of "However, 
an implementation might have other mechanisms to prevent flapping, in 
which case the delay might not be needed."

---

From: IJsbrand Wijnands <[email-masked]>
Date: Tue, 29 Sep 2015 10:29:52 +0200

Hi Ben,

How about this:

"If the operator is not concerned with the tLDP session flapping and/or other procedures are in place to avoid this altogether, there is no need to apply the delay."

Thx,

Ice.

> On 28 Sep 2015, at 21:47, Ben Campbell <[email-masked]> wrote:
> 
> I meant a few words to explain why this is not a MUST, not the fact that the sessions may flap. For example something to the effect of "However, an implementation might have other mechanisms to prevent flapping, in which case the delay might not be needed."

---

From: "Ben Campbell" <[email-masked]>
Date: Tue, 29 Sep 2015 09:12:52 -0500

On 29 Sep 2015, at 3:29, IJsbrand Wijnands wrote:

> Hi Ben,
>
> How about this:
>
> "If the operator is not concerned with the tLDP session flapping 
> and/or other procedures are in place to avoid this altogether, there 
> is no need to apply the delay."

That works for me. Thanks!

Ben.

>>>>> implementation has means to deal with this or has other mechanism 
>>>>> to solve this problem, that is fine too.
>>>
>>>>
>> I meant a few words to explain why this is not a MUST, not the fact 
>> that the sessions may flap. For example something to the effect of 
>> "However, an implementation might have other mechanisms to prevent 
>> flapping, in which case the delay might not be needed."