AD review of: draft-ietf-tewg-diff-te-proto-05.txt

["Wijnen, Bert (Bert)" <bwijnen@lucent.com>]

Editor/authors and WG, here is my review.

Serious (in my view):
- On page 10, you say that there are 8 BCs (0-7), so that
  seems a fixed number to me. With tha in mind I read:
    As detailed above in section 4.1.1, up to 8 Bandwidth Constraints
    ( BCb, 0 <= b <= 7) are configurable on any given link.
  
  so far so good.

    With DS-TE, the existing "Maximum Reservable Bw" sub-TLV is retained

  might be good to add a citation and reference to the RFC where that
  existing "Maximum Reservable Bw" sub-TLV is defined.
  Francois told me it is RFC3630. So something aka:

    With DS-TE, the existing "Maximum Reservable Bandwidth" sub-TLV  
    (sub-TLV of the Link TLV of the TE Extensions to OSPF Version 2
    [RFC3630]) is retained

  I think it is better to expand Bw to Bandwidth in the above, cause
  that is how it is named in RFC3630.

    with a generalized semantic so that it MUST now be interpreted as the
    aggregate bandwidth constraint across all Class-Types [ i.e.
    SUM (Reserved (CTc)) <= Max Reservable Bandwidth], independently of
    the Bandwidth Constraints Model.

So does that mean that you basically UPDATE RFC3630? You change the
definition of one of the sub-TLVs in that document. If so (which I
think) then you must add to the abstract that you do update RFC3630.
  
    This document also defines the following new optional sub-TLV to
    advertise the eight potential Bandwidth Constraints (BC0 to BC7):
 
    "Bandwidth Constraints" sub-TLV:
          - Bandwidth Constraint Model Id (1 octet)
          - Bandwidth Constraints (Nx4 octets)

  So should I assume that if there are only 3 bandwidth constraints, then
  N above is 3 ?? I think it can be made clearer if there always need to be
  8, or if that is not the case, what the value of N is in that case and
  how that is determined.
 
    Where:
 
          - With OSPF, the sub-TLV is a sub-TLV of the "Link TLV" and its
            sub-TLV type is TBD. See IANA Considerations section below.
  
  Would be good to describe if this needs to be a length to be padded
  to 32 bits (as also in RFC3630). I think a diagram mightbe helpfull too.

          - With ISIS, the sub-TLV is a sub-TLV of the "extended IS
            reachability TLV" and its sub-TLV type is TBD. See IANA
            Considerations section below.
  
  similarly, point to RFC describing the ISIS TLVs, so people know where 
  it fits.

          - Bandwidth Constraint Model Id: 1 octet identifier for the
            Bandwidth Constraints Model currently in use by the LSR
            initiating the IGP advertisement.
               - Value 0 identifies the Russian Dolls Model specified in
            [DSTE-RDM].
               - Value 1 identifies the Maximum Allocation Model
            specified in [DSTE-MAM].

  When you do the above, it seems to me that those 2 documents become
  normative. And a stds track RFC can not make normative reference to
  experimental RFCs. See also my concerns about IANA considerations below

          - Bandwidth Constraints: contains BC0, BC1,... BC(N-1).

  explain what N is here, how is it determined? 

            Each Bandwidth Constraint is encoded on 32 bits in IEEE
            floating point format. The units are bytes (not bits!) per
            second. Where the configured TE-class mapping and the
            Bandwidth Constraints model in use are such that BCh+1,
            BCh+2, ...and BC7 are not relevant to any of the Class-Types
            associated with a configured TE-class, it is recommended that
            only the Bandwidth Constraints from BC0 to BCh be advertised,
            in order to minimize the impact on IGP scalability.

  I suggest to make citation/reference to IEEE floating point spec

