Re: [Pce] Shepherd review of draft-ietf-pce-stateful-pce-auto-bandwidth-08

[Rakesh Gandhi <[e-mail]>]

 Thanks Adrian for the detailed review and suggestions.  Please see replies
inline with <RG>...



On 2019-04-17, 11:03 PM, "Adrian Farrel" <[e-mail]> wrote:



    Hello authors,



    Sorry about the clunk as this draft shifted between shepherd.



    I have some fairly minor review comments (below). Could you please
address

    these in a new revision.



    Meanwhile, I will start on the shepherd write-up ready to move ahead as

    quickly as possible.



    Thanks,

    Adrian

    ---



    Odd leading space on 4th line of Abstract



<RG> Fixed.

    ---



    Abstract para 2

    s/Automatic bandwidth/Automatic bandwidth adjustment/ ??





<RG> Added – “The automatic bandwidth feature”

    ---



    2.3

    Down-Adjustment-Interval

    s/lesser/less/



<RG> Fixed.

    ---



    I found the definition of Maximum Average Bandwidth and the definitions

    of Up/Down-Adjustment-Interval to be circular. Unless, perhaps, the

    definition of Adjustment-Interval is missing.



    That is, Maximum Average Bandwidth is defined as

      max {Bandwidth-Sample(i)} for each sample interval i in the

                                Adjustment-Internal

    But Up/Down-Adjustment-Interval both appear to be dependent on Maximum

    Average Bandwidth.



<RG> Removed the Adjustment-Interval from the Max Average Bandwidth
definition. That should remove the circular dependency.

    ---



    3.

    s/the PCC, the LSP that/the PCC, which LSPs/



<RG> Fixed.

    ---



    4.1

    OLD

       Auto-Bandwidth feature allows automatic and dynamic adjustment of the

       reserved bandwidth of an LSP over time, i.e. without network operator

       intervention to accommodate the varying traffic demand of the LSP.

    NEW

       The Auto-Bandwidth feature allows automatic and dynamic adjustment of

       the reserved bandwidth of an LSP over time (i.e., without network

       operator intervention) to accommodate the varying traffic demand of

       the LSP.

    END



<RG> Fixed.

    ---



    4.1 has...



       The bandwidth

       adjustment uses the make-before-break (MBB) signaling method so that

       there is no disruption to the traffic flow carried by the LSP.



    I think this should be...



       Bandwidth adjustment must not cause disruption to the traffic flow

       carried by the LSP.  One way to achieve this is to use the make-

       before-break (MBB) signaling method.



    This is the softest way I can think of saying what you don't want to

    say which is that RSVP-TE signaling supports in-place bandwidth

    adjustmnt simply by sending a new Path message. (Noting that failure to

    make the adjustment can be seen either in a non-fatal PathErr or in a

    Resv with unchanged bandwidth.)



<RG> Fixed.



    Similarly in 4.3 maybe...



    OLD

       It should be noted that any bandwidth change requires re-signaling of

       an LSP in a make-before-break fashion, which can further trigger

       preemption of lower priority LSPs in the network.

    NEW

       It should be noted that any bandwidth change requires re-signaling of

       an LSP, which can further trigger preemption of lower priority LSPs

       in the network.

    END



<RG> Fixed.

    ---



    4.2 has...



       When the Auto-Bandwidth feature is enabled, the measured traffic rate

       is periodically sampled at each Sample-Interval (which can be

       configured by an operator and the default value as 5 minutes) by the

       PCC which is the head-end node of the LSP.



    As you know, in the PCE architecture, the PCC is not necessarily the

    head-end LSR. You need either to restrict this function to only

    operating when the PCC *is* the head-end LSR, or you need to re-word

    slightly.  Either works for me.



<RG> Updated. “by the PCC, when the PCC is the head-end node of the LSP.”



    ---



    I wonder whether you intend that Section 4 (in particular Section 4.2)

    is normative in this document. It could be that you are just describing

    the procedures in a general way - in which case a one line statement of

    that would address my concern. Or it could be that you intend to define

    how auto-bandwidth adjustment must be implemented.



    That is, the document claims to describe changes to PCEP to support

    auto-bandwidth, but this section appears to be describing how auto-

    bandwidth must be implemented, and I don't think I agree that the

    description is completely wise.



<RG> Added – “This section describes the Auto-Bandwidth feature in a
general way.”





    For example...



       The

       PCC, in-charge of calculating the bandwidth to be adjusted, will

       adjust the bandwidth of the LSP to the highest traffic rate sample

       (MaxAvgBw) amongst the set of bandwidth samples collected over the

       adjustment-interval period (in the Up or Down direction).



    ...means that a single spike in a potentially very small window over

    a potentially very large period (defaults are 5 minutes in 24 hours)

    could see a readjustment of the LSP's bandwidth. But such a spike could

    easily be a freak and it seems unwise to take bandwidth out of the

    network for it without allowing the application of operator policy.





<RG> Reworded the sentence.  “The PCC, in-charge of calculating the

   bandwidth to be adjusted, can decide to adjust the bandwidth of the

   LSP to the highest traffic rate sample depending on the operator policy”





    More generally, if this document is defining auto-bandwidth as an

    MPLS-TE function, it belongs in TEAS (or possibly MPLS), while if it is

    defining PCEP extensions, it is fine where it is.



    ---



    5.2

    s/PCEP peer the/PCEP peer of the/



<RG> Added.



    ---

    Section 5.2



    There are various MUSTs and MUST NOTs for the attribute values, there is

    a need to include a description of the error processing when these

    conditions

    are not met.



<RG> Added –

“ The adjustment-interval parameter MUST NOT be less than the

sample-interval, otherwise the Sub-TLV MUST be ignored and the

previous value is maintained.”





    --



    Section 5.2



    Since the AUTO-BANDWIDTH-ATTRIBUTES TLV is a MUST for all LSPs with

    Auto-BW feature, do we want to encode all the sub-TLVs every-time in all

    PCEP messages?



    Instead, could we say that sub-TLV is only included if there is a change

    between the last information and now? The default values for missing

    sub-TLVs apply for the first PCEP message for the LSP?



    --

<RG> Added.

    ---



    5.2.1



    You might explain that 604800 seconds is 7 days.



<RG> Added.

    ---



    5.2.3

    s/PCEP peer the/PCEP peer of the/



<RG> Fixed.



    ---



    5.2.3



    Is it worth noting that regardless of how the threshold are set, the

    adjustment will not be made until at least one sample-interval simply

    because no sample will be made on which to base a comparison with a

    threshold?



<RG> Added.

    ---



    5.2.3.1



       If the difference between the current MaxAvgBw and the current

       bandwidth reservation is greater than or less than or equal to the

       threshold value, the LSP bandwidth is adjusted to the current

       bandwidth demand (MaxAvgBw).



    "greater than or less than or equal to"



    You mean regardless of the setting of this threshold value, the LSP

    bandwidth is adjusted?



    No, you don't mean that!



    How about...



       If the modulus of difference between the current MaxAvgBw and the

       current bandwidth reservation is greater than or equal to the

       threshold value, the LSP bandwidth is adjusted to the current

       bandwidth demand (MaxAvgBw).



<RG> Updated.



    ---



    6.  Security Considerations





    Please expand this section a little.  You should certainly include
RFC8281

    as reference. Also include more on TLS use.



<RG> Updated.



    --



    8.3. AUTO-BANDWIDTH-ATTRIBUTES Sub-TLV



    Consider making a **few** code points for experiments into this sub-TLV

    range?



<RG> Added.

    -



    It says that "AUTO-BANDWIDTH-ATTRIBUTES Sub-TLV Types"

    is a sub-registry in the "PCEP TLV Type Indicators", which is not the
case.



    OLD:

       This document specifies the AUTO-BANDWIDTH-ATTRIBUTES Sub-TLVs.  IANA

       is requested to create an "AUTO-BANDWIDTH-ATTRIBUTES Sub-TLV Types"

       sub-registry in the "PCEP TLV Type Indicators" for the sub-TLVs

       carried in the AUTO-BANDWIDTH-ATTRIBUTES TLV.  New sub-TLV are

       assigned by Standards Action [RFC8126].

    NEW:

       This document specifies the AUTO-BANDWIDTH-ATTRIBUTES Sub-TLVs.  IANA

       is requested to create an "AUTO-BANDWIDTH-ATTRIBUTES Sub-TLV Types"

       sub-registry within the "Path Computation Element Protocol (PCEP)

       Numbers" registry to manage the type indicator space for sub-TLVs of

       the AUTO-BANDWIDTH-ATTRIBUTES TLV.  New sub-TLV are assigned by

       Standards Action [RFC8126]. The valid range of values in the registry

       is 0-65535.  IANA is requested to initialize the registry with the

       following values.  All other values in the registry should be marked

       as "Unassigned".

    END



<RG> Updated.

    --



    Nits



    expand RBNF, LSPA



<RG> Fixed.



Thanks,

Rakesh