Re: [Pce] Benjamin Kaduk's Discuss on draft-ietf-pce-stateful-pce-auto-bandwidth-11: (with DISCUSS and COMMENT)

Benjamin Kaduk <kaduk@mit.edu> Thu, 19 September 2019 20:21 UTC

Return-Path: <kaduk@mit.edu>
X-Original-To: pce@ietfa.amsl.com
Delivered-To: pce@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id E0925120129; Thu, 19 Sep 2019 13:21:14 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -4.199
X-Spam-Level:
X-Spam-Status: No, score=-4.199 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9tEyr8520-ZV; Thu, 19 Sep 2019 13:21:11 -0700 (PDT)
Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id EEC5712006A; Thu, 19 Sep 2019 13:21:10 -0700 (PDT)
Received: from kduck.mit.edu ([24.16.140.251]) (authenticated bits=56) (User authenticated as kaduk@ATHENA.MIT.EDU) by outgoing.mit.edu (8.14.7/8.12.4) with ESMTP id x8JKKj5t005187 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 19 Sep 2019 16:20:55 -0400
Date: Thu, 19 Sep 2019 15:20:44 -0500
From: Benjamin Kaduk <kaduk@mit.edu>
To: Dhruv Dhody <dhruv.ietf@gmail.com>
Cc: The IESG <iesg@ietf.org>, draft-ietf-pce-stateful-pce-auto-bandwidth@ietf.org, Adrian Farrel <adrian@olddog.co.uk>, pce-chairs <pce-chairs@ietf.org>, pce@ietf.org
Message-ID: <20190919202036.GA48975@kduck.mit.edu>
References: <156874887441.17499.7855633347557404477.idtracker@ietfa.amsl.com> <CAB75xn5MUrTS43f5boMo4Kq7dfCo+YmLu1Qh_Qf-XN9hXLKsww@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAB75xn5MUrTS43f5boMo4Kq7dfCo+YmLu1Qh_Qf-XN9hXLKsww@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: <https://mailarchive.ietf.org/arch/msg/pce/TiqjwxGVa2hacY6t3m9QHiz3lqY>
Subject: Re: [Pce] Benjamin Kaduk's Discuss on draft-ietf-pce-stateful-pce-auto-bandwidth-11: (with DISCUSS and COMMENT)
X-BeenThere: pce@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Path Computation Element <pce.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/pce>, <mailto:pce-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/pce/>
List-Post: <mailto:pce@ietf.org>
List-Help: <mailto:pce-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/pce>, <mailto:pce-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 19 Sep 2019 20:21:15 -0000

Hi Dhruv,

On Wed, Sep 18, 2019 at 08:16:20PM +0530, Dhruv Dhody wrote:
> Hi Ben,
> 
> Thanks for your review. Please see inline...
> 
> On Wed, Sep 18, 2019 at 1:04 AM Benjamin Kaduk via Datatracker
> <noreply@ietf.org> wrote:
> >
> > Benjamin Kaduk has entered the following ballot position for
> > draft-ietf-pce-stateful-pce-auto-bandwidth-11: 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-pce-stateful-pce-auto-bandwidth/
> >
> >
> >
> > ----------------------------------------------------------------------
> > DISCUSS:
> > ----------------------------------------------------------------------
> >
> > This document seems generally useful, and thanks for it.  I think
> > there's a couple places where it's ambiguous or unclear, and hopefully
> > we can tighten up the language without needing extensive changes.
> >
> > There may be an internal inconsistency in Section 2.3's definitions
> > relating the Overflow-Count and the Overflow-Threshold: the description
> > of Overflow-Count reads as if it is a reported quantity, but the
> > description of Overflow-Threshold implies that Overflow-Count is a
> > configured parameter.  (Similarly for the Underflow- variants.)
> > Specifically, the sentence "[t]his value indicates how many times
> > consecutively, the percentage or absolute difference between the current
> > MaxAvgBw and the current bandwidth reservation of the LSP is greater
> > than or equal to the Overflow-Threshold value" uses the descriptive
> > verb "is" as opposed to a conditional such as "needs to be [...] in
> > order to [...]".
> >
> 
> Updated.
> 
> > Section 5.2.3.2 says:
> >
> >    If the percentage difference between the current MaxAvgBw and the
> >    current bandwidth reservation is greater than or less than or equal
> >    to the threshold percentage, the LSP bandwidth is adjusted to the
> >    current bandwidth demand (MaxAvgBw) (as long as the difference in the
> >    bandwidth is at least or above the Minimum-Threshold).
> >
> > As written ("greater than or less than or equal to"), this is always
> > true, which cannot be what was intended.  Probably we should reword to
> > just "greater than" and talk about the percentage absolute difference,
> > i.e., 100 * abs(MaxAvgBw - CurrentRes) / CurrentRes .
> >
> 
> Updated.
> 
> >
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> >
> > As a general note, I'm not sure the phrase "calculated bandwidth to be
> > adjusted" is a great fit for how it's currently being used.
> > Specifically, the "calculation" in question seems to just be taking the
> > maximum of a sliding window of per-timeslice average bandwidth values,
> > and this seems like a sufficiently small amount of computation that I'd
> > be comfortable describing it as a "measurement", for "measured bandwidth
> > as input to path calculation".  That is, this doesn't seem like it comes
> > close to the amount of computation involved in actually computing paths,
> > so I keep misreading what information is being sent where.
> >
> 
> In an earlier version there was a feature where the PCC could report
> the measured value at each sample interval (it moved to another
> document). IMHO for the clarity in the WG, it is still better to
> differentiate between the raw measured value (telemetry) and the value
> that is currently reported which is marked as "calculated".