- Then on next page (page 11) I read:
    A DS-TE LSR receiving the "Bandwidth Constraints" sub-TLV with a
    Bandwidth Constraint Model Id which does not match the Bandwidth
    Constraint Model it currently uses, MAY generate a warning to the
    operator reporting the inconsistency between Bandwidth Constraint
    Models used on different links. Also, in that case, if the DS-TE LSR
    does not support the Bandwidth Constraint Model designated by the
    Bandwidth Constraint Model Id, or if the DS-TE LSR does not support
    operations with multiple simultaneous Bandwidth Constraint Models,
    the DS-TE LSR MAY discard the corresponding TLV. If the DS-TE LSR
    does support the Bandwidth Constraint Model designated by the
    Bandwidth Constraint Model Id and if the DS-TE LSR does support
    operations with multiple simultaneous Bandwidth Constraint Models,
    the DS-TE LSR MAY accept the corresponding TLV and allow operations
    with different Bandwidth Constraints Models used in different parts
    of the DS-TE domain.
  
  What is are all these MAY cases???  What else can a DS-TE LSR do if it
  does not recognize or support a Model Id ??

- Section 6.2.1 should be more explicit.
  I think you need to specify that you want a Class-Number of TBD,
  a Class-Name of CLASSTYPE, a C-Type of 1.
  I think value zero should be stated to be reserved.
  
- Section 6.3
  I suspect that the reason why you reserve CT=0 and do not pass it
  ever in a CLASSTYPE object sis for backward compatibility?
  It would be good to make that explicit if that is the case.
  If not, then explain why.

- section 6.3 has:

    If a path message contains multiple CLASSTYPE objects, only the first
    one is meaningful; subsequent CLASSTYPE object(s) MUST be ignored and
    MUST not be forwarded.
  
  I think you mean "MUST NOT" instead of "MUST not" ??

- In section 6.4 I see:

    In both situations, this causes the path set-up to fail. The sender
    SHOULD notify management that a LSP cannot be established and
    possibly might take action to retry reservation establishment without
    the CLASSTYPE object.

  What is "management" in this case? Is it a management system ?

- I suspect that the Security Considerations section is to weak.
  Especially, text aka
    This document does not introduce additional security threats beyond
    those inherent to Diff-Serv and MPLS Traffic Engineering and the same
    security mechanisms proposed for these technologies are applicable
    and may be used. 

  So it is optional to use those mchanisms since you say "may be used"/
  Probably better to say something aka:

    This document does not introduce additional security threats beyond
    those described for Diff-Serv [add refrence] and MPLS Traffic
    Engineering [add reference] and the same security measures and
    procedures described in those documents apply here.

  Same comments on the rest of security considerations.

- issues with IANA Considerations.
  I bet IANA will have a HARD time to understand what they need to do.
  You must be much cleared.

  - for one thing you refer to section 5.3, which does not exist.
  - I would make separate subsections for each assignment in different
    namespaces. That makes it clearer.

  - you write:

      This document defines a number of objects with implications for IANA.

    This doc requests IANA to male assigments in a few namespaces.
    It also requests IANA to create and maintain a new namespace registry.

    The above 2 sentences would be section 14 if I had written this.

    Then I would have a subsection for each of the following

      This document defines in section 5.1 a new sub-TLV, the "Bandwidth
      Constraints" sub-TLV, for the OSPF "Link" TLV [OSPF-TE]. A sub-TLV
      Type in the range 10 to 32767 needs to be assigned by Expert Review.
      This sub-TLV Type also needs to be registered by IANA.
   
    RFC3630 states that values in 1--32767 are assigned by Standards Action
    (and not by Expert Review). So better would be:

      14.1 Bandwidth Constraints sub-TLV for OSPF version 2

      This document defines in section 5.1 a new sub-TLV, the "Bandwidth
      Constraints" sub-TLV, for the OSPF "Link" TLV [OSPF-TE]. The sub-TLV
      requested is in the range 10 to 32767 needs to be assigned by IANA.
      IANA has assigned TBD for the "Bandwidth Constraints" sub-TLV.

    Pls do a similar text for the ISIS sub-TLV assignment.

  - For:

      This document defines in section 5.3 a "Bandwidth Constraint Model
      Id" field within the "Bandwidth Constraints" sub-TLV. This document
      also defines in section 5.3 two values for this field (0 and 1).
      Future allocations of values in this space and in the range 2 to 127
      should be handled by IANA using the First Come First Served policy
      defined in [IANA]. Values in the range 128 to 255 are reserved for
      experimental use. 
  
    I feel that this document should NOT make the assignments for the
    RDM/MAM and MAR models. Those are experimental RFCs and I think that if
    we make assignments here, that we cause a normative ref to experimental
    RFC and that is a nono for a stds track document.

    Each Model document can claim (in its IANA section) an assignment from 

    So what we must do here is to specify the namespace, request IANA to
    create and maintain it, and specify the rules for making assignments.
    Se also my earlier comments on the ranges. I think that 128-255 for 
    experimental use is far too many. I think that something aka the following\
    makes much more sense:

      14.x A new namespace for Bandwidth Constraint Model Identifiers

      This document defines in section 5.1 a "Bandwidth Constraint Model
      Id" field (namespace) within the "Bandwidth Constraints" sub-TLV,
      both for OSPF and ISIS.

      IANA is requested to create and maintain this new namespace. The
      field for this namespace is 1 octet, and IANA guideliens for
      assigments for this field are as follows:
 
      o values in the range 0-64 are to be assigned via Standards Action
        as defined in [RFC2434]

      o values in the range 65-127 are to be assigned via Specification
        Required

      o values in the range 128-252 are not to be assigned at this
        time.  Before any assignments can be made in this range, there
        MUST be a Standards Track RFC that specifies IANA Considerations
        that covers the range being assigned.

      o values in the range 253-255 are for experimental use; these will
        not be registered with IANA, and MUST NOT be mentioned by RFCs.

    Or something like that.

  - Pls also reconsider the text for the RSVP-TE object and error codes
    in light of the above

  - If you can add the IANA web page that contains the current registry,
    then it will help IANA to more quickly understand what they need to
    do, and it will help future readers to quickly find the
    assignments repository.

