Re: [Pals] Mirja Kühlewind's Discuss on draft-ietf-pals-mpls-tp-pw-over-bidir-lsp-08: (with DISCUSS and COMMENT)

[Mach Chen <[e-mail]>]

Hi Mirja,

Yes, this is a common and well-known design pattern in MPLS related TLV/sub-TLV definition. 

If we do not use sub-TLV pattern, then we need to define multiple top level TLVs. And if we go that way, it will bring fundamental changes to the document. I tend to keep the current design. 

In addition, from the implementation point of view, seems there is not too much difference.


Best regards,
Mach 

> -----Original Message-----
> From: Mirja Kuehlewind (IETF) [mailto:[e-mail]]
> Sent: Wednesday, July 06, 2016 3:35 PM
> To: Mach Chen
> Cc: The IESG; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: Mirja Kühlewind's Discuss on
> draft-ietf-pals-mpls-tp-pw-over-bidir-lsp-08: (with DISCUSS and COMMENT)
> 
> Hi again,
> 
> one more comment: I’m okay with the use of sub-TLVs if that is common and
> well-known design pattern that people are used to. However, as you only can
> have one sub-TLV at a time, please re-consider carefully, as this might add
> complexity with no gain (and therefore is only a source of potential errors, e.g.
> if more than one is present).
> 
> Mirja
> 
> 
> > Am 06.07.2016 um 09:27 schrieb Mirja Kuehlewind (IETF)
> <[e-mail]>:
> >
> > Hi Mach,
> >
> > thanks! Suggested changes sound good to me. Will wait for the new revision!
> >
> > One further question on the „MUST be zero“ part: Isn’t this effectively also
> a „Reserved“ field, in the sense that is could be used in future? „MUST be
> zero“ sounds like it must be zero for ever and ever (because there is a technical
> reason to have zeros there); I don’t think that’s the case, right?
> >
> >
> > Mirja
> >
> >> Am 06.07.2016 um 08:12 schrieb Mach Chen <[e-mail]>:
> >>
> >> Hi Mirja,
> >>
> >> Thanks for your "DISCUSS and COMMENT"!
> >>
> >> Please see some replies inline...
> >>
> >>> -----Original Message-----
> >>> From: Mirja Kuehlewind [mailto:[e-mail]]
> >>> Sent: Wednesday, July 06, 2016 12:39 AM
> >>> To: The IESG
> >>> Cc: [e-mail]; Stewart
> >>> Bryant; [e-mail]; [e-mail];
> >>> [e-mail]
> >>> Subject: Mirja Kühlewind's Discuss on
> >>> draft-ietf-pals-mpls-tp-pw-over-bidir-lsp-08: (with DISCUSS and
> >>> COMMENT)
> >>>
> >>> Mirja Kühlewind has entered the following ballot position for
> >>> draft-ietf-pals-mpls-tp-pw-over-bidir-lsp-08: Discuss
> >>>
> >>> 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-pals-mpls-tp-pw-over-bid
> >>> ir-lsp/
> >>>
> >>>
> >>>
> >>> --------------------------------------------------------------------
> >>> --
> >>> DISCUSS:
> >>> --------------------------------------------------------------------
> >>> --
> >>>
> >>> I think the protocol specification is not complete:
> >>>
> >>> - What happens if none of the two S and C bits are set?
> >>
> >> Indeed, this is not explicitly stated in the document. One way is to clarify
> what happens if none of the two S and C bits are set. Another way is to along
> your suggestion below. After consideration, I think your suggestion is better
> and we will accept your suggestion.
> >>
> >> Then now, C bit will be omitted, and only the S bit will be left and it's
> definition as below:
> >>
> >> "S (Strict) bit: This instructs the PEs with respect to the handling of the
> underlying LSPs. When the S bit set, the remote PE MUST use the tunnel/LSP
> specified in the PSN Tunnel Sub-TLV as the PSN tunnel on the reverse direction
> of the PW, or the PW will fail to be established. When the S bit cleared, the
> remote T-PE/S-PEs SHOULD select co-routed LSP (as the forwarding tunnel) as
> the reverse PSN tunnel. If there is no such tunnel available, it may trigger the
> remote T-PE/S-PEs to establish a new LSP."
> >>
> >> And correspondingly, the some text in the Operation section will modified
> accordingly.
> >>
> >>>
> >>> - What happens if more the one sub-TLV is present?
> >>
> >> How about:
> >>
> >> OLD:
> >> "Each PSN Tunnel Binding TLV can only have one such sub-TLV."
> >>
> >> NEW
> >> "Each PSN Tunnel Binding TLV MUST only have one such sub-TLV. When
> sending, only one sub-TLV MUST be carried. When received, if there are more
> than one sub-TLVs carried, only the first sub-TLV MUST be used, the rest
> sub-TLVs MUST be ignored."
> >>
> >> Hope this addresses the DISCUSS.
> >>
> >>
> >>>
> >>> Actually I think that the protocol design is more complicated than
> >>> needed and simplifying it would resolve these error cases. But as
> >>> that's not a DISCUSS reason, please see more detailed comments below!
> >>>
> >>>
> >>> --------------------------------------------------------------------
> >>> --
> >>> COMMENT:
> >>> --------------------------------------------------------------------
> >>> --
> >>>
> >>> A few questions/comments:
> >>>
> >>> 1) The shepherd writeup says: "An IPR discolusure has been filed and
> >>> the WG has not remarked on this."
> >>>   Does this mean the wg is not aware of it or it is aware and nobody
> >>> commented?
> >>>
> >>
> >> I think it's the latter.
> >>
> >>> 2) Acronym: I found a few acronyms that where not spelled out, e.g.
> >>> FEC as well as LDP and TE in the abstract. One can argue if that's
> >>> needed but usually I think it helps. Further there are quite a few
> >>> acronyms that are introduced somewhere and then never used again
> >>> (e.g. Point-to-Point (P2P); that makes it actually harder to read, so I would
> recommend to remove them.
> >>
> >> Sure, will look through the document and expand the acronyms when
> needed and remove never used acronyms.
> >>
> >>>
> >>> 3) I think I don't understand the following MUST: "PSN Tunnel
> >>> Binding TLV is an optional TLV and MUST be carried in the Label
> >>> Mapping message [RFC5036] if PW to LSP binding is required." and
> >>> again later "To perform PSN binding, the Label Mapping messages MUST
> >>> carry a PSN Tunnel Binding TLV". Maybe just remove it?
> >>
> >> The "MUST" applies when "PW to LSP binding is required". Since there are
> "MUST in the Operation sections, I will remove the one in the TLV definition
> section. How about:
> >>
> >> OLD:
> >> "PSN Tunnel Binding TLV is an optional TLV and MUST be carried in the Label
> Mapping message [RFC5036] if PW to LSP binding is required."
> >>
> >> NEW:
> >> "PSN Tunnel Binding TLV is an optional TLV."
> >>
> >>
> >>>
> >>> 4) Why is there a "MUST be zero" and a "Reserved" field? Shouldn't
> >>> this all just be a large "Reserved" field? And the "Reserve" field is not
> discussed.
> >>
> >> The "MUST be zero" and the allocated flags belong to the Flag field. The
> "Reserved" filed is just as its name, it is reserved for future use. Maybe some
> text can be added to clarify this. For example : " The Reserved field is 2 octets
> in length and is left for future use."
> >>
> >>>
> >>> 5) On the T (Tunnel Representation) bit: Why is that an extra bit?
> >>> Usually you can simply use the length field here and then either
> >>> have the LSP Number fields in the sub-TLV or not.
> >>
> >> Currently, there are two sub-TLVs, one is for IPv4 and the other is for IPv6, if
> we use the length to imply sub-TLV types, then the sub-TLVs will become four.
> And we have to guarantee that the four sub-TLVs have their own unique length,
> otherwise, a receiver cannot parse the packet. In addition, there may be more
> sub-TLVs introduced in the future, seems it's difficult or impossible to make
> sure that all sub-TLVs have different length.
> >>
> >>>
> >>> 6) Why are there two bits for C and S if they are mutual exclusive
> >>> and one of them has to be set? In this case you should use one bit
> >>> and say e.g. 0 means
> >>> S(trict) and 1 means C(o-routed path). This would also avoid the
> >>> error case when both are set! (Otherwise if it should be possible to
> >>> not set both flags, this case is not described  in the draft and
> >>> must be
> >>> added.)
> >>
> >> Will keep the S bit and remove the C bit.
> >>
> >>>
> >>> 7) Sub-TLV: I'm not an LDP expert but I assume having sub-TLV is not
> >>> a general concept that is widely used this way but something introduced in
> this document.
> >>> If that's the case, I have a couple of questions/remarks:
> >>
> >> No, having sub-TLV is general concept for LDP and is widely used.
> >>
> >>
> >>>
> >>>  - Given that the PSN Tunnel Binding TLV already has a length field
> >>> that includes the sub-TLV, why do you need another length field in the
> sub-TLV?
> >>
> >> Since a sub-TLV is also a TLV, it has a length field. The length of the sub-TLV
> seems a bit "redundant" just because the PSN Tunnel Binding TLV can carry only
> one sub-TLV, otherwise the length is necessary. In addition, a sub-TLV may be
> reused in another TLV that may carry more than one sub-TLVs. I tend to keep
> this length.
> >>
> >>> - Why is there another 'Reserved' field in the sub_TLV (that is also
> >>> not discussed)?
> >>
> >> This is to make the sub-TLV 4-octet alignment.
> >>
> >>> - If you already have different types of sub-TLVs here, why don't
> >>> you simply define some with or without the the LSP Number fields instead
> of the T bit?
> >>
> >> This is a design choice, your suggested is an alternative way.
> >>
> >>> - And finally do you really need these sub-TLVs  or would a flag be
> >>> enough to indicate IPv4 or IPv6 (or again just use the length field to
> distinguish)?
> >>
> >> There may be other ways, but IMHO, as said above, sub-TLV-based
> mechanism is common for LDP and widely used.
> >>
> >>> - And this should be normative "Each PSN Tunnel Binding TLV can only
> >>> have one such sub-TLV." -> "Each PSN Tunnel Binding TLV MUST have
> >>> only one such sub-TLV." and you must specify what happen if more
> >>> then one is presented (if you actually want to use sub-TLVs).
> >>
> >> OK.
> >>
> >>>
> >>> 8) There are a couple of MUSTs basically twice in the document (once
> >>> in section
> >>> 2 and another time in sections 4 and 5). I would recommend to avoid
> >>> these duplications and subsequent redundancy and really state it
> >>> only once to avoid any future confusions.
> >>
> >> OK, will go though the document and make the usage consistent.
> >>
> >> Best regards,
> >> Mach
> >>>
> >>
> >