Subject: [PWE3] AD review of draft-ietf-pwe3-fat-pw

From: Adrian Farrel <[email-masked]>
Date: Sat, 29 Jan 2011 13:03:32 +0000

Hi,

I have performed an AD review of your draft.

Don't panic!

I review all drafts that I am responsible for before putting them
forward for IETF last call. The main objective is to catch nits and
minor issues that would show up during the last call or in IESG
review. The intention is to help polish your document and make sure
it is clean and shiny so that other reviewers will stick to the
technical details. I am responsible for this I-D because Stewart
Bryant is a co-author and therefore cannot take responsibility for the
document.
                                                                            
Your document describes a very small piece of function, and so there
is very little of technical substance to discuss. As such, I was a bit
surprised at the length and weight of this draft and I feel that it
could usefully be heavily pruned. However, I doubt that there is energy
or enthusiasm for that work, so I think you will just want to limit
yourself to addressing my issues below.

Most of my comments are pretty trivial: they are concerned with clarity 
and consistency of your text. But there are quite a lot of them, so
I'd like to see a quick respin of the document before I issue the IETF
last call. As soon as I see a new revision posted, I'll set the ball in
motion.

Of course, all of my issues are up for discussion.

Thanks for the work,
Adrian

---

I found the use of language a little relaxed. You use the same word,
"label", to refer to a label value and to an entry in the shim header. 
It would probably be helpful to use the term "label" to refer to the
label itself, and "label entry" to indicate the label, TC bits, and TTL.

---

It seems to me that the description of what can be in the additional
label entry is conflicted.

   On receipt of the
   pseudowire packet at the egress PE (which knows this additional label
   is present) the flow label is discarded without processing.

This means that the label will not be used for any purpose in the packet 
processing except in the DPI of ECMP hashing.

   The flow label can never become the top label in
   normal operation

That is, you will never pop a label to discover the flow label except at
the egress where it is expected and ignored.

   Note that the flow label MUST NOT be an MPLS reserved label (values
   in the range 0..15) [RFC3032], but is otherwise unconstrained by the
   protocol.

And why is that restriction required given the two previous points?
                                              
   The flow label can never become the top label in
   normal operation, and hence the TTL in the flow label is never used
   to determine whether the packet should be discarded due to TTL
   expiry.  Therefore there are no lower restrictions on the TTL value.

Why do you call out "lower"? How about saying "there are no restrictions"

   The use of the TC bits (formerly known as the EXP bits) in the flow
   label is outside the scope of this document.  Unless otherwise agreed
   by the ingress and egress PEs these bits MUST be set to zero by the
   ingress PE and MUST be ignored by the egress PE.

"Out of scope" seems to be different from the subsequent statement about
how he TC field must be treated. I am also not clear why the TTL 
discussion is different from the TC discussion.

---

Document title: please spell out PSN           

---
                                                         
Abstract: yuck! :-O

Suggestion...

OLD
   Where the payload carried over a pseudowire carries a number of
   identifiable flows it can in some circumstances be desirable to carry
   those flows over the equal cost multiple paths (ECMPs) that exist in
   the packet switched network.  Most forwarding engines are able to
   hash based on label stacks and use this to balance flows over ECMPs.
   This draft describes a method of identifying the flows, or flow
   groups, to the label switched routers by including an additional
   label in the label stack.
NEW
   Where the payload of a pseudowire comprises a number of distinct
   flows, it can be desirable to carry those flows over the equal cost
   multiple paths (ECMPs) that exist in the packet switched network.
   Most forwarding engines are able to hash based on MPLS label stacks
   and use this mechanism to balance MPLS flows over ECMPs.

   This document describes a method of identifying the flows, or flow
   groups, within pseudowires such that Label Switching Routers can
   balance flows at a finer granularity than individual pseudowires.
   The mechanism uses an additional label in the MPLS label stack.
END

---

Try to avoid "ECMP paths" (or ECMPP as we call them ;-)

---

Section 1 para 1

   the use of ECMP paths for is known to be
   beneficial (and not harmful) to the operation of the PW.

??

---

Throughout...

The plural of "pseudowire" is "pseudowires", not "pseudowire's".
You may safely use "PWs" in your document.