Ah, in that context it makes more sense to do it this way.  Perhaps an
informative reference to the other document/usage and note about the
terminology would help, though?

> > Section 1
> >
> >    Over time, based on the varying traffic pattern, an LSP established
> >    with a certain bandwidth may require to adjust the bandwidth reserved
> >    in the network dynamically.  The head-end Label Switch Router (LSR)
> >    monitors the actual bandwidth demand of the established LSP and
> >    periodically computes new bandwidth.  The head-end LSR adjusts the
> >    bandwidth reservation of the LSP based on the computed bandwidth
> >    automatically.  This feature is commonly referred to as Auto-
> >    Bandwidth.  The Auto-Bandwidth feature is described in detail in
> >    Section 4 of this document.
> >
> > >From just this text, I can't tell if this is describing the mechanisms of
> > this document or an existing Auto-Bandwidth feature that is related to
> > the mechanisms of this document.
> >
> 
> The auto-bandwidth feature without PCE exists, where the ingress does
> the measurement as well as path computation. This feature has been
> implemented long back but was not part of any standard because it was
> a local implementation at the ingress node and no protocol changes was
> required. With the PCE things changed and here we are...

Ah, I see.  Maybe "This feature, when available in the head-end LSR
implementation, is common referred to as [...]", then?

> >    In the model considered in this document, the PCC (head-end of the
> >    LSP) collects the traffic rate samples flowing through the LSP and
> >    calculates the new adjusted bandwidth.  The PCC reports the
> >    calculated bandwidth to be adjusted to the PCE.  This is similar to
> >    the Passive stateful PCE model, while the Passive stateful PCE uses
> >    path request/reply mechanism, the Active stateful PCE uses
> >    report/update mechanism.  [...]
> >
> > nit: I think the punctuation in this last sentence needs some tweaking,
> > as there looks to be a comma splice at present.
> > (It's also unclear how much we need to talk about Passive stateful PCE,
> > since just two paragraphs ago we say we focus on Active stateful PCE in
> > this document.)
> >
> 
> This was updated based on Barry's suggestion. It makes sense to
> include a sentence here in Introduction to set the stage.

Barry's text looks great, thanks.

> >    This document defines the PCEP extensions needed to support Auto-
> >    Bandwidth feature in a Active stateful PCE model where the LSP
> >    bandwidth to be adjusted is calculated on the PCC (head-end of the
> >    LSP). The use of PCE to calculate the bandwidth to be adjusted is out
> >    of scope of this document.
> >
> > Maybe I'm just confused about active vs. passive in general, but I
> > thought that active stateful PCE involved the PCE "making the decisions"
> > with the PCC just providing input data.  But here we're saying that the
> > PCC is making the bandwidth decision, even though the rest of the LSP
> > computation remains on the PCE?
> 
> Yes, this is correct. The calculated bandwidth is still an input for
> PCE to make the path computation taking that as an input.
> 
> > Or is "calculate the bandwidth to be
> > adjusted" referring to "evaluate the expression max_{time periods}(avg.
> > bandwidth per time period)" (as opposed to "compute the bandwidth
> > adjustment to make)?
> >
> 
> It is the former.
> 
> > Section 2.3
> >
> > Are we assuming any relationship between the Up-Adjustment-Threshold and
> > the Overflow-Threshold?
> >
> 
> Yes, as mentioned here while describing Overflow-Threshold -
> 
>       The LSP bandwidth is adjusted to the
>       current bandwidth demand bypassing the Up-Adjustment-Interval if
>       the overflow condition is met consecutively for the Overflow-
>       Count.