Nits/admin issues

- there is no Table of Contents (required for docs > 15 pages
  see draft-rfc-editor-rfc2223bis-07.txt
- there is no IPR section (as per RFC2026) sect 10
- there are no copy-rigth statements (RFC-editor can add those)
- pls add and note RFC-Editor to remove the summary of SUB-IP
  related drafts (or just remove it by next rev)
- do not use citation in abstract (as per RFC-Editor instructions)
  The one that you use could be replaced by "(RFC3564)"
- update references section to point to current docs, for example
  te-reqs-07 is now RFC3564
- whenever you use acronyms the first time, pls make sure to
  also use the full text. SO in abstract spell out IGP and RSVP-TE
  in a similar way you did it for DS-TE.
  But this must be done throughout document. See RFc-Editor instructions
- The use of capitals in "Bandwidth Constraints Model" is very 
  inconsistent throught this document and also in the 3 experimental
  modle documents (RDM, MAM and MAR)
  Even sometimes it is "Constraints" (plural) and other times it is
  "Constraint" (singular). 
- sect 4.1.2 towards the end
     The "LSP Size Overbooking" method and the "Link size overbooking"
  pls be consustent in use of capitals
- In section 4.3.1 I see
    The configured Class-Type indicates, in accordance with the supported
    Bandwidth Constraint Model, what are the Bandwidth Constraints that
    MUST be enforced for that LSP.
  s/what are// ?? 
  sounds better in my ears... but maybe it is just me.
- section 4.3.3
  two times I see:
           specified is section 4.2.1 above
  s/is/in/ !!
- page 10, pls add a citation and reference to the IEEE floating point 
  format specification

- section 6.3
  Why do you use different editorial styles or textual formatting to
  describe how an LSR should handle the receipt of a CLASSTYPE object?

- in section 6.5
  You use Class-Type in full in the first few bullets and then switch
  to CT. Not very consistent.

- section 8, last para
  Quite a flood of terms to indicate that something is optional, no?
  Do you really think that this flood of words is very readable?
  For example, MAY means that something is optional according to RFC2119
  So why state 
    "... MAY also optionally..., when used, the optional additional.."?
  It seems to repeat the "MAY" or "optional" at least 3, maybe even 4 times
  in that one sentence.

- sect 10, 4th para
  s/an a bandwidth/a bandwidth/

- check spelling in sect 11.2

- APpendix A, might want toa dd a few words why it is included.
  What is a "Head-End" ?? May want to explain that

- appendix C 2nd line
  s/LSRs and not DS-TE/LSRs are not DS-TE/ ??

Thanks,
Bert