You have a number of other plurals formed as apostrophe-s. Please
search for "'s" and fix as necessary.

---

Section 1

   Some pseudowires are used to transport large volumes of IP traffic
   between routers at two locations.

I assume that, for some definition of "location" this is also true for
routers at the same location. So how about...

   Some pseudowires are used to transport large volumes of IP traffic
   between routers.

---

Section 1

   Such pseudowire's do not require strict ordering to
   be preserved between packets of the pseudowire.  They only require
   ordering to be preserved within the context of each individual
   transported IP flow.

I see where this is going, but the first sentence is massively more
relaxed than the second! How about turning it around...

   Such pseudowires only require packet ordering to be preserved within
   the context of each individual transported IP flow. They do not
   require packet ordering to be preserved between all packets of the
   across all IP flows within the pseudowire.

---

Section 1

   Some operators have requested the ability to
   explicitly configure such a pseudowire to leverage the availability
   of multiple ECMP paths.

I wish you did not feel the need to include this assertion. Why not just
get on and describe the benefits (as you do in the next sentence).

Currently it really sounds like this is an idea that is strongly opposed
and you are trying to justify it to the rest of the community. In fact, 
the community will judge according to the technical merits.

---

Please don't use the word "draft" in the text as it creates work for the
RFC Editor and might get missed in the publication process. Obviously, by
the time this is published as an RFC it will no longer be a draft. Best 
thing to do is search and replace "draft" with "document".

---

Section 1

   Typically, forwarding hardware can deduce that an IP payload is being
   directly carried by an MPLS label stack, and is capable of looking at
   some fields in packets to construct hash buckets for conversations or
   flows.  However, an intermediate node has no information on the type
   pseudowire being carried in the packet.

Is the first sentence relevant to this document? I don't think so, but if
you believe it is, I think you will need to add a statement about how 
this deduction is made.

---

Section 1

   In the case of a pseudowire emulating
   a high bandwidth trunk, the granularity obtained by hashing the
   default label stack is inadequate for satisfactory load-balancing.

Is "the default label stack" intended to convey some special meaning?
I suspect you are trying to say "the label stack that would be
constructed if this I-D hasn't been implemented". You will simply have
to go down the path of saying...

   In the case of a pseudowire emulating
   a high bandwidth trunk, the granularity obtained by hashing the
   label stack is inadequate for satisfactory load-balancing.

---

Section 1

   This draft proposes a method to introduce granularity on the hashing
   of traffic running over pseudowires by introducing an additional
   label, chosen by the ingress node, and placed at the bottom of the
   label stack.

As above: s/draft/document/
Also, please be less timid. Your document defines a method.

---

Section 1.1

s/1.1.  ECMP in Label Switched Routers/1.1.  ECMP in Label Switching Routers/

s/Label switched routers/Label Switching Routers (LSRs)/

---

Section 1.1

   been proposed [I-D.kompella-mpls-entropy-label], however that is

Reading would be significantly enhanced by  forward reference to section
8.

---

Section 3

   It is recommended

RECOMMENDED ?

---

Section 3

   It is recommended that the method chosen to
   generate the load balancing labels introduces a high degree of
   entropy in their values, to maximise the entropy presented to the
   ECMP path selection mechanism in the LSRs in the PSN, and hence
   distribute the flows as evenly as possible over the available PSN
   ECMP paths.

It isn't clear to me that entropy is necessarily a good thing. It
depends on the hash algorithm, doesn't it.

---

Figure 2

Those four-octet things are not actually labels are they.

---

What is the purpose of the T bit? Why would an implementation go to 
the trouble of including the flow-label TLV only to set T=0?

---

Section 4

   If PWE3 signalling [RFC4447] is not in use for a pseudowire, then
   whether the flow label is used MUST be identically provisioned in
   both PEs at the pseudowire endpoints.  If there is no provisioning
   support for this option, the default behaviour is not to include the
   flow label.

This paragraph is misplaced in the section on signaling. Please put it
elsewhere. 

I am not convinced by the 2119 language in this paragraph since an
implementation cannot control the behavior.

---

Section 4

Do you not think that you should say that the new sub-TLV is carried in
the Interface Parameters TLV, and on which messages?

---

Section 4.1

   o  FL is the flow label sub-TLV identifier assigned by IANA.

Can you either put in a [TBD] flag so the RFC Editor will catch this,
or a forward pointer to section 10.

---

Section 5

Consider an egress that doesn't support flow label, but previous S-PEs
that do. In this case you are content to throw the baby out with the
bath water?

---

Section 6                                       

   We believe that this
   problem is addressed by the following two methods

I would like you to be more definitive, please.

But, that said, the subsequent bullets do not provide methods of
addressing OAM in this case. Rather, the text attempts to say why
the problem is not important. In my opinion it fails since, although
ECMP is out of scope for MPLS-TP, a user has reasonable expectations
that some of the flows on their PW will not be wiped out for the IGP
convergence period, when (for example) rapid notification followed
by change of the flow label could cause the traffic to be balanced
differently and avoid the failure, or when a partial failure of a PW
could trigger a switch to a different PW.

---

Section 6

s/corruption of an/corruption of a/

---

You need to get rid of the reference to I-D.ietf-mpls-lsr-self-test
This work has clearly been abandoned.

---

s/Figure 4: Use of Router Alert LAbel/Figure 4: Use of Router Alert Label/

---

Figure 4

Shouldn't "payload" actually read "VCCV message"?

Shouldn't you include the warning (from 5085) that inserting the router 
alert label probably breaks the ECMP hashing such that the packets will
go another way.

---

How funny that labels that are conventionally described as being "above"
on the label stack are shown in your diagrams as below.

---

Section 7                            

s/The methods describe/The methods described/
s/The method proposed/The method defined/
s/in those cased/in those cases/
s/The methods describe/The methods described/

---

Section 7

   order is not maintained amongst packets of different
   flows.
                                     
I know what you mean, but you have actually said something subtly 
different.

---
                                 
Section 7

   Ethernet may have this property, and for that reason
   this document focuses on Ethernet.  Applications for other high-
   access-bandwidth PW's (e.g.  Fibre Channel) may be defined in the
   future.

Does the document really *focus* on Ethernet? There are a couple of
places where Ethernet is cited as an example (but where the text says
equally applicable, blah, blah) and two paragraphs a little below this
statement that say "the Ethernet PW". Nothing else.

Can't you generalise?

---

Section 7

   If sequence number support is required this
   mechanism is not applicable.

It's stronger than that, isn't it?
You need to prohibit the mechanism.
But shouldn't you also prohibit ECMP?

---

Section 7

   Where it is known that the traffic carried by the Ethernet pseudowire
   is IP the method of identifying the flows are well known and can be
   applied. Such methods typically include hashing on the source and
   destination addresses, the protocol ID and higher-layer flow-
   dependent fields such as TCP/UDP ports, L2TPv3 Session ID's etc.

Either s/method/methods/ or s/are/is/

If these methods are so well known, you will have no difficulty in
supplying a reference or two.

I think you have mixed the method of identifying the flows with the 
method of hashing the flows into buckets.

---

Section 7.1

   In the ECMP case and the link bundling case the NSP may attempt to
   take bandwidth into consideration when allocating groups of flows to
   a common path.  That is permitted, but it must be borne in mind that
   the semantics of a label stack entry (LSE) as defined by [RFC3032]
   cannot be modified, the value of the flow label cannot be modified at
   any point on the LSP, and the interpretation of bit patterns in, or
   values of, the flow label by an LSR are undefined.

If you have something to say, you should say it. Otherwise your paragraph
is lost on your readers.

I think you are trying to say "There is no mechanism currently defined
to indicate the bandwidths in use by specific flows using the fields of
the MPLS shim header. Furthermore, since the semantics of the MPLS shim
header are fully defined in [RFC3032] and [RFC5462], those fields cannot
be assigned semantics to carry this information. This document does not
define any semantic for use in the TTL or TC fields of the label entry
that carries the flow label, but requires that the flow label itself
be selected with a high degree of entropy suggesting that the label 
value should not be overloaded with additional meaning in any subsequent
specification."

---

Section 7.1

   A different type of load balancing is the desire to carry a
   pseudowire over a set of PSN links in which the bandwidth of members
   of the link set is less than the bandwidth of the pseudowire.  This
   problem is addressed in [I-D.stein-pwe3-pwbonding].  Such a mechanism
   can be considered complementary to this mechanism.