I was thinking more along the lines of if the Overflow-Threshold value was
going to be a larger number than the Up-Adjustment-Threshold.

> 
> >       traffic demand.  If the percentage or absolute difference between
> >       the current MaxAvgBw and the current bandwidth reservation of the
> >       LSP is greater than or equal to the threshold value, the overflow
> >       condition is set to be met.  The LSP bandwidth is adjusted to the
> >
> > nit: I think s/set to be met/said to be met/ ?  (this appears twice)
> >
> 
> Ack
> 
> >    Minimum-Threshold:  The increase or decrease of the LSP bandwidth
> >       should be at least or above the minimum-threshold represented as
> >       an absolute bandwidth value before the bandwidth adjustment for
> >       the LSP is made.  This threshold can be seen as a suppression
> >       threshold that is used along with a percentage threshold to avoid
> >       unnecessary auto-bandwidth adjustments and re-signaling of the LSP
> >       at low bandwidth values.
> >
> > I can't tell from this text if this threshold is a (LSP) bandwidth
> > value/measurement or a bandwidth offset between configured and measured
> > values.  (The subsequent protocol element definitions are more clear
> > that it's the latter.)
> >
> 
> It is latter. Do you have a suggested change, I am not sure what change to make.

How about:

   Minimum-Threshold:  When percentage-based thresholds are in use, they
      are accompanied by this minimum threshold, which is used to enforce
      that the magnitude of deviation of calculated LSP bandwidth from the
      reserved bandwidth exceeds a specific non-percentage-based criterion
      before any adjustments are made.  This serves to suppress unnecessary
      auto-bandwidth adjustments and re-signaling of the LSP at low
      bandwidth values.


> > Section 3
> >
> > The column headings in Table 1 refer to who initiates the LSP in
> > question, right?
> >
> 
> yes
> 
> >    The PCEP speaker supporting this document must have a mechanism to
> >    advertise the automatic bandwidth adjustment capability for both PCC-
> >    Initiated and PCE-Initiated LSPs.
> >
> > Just checking on "must" vs. "MUST" here.
> >
> 
> The use of normative for requirements for the protocol extensions may
> not be required. .
> 
> >    o  It is required to identify and inform the PCC, which LSPs are
> >       enabled with Auto-Bandwidth feature.  Not all LSPs in some
> >       deployments would like their bandwidth to be dependent on the
> >       real-time bandwidth usage but be constant as set by the operator.
> >
> > nit: I suggest NEW:
> >
> > %  o  It is required to identify and inform the PCC which LSPs have
> > %     enabled the Auto-Bandwidth feature.  Not all LSPs in some
> > %     deployments would like their bandwidth to be dependent on the
> > %     real-time bandwidth usage; for some LSPs leaving the bandwidth
> > %     constant as set by the operator is preferred.
> >
> 
> Ack
> 
> > Section 4.2
> >
> >    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, when the PCC is the head-end node of the LSP.  The traffic rate
> >
> > Do we cover what happens in the case when the PCC is not the head-end
> > node of the LSP?
> >
> 
> No.

Should we document that somewhere?

> > Section 5.1
> >
> >    o  The PCEP speaker that does not recognize the extensions defined in
> >       this document sends the PCErr message with error-type 2
> >       (capability not supported) as per Section 6.9 in [RFC5440].
> >
> > This refers to when the actual new messages (e.g.,
> > AUTO-BANDWIDTH-ATTRIBUTES TLV) are being used, not merely advertising
> > the Auto-Bandwidth Capability TLV in the OPEN object, right?  That is,
> > this is descriptive language of what the core protocol spec mandates as
> > error handling in the presence of unrecognized input?
> >
> 
> This is a mistake, that error is only for unknown message. Will fix this.
> 
> > Section 5.1.1
> >
> > nit: in the previous section we captialized this as "Auto-Bandwidth
> > Capability", but here (including the section heading) it is
> > "AUTO-BANDWIDTH-CAPABILITY".  Should they be consistent?
> >
> 
> Updated
> 
> > Section 5.2
> >
> >    Future specification can define additional sub-TLVs.
> >
> > I see that we see later that unrecognized sub-TLVs are ignored, but
> > would there be a case where we want to use a bit in the
> > AUTO-BANDWIDTH-CAPABILITY flags to negotiate support for a sub-TLV
> > (i.e., to confirm that the peer will understand it, instead of just
> > sending it and not knowing if it is understood)?
> >
> 
> I guess a future extension that defines any new sub-TLV for
> auto-bandwidth could take that approach.
> 
> > Section 5.2.1
> >
> > It sounds like we should ignore values larger than 7 days as well
> > (rather than erroring out)?
> >
> 
> Updated
> 
> > Section 5.2.2, 5.2.3
> >
> > I'd like to see a little bit more detail specified of the interaction
> > between (regular) Adjustment-{Interval,Threshold} and
> > Down-Adjustment-{Interval,Threshold}.  We do have this text about (e.g.)
> > "[t]he Adjustment-Interval sub-TLV specifies the time interval for both
> > upward and downward trend", which conveys the general intent, but I'm
> > not sure I have all the details about there being one default value,
> > that applies to both, and changes to the (regular) parameter continue to
> > apply to both, until an explicit Down- variant is seen, at which point
> > the control of the two parameters diverge permanently and they must
> > always be separately controlled.
> >
> 
> Updated
> 
> > Section 5.2.3.1
> >
> >    o  Adjustment-Threshold: The absolute Adjustment-Threshold bandwidth
> >       value, encoded in IEEE floating point format (see
> >
> > nit: I'd suggest that this is a bandwidth *difference* value (or offset),
> > pedantically speaking.  (The following paragraph is quite clear on the
> > actual usage, though, so this is nit-level.)  This comment applies to
> > basically all the threshold fields in the document, but I'll just make
> > it once, here.
> >
> 
> Updated
> 
> > Section 5.2.3.2
> >
> >    o  Reserved: SHOULD be set to zero on transmission and MUST be
> >       ignored on receipt.
> >
> > I think the formulation of "MUST be set to zero on transmission and
> > SHOULD be ignored on receipt" is more common.
> >
> 
> Changed to MUST for both.
> 
> >    o  Percentage: The Adjustment-Threshold value (7 bits), encoded in
> >       percentage (an integer from 1 to 100).  The value 0 is considered
> >       to be invalid.  The default value is 5 percent.
> >
> > I assume that the question of whether to allow sub-percentage
> > granularity came up, and don't want to revisit the WG's decision.  But
> > please confirm that it was discussed.
> >
> 
> I don't have a thread to point to where this was explicitly talked
> about. But I believe we have WG consensuses for this.

That's good enough for me, thanks.

> > Also, are values over 100 also "invalid"?  What should the recipient do
> > upon receipt of an invalid value?
> >
> >    If the percentage difference between the current MaxAvgBw and the
> >    current bandwidth reservation is greater than or less than or equal
> >    to the threshold percentage, the LSP bandwidth is adjusted to the
> >    current bandwidth demand (MaxAvgBw) (as long as the difference in the
> >    bandwidth is at least or above the Minimum-Threshold).
> >
> > I'd suggest rewording to make the two criteria equal peers (e.g., "If X
> > and Y") rather than relegating the minimum-threshold to a parenthetical
> > that's easy to ignore.
> >
> 
> Ack
> 
> > Section 5.2.3.3
> >
> > It would probably be good to reiterate here (and at other similar
> > locations) that this "is used to decide" only when there's a need for
> > different behavior in the upward and downard directions.
> >
> > Also, nit-level, I think "overrides" is more appropriate than
> > "overwrites", since the non-Down- variant remains active for its
> > separate role.
> >
> 
> Ack
> 
> > Section 5.2.4.2
> >
> >       per second.  The default maximum-bandwidth value is not set.
> >
> > I guess effectively the default is FLOAT_MAX, inherited from RFC 5440.
> >
> 
> Yes
> 
> > Section 5.2.5.1
> >
> >    o  Overflow-Threshold: The absolute Overflow-Threshold bandwidth
> >       value, encoded in IEEE floating point format (see
> >       [IEEE.754.1985]), expressed in bytes per second.  Refer to Section
> >       3.1.2 of [RFC3471] for a table of commonly used values.  If the
> >       increase of the current MaxAvgBw from the current bandwidth
> >       reservation is greater than or equal to the threshold value, the
> >       overflow condition is met.
> >
> > nit: I'd suggest to s/increase/difference/, since (IIUC) the operation
> > here is "observed bandwidth remains above the (reservation plus)
> > threshold for <count> times", not "observed bandwidth increases in
> > successive reporting intervals".  This assumes that the
> > Adjustment-Interval is larger than <count> times the reporting interval,
> > of course.
> >
> 
> Updated
> 
> > Section 5.2.5.3
> >
> >    o  Underflow-Threshold: The absolute Underflow-Threshold bandwidth
> >       value, encoded in IEEE floating point format (see
> >       [IEEE.754.1985]), expressed in bytes per second.  Refer to Section
> >       3.1.2 of [RFC3471] for a table of commonly used values.  If the
> >       decrease of the current MaxAvgBw from the current bandwidth
> >       reservation is greater than or equal to the threshold value, the
> >       underflow condition is met.
> >
> > As above, I suggest s/decrease/difference/ (perhaps with an additional
> > indication that the magnitude of the difference is to be used, not the
> > signed difference).
> >
> 
> Updated
> 
> > Section 5.7
> >
> > I'm not sure I understand why there's a need for an
> > auto-bandwidth-specific "overload" notification, as opposed to having
> > such messages suppressed as part of a more general overload condition.
> > But that doesn't mean there's not a need, of course, as I'm not very
> > familiar with this area!
> >
> 
> The auto-bandwidth feature increases the rate of message exchange and
> path computation. So in case of an overload because of auto-bandwidth
> it would be good for a PCEP speaker to indicate that. Also the general
> overload only worked in one direction 'PCE to PCC'. Here PCE could
> send to many updates to PCC and making PCC to be overloaded as well.

Thanks for the explanation; I understand better now.

> > Section 6.6
> >
> >    An implementation MAY allow a limit to be placed on the rate of auto-
> >    bandwidth related messages sent by a PCEP speaker and received by a
> >    peer.  An implementation MAY also allow sending a notification when a
> >    PCEP speaker is overwhelmed or the rate of messages reach a
> >    threshold.
> >
> > I don't want to revisit a WG consensus outcome, but I could see "SHOULD"
> > for "allow sending a notification", noting that it is "SHOULD allow
> > sending" and not "SHOULD send".
> >
> 
> Updated
> 
> > Section 7
> >
> >    This document defines AUTO-BANDWIDTH-CAPABILITY TLV and AUTO-
> >    BANDWIDTH-ATTRIBUTES sub-TLVs which do not add any new security
> >    concerns beyond those already discussed in [RFC8231] and [RFC8281]
> >
> > I'd suggest to qualify this as "do not add any substantial new security
> > concerns".  We did just in the previous section talk about how this
> > functionality could increase computational and operational load, which,
> > taken to an extreme, can affect availability and could be considered a
> > security consideration.
> >
> > We could also perhaps give guidance about setting the min and max
> > bandwidth sub-TLVs to constrain automatic adjustment to be within a
> > known range and thus bound the extent of second-order effects on the
> > rest of the MPLS-TE domain.
> >
> 
> Updated
> 
> > Section 8.2
> >
> > It might be good to say within what registry the sub-registry being
> > created is to be located.
> >
> > Also, the last paragraph could perhaps be rephrased as "the initial
> > contents of the registry are empty, with all bits unassigned".
> >
> 
> Ack
> 
> > Section 9.2
> >
> > I think that the RFC 7525 and 8253 references fall under the RECOMMENDED
> > guidance in Section 7, in which case per
> > https://www.ietf.org/about/groups/iesg/statements/normative-informative-references/
> > they should be classified as Normative references.  (It's probably also
> > good to cite RFC 7525 as BCP 195 and not just RFC 7525.)
> >
> > The IEEE.754 reference is also normative since we use its wire encoding
> > (but it could practically be considered "well-known" at this point,
> > given its ubiquity).
> >
> >
> 
> Ack
> 
> Diff: https://tools.ietf.org/tools/rfcdiff/rfcdiff.pyht?url1=draft-ietf-pce-stateful-pce-auto-bandwidth-11&url2=https://raw.githubusercontent.com/dhruvdhody/ietf/master/draft-ietf-pce-stateful-pce-auto-bandwidth-12.txt
> 
> Working Copy: https://raw.githubusercontent.com/dhruvdhody/ietf/master/draft-ietf-pce-stateful-pce-auto-bandwidth-12.txt

Thanks!

-Ben