The referenced I-D seems to be dead. I'm not quite sure why you need to
reference it here. How about just taking out the paragraph?

---

Section 7.2 appears to describe exactly the link bundling case that you
discussed in section 7. I have recently noticed considerable confusion
between link bundles and LAG and suggest you restrict LAG to be a data
link function that is below the level of an "interface" in the IP/MPLS
world. That does not mean that an individual node, knowing (by
unspecified means) that it is about to use an interface that is 
constructed from a LAG, can't hash the traffic across the LAG.

Ultimately, you should consider ECMP (i.e. multiple IGP paths) where 
each hop on each path may be made a link bundle, and where each 
component link of a bundle may be supported by LAG.

Can you go back and check that you have used the language precisely.

---

Section 7.4

   If the payload on a PW is made of a single inner flow (i.e. an
   encrypted connection between two routers), or the flow identifiers
   are too deeply buried in the packet, then the functionality described
   in this document does not give any benefits, though neither does it
   cause harm relative to the existing situation.

Is that s/i.e./e.g./?

"Too deeply buried" meaning "If the NSP cannot access sufficient 
information to distinguish flows, perhaps because the protocol stack
is too deep for its capabilities..."?

Is the statement about "no harm" necessary? After all, the mechanism
will simply not be used in this case.

---

Section 7.4

   The most common case
   where a single flow dominated the traffic on a PW is when it is used
   to transport enterprise traffic.  Enterprise traffic may well consist
   of a large single TCP flows, or encrypted flows that cannot be
   handled by the methods described in this document.

s/dominated/dominates/
s/a large single TCP flows/large TCP flows/

Note that this second half of the paragraph is a non-sequitur from the
first half. You probably need to wrap the whole paragraph with a thought
about single or dominant large flows.

---

Section 7.4

   1.  The operator can do nothing and the system will work as it does
       without the flow label.

s/can do nothing/can choose to do nothing/

---

Section 7.4

   An operator has six options under these circumstances:

:-) You tease!

There are only five listed.

---

Section 7.4

s/an single flow/a single flow/

---

Section 7.4

   5.  The operator could configure the ingress to assign a label of
       special significance (such as a reserved label) to all high
       bandwidth flows so that some other action (not specified in this
       document) could be taken on the flow.

I think you need to delete this as it contradicts two specific 
statements made elsewhere...

1. MUST NOT use a reserved label as the flow label
2. MUST NOT assign semantics to the flow label

---

8.  Applicability to MPLS

While I see where you are coming from, I must point out that the whole
of your document is about MPLS. Could you come up with a better title?

---

Section 9

   The ingress PE SHOULD take steps to ensure that the load-balance
   label is not used as a covert channel.

Again, you are saying something between the lines :-(
Since the ingress PE allocates the label (isn't it called the flow label
not the load-balance label?) the "SHOULD take steps" is very odd! And 
why is this a security requirement? How would security be compromised?

---

Section 9

This document has signaling extensions in it, so you should reference
RFC 4447 from this section. You should also reference RFC 5920.

---

Section 9

   The flow label TTL should therefore never be considered
   by the forwarder, and hence SHOULD be set to a value of 1.  This will
   prevent the packet being inadvertently forwarded based on the value
   of the flow label.  Note that this may be a departure from
   considerations that apply to the general MPLS case.

But you have previously said that the flow label TTL is never used to 
discard a packet based on TTL expiry [section 1.2]. You need to make
your mind up. And, if there are rules you wish to apply for setting
fields, you need to put them in the main part of the protocol spec,
not buried in the security section.

And (while I'm at it) how is this a security feature?

Oh, and another thing...

The sequence of your logic is broken. It would be fine to say:
If the flow label is inadvertently examined as if it were a normal 
label, the packet might be forwarded. This can be prevented by setting
the associated TTL to 1.
But I don't think that "should never be considered" leads to "should be
set to 1".

Finally, the note at the end is at best unhelpful. What are you trying
to say, and why?

---

Section 10

Please change this text to reference early allocation that has been 
made for you. Note that the value 17 has NOT been assigned (although
the value 0x17 has).