Subject: [Gen-art] Genart last call review of draft-ietf-rtgwg-backoff-algo-07

From: Elwyn Davies <[email-masked]>
Date: Thu, 15 Feb 2018 11:12:08 -0800

Reviewer: Elwyn Davies
Review result: Ready with Nits

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-rtgwg-backoff-algo-07.txt
Reviewer: Elwyn Davies
Review Date: 2018/02/15
IETF LC End Date: 2018/02/14
IESG Telechat date: 2016/02/22

Summary: Ready with nits. The draft does not refer to OSPFv3 - i am not sure if
this is an oversight or because ODSPFv3 already has this mechanism - either way
it should be mentioned.  One question that occurred to me is whether the draft
could be considered as updating the OSPFv2/v3 and ISIS standards (not that IETF
has any control over ISIS).

Major issues:
None

Minor issues:
(Non-)Relation between HOLDDOWN_INTERVAL and *_SPF_DELAY values:  I notced that
Benjamin Kaduk's SECDIR review of this document
(https://datatracker.ietf.org/doc/review-ietf-rtgwg-backoff-algo-07-secdir-lc-kaduk-2018-02-14/)
was concerned that certain state transitions would never occur.  I loooked at
this and realized that his assumption that LONG_SPF_DELAY < HOLDDOWN_INTERVAL
is not required by the document and s6 explicitly resiles from offering
suggested default values.  Without this assumption, the state machine appears
to be correct. Not being familiar with the consequences of setting the
HOLDDOWN_INTERVAL relative to the *_SPF_DELAY, I am not sure if anything could
be said about such consequences, but I think it would avoid other people making
the same assumption as the SECDIR reviewer if it was explicitly stated that
HOLDDOWN_INTERVAL is not necessarily bigger than any of the *_SPF_DELAY values
and adding any advice from experience about how to choose appropriate values. 
This might also avoid naive implementers shortcutting the state machine
implementation if they made the same assumption.

Requirements Language: Suggest s/RFC2119/RFC8174/ as there are uses of lower
case versions of the reserved words.

Default values for parameters:  There is a possible conflict between s3, where
example values for the various interval parameters are given and s6 which
states that no default values are specified in the document.  The difference in
termnology maybe too subtle for some implementers.

Aborting or otherwise of SPF calculation if an IGP event occurs while an SPF
calculation is in progress.  A note about whether this should happen (if it is
possible) would be desirable.

OSPFv3: Does this (not) equally apply to OSPF v3 for IPv6?  If so it should be
mentioned and RFC 5340 included in the references.

s12:  I suspect (although it could be arguable) that the ISIS definition, RFC
2328 (OSPFv2) and (if added) RFC5340 are normative as you need to understand
how they work.  This work could even be considered to update these documents.

Nits/editorial comments:
General: The term 'back-off' may not be familiar to non-Emglish mother tongue
speakers and on first occurrence needs a little explanation for naive readers
to indicate what it means and to what the back-off is being applied.  I have
suggested some additional text to this end for the abstract and s1.

Abstract:
OLD:
   This document defines a standard algorithm to back-off link-state IGP
   Shortest Path First (SPF) computations.
NEW:
   This document defines a standard algorithm to temporararily postpone or
   'back-off' link-state IGP Shortest Path First (SPF) computations to reduce
   the computational load on IGP nodes if network events occurring at closely
   spaced times would otherwise lead to multiple, essentially redundant
   recalculations of the routing tables.
ENDS

s1, para 1: s/at the same time/essentially at the same time/

s1, para 2: s/new Shortest Path First (SPF)/new Shortest Path First (SPF)
routing table/

s1, para 2:
OLD:
   experiencing multiple temporally close failures over a short
   period of time
NEW:
   experiencing multiple temporally close failures (that is, eventuating over a
   short period of time)
ENDS

s1, para 2: There is a right bracket missing in the following and starting a
clause with 'such as' and ending it with an ellipsis ('...') is redundant. >   
such as LDP [RFC5036], RSVP-TE [RFC3209], >    BGP [RFC4271], Fast ReRoute
computations (e.g.  Loop Free Alternates >    (LFA) [RFC5286], FIB updates...
It is unclear to me where the bracket should go: maybe after [RFC5286] or at
the end. Please clarify.

s1, para 2: the phrase
> This also reduces the churn on
>    routers and in the network and.
is useless, vague jargon.  The previous sentence expresses what I suspect is
meant by 'churn'. so this is redundant and can be omitted.

s1, para 3:
OLD:
To allow for this, IGPs implement an SPF back-off algorithm.
NEW:
To allow for this, IGPs usually implement an SPF back-off algorithm that
postpones or backs-off the running of the SPF calculation when the algorithm
predicts that a run would be essentially redundant or even counter-productive
because it appears that multiple closely timed routing-affecting events can be
expected. ENDS

s1, para 3: s/choosen/chosen/

s2, last bullet: SPF_DELAY is not defined at this point:
s/SPF_DELAY timers values/values for any timers used to back-off SPF
calculations/

s2, last bullet:  s/Even though/This is important even though/

s3, para 1: Undesirable ellipsis:
s/a metric change on a link or prefix.../and a metric change on a link or
prefix./

s3:Need to expand SRLG on first use - it isn't deemed to be well-known.

s3, INITIAL_SPF_DELAY bullet: s/A very small delay to quickly handle link
failure/A very small delay to quickly handle a single isolated link failure/

s3, SHORT_SPF_DELAY bullet:
OLD:
    SHORT_SPF_DELAY: A small delay to have a fast convergence in case of
    a single failure (node, SRLG..), e.g., 50-100 milliseconds.
NEW:
    SHORT_SPF_DELAY: A small delay to provide fast convergence in the case of
    a single component failure (node, SRLG..) that leads to multiple IGP events,
    e.g., 50-100 milliseconds.
ENDS

s5/s5.1: There is currently no text in s5: this is generally considered
inappropriate.  Suggest removing the first sentence in s5.1 ("This section
describes the state machine.") and adding to s5: NEW: This section describes
the abstract finite state machine (FSM) intended to control the timing of the
running of SPF calculations in response to IGP events.

s5.1, QUIET bullet: s/occured/occurred/

s5.2:  There is no need for 3 expansions of FSM - the expansion can be moved to
s5 as suggested above.

s5.3 title: s/States/State/

s6, next to last para: s/it's RECOMMENDED to play it safe/it is recommended
that timer intervals should be chosen conservatively/ (this is an operational
recommendation).

s6, last para: s/RECOMMENDED/recommended/ (ditto).

s7, para 1: s/is based on/is dependent on/, s/RECOMMENDED/recommended/
(operational again)

s8: Other documents (e.g., from vendors) have used the terms SPF wait time and
SPF hold time.  It might be useful to mention that this document essentially
provides ways to implement these settings.

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Fri, 16 Feb 2018 00:30:42 +0000

Hi Elwyn, 

On 2/15/18, 2:12 PM, "Elwyn Davies" <[email-masked]> wrote:

    Reviewer: Elwyn Davies

    

    
    Major issues:
    None
    
    Minor issues:

The definition of HOLDDOWN_INTERVAL explicitly states: 

HOLDDOWN_INTERVAL: The time required with no received IGP events
   before considering the IGP to be stable again and allowing the
   SPF_DELAY to be restored to INITIAL_SPF_DELAY. e.g., 3 seconds.  The
   HOLDDOWN_INTERVAL MUST be defaulted or configured to be longer than
   the TIME_TO_LEARN_INTERVAL.

Perhaps, it should be restated in the third paragraph of section 6. 

I believe this was brought up before and we will make this change. If not, we'll change to the RFC 8174 language. 

I would expect those implementing IGPs to know the different between an "example" and a "default". 

This is certainly out scope and I'm not sure why anyone would deduce from this draft that an implementation should or shouldn't do this. 

Yes. This will be added as reference. 

While implemented from the beginning, SPF Backoff is not specified by the IGP protocol specifications. 

Bruno and I will look at the editorial comments and let you know which ones we do and don't incorporate. 

Thanks,
Acee 
    
    Nits/editorial comments:
    
    Abstract:
    OLD:

    
    s1, para 2:
    OLD:

    s1, para 2: the phrase
    
    s1, para 3:
    OLD:

---

From: <[email-masked]>
Date: Fri, 16 Feb 2018 14:00:04 +0000

Hi Elwyn, Acee,

Thanks for your review and comments.
Please see inline [Bruno]

 > -----Original Message-----
 > From: Acee Lindem (acee) [mailto:[email-masked]]
 > Sent: Friday, February 16, 2018 1:31 AM
 > To: Elwyn Davies; [email-masked]
 > Cc: [email-masked]; [email-masked]; [email-masked]
 > Subject: Re: Genart last call review of draft-ietf-rtgwg-backoff-algo-07
 > 
 > Hi Elwyn,
 > 
 >     it should be mentioned.

[Bruno] This is an oversight and has been added.

 >   One question that occurred to me is whether the draft
 
[Bruno] I personally don't think so. I'd leave this to OSPF/IS-IS/LSR chairs and routing AD.
 
 >     be said about such consequences, 

[Bruno] There is no "significant" bad consequence.
Thinking about this, I could see 2 consequences:
- There could be a SPF computation scheduled after the HOLDDOWN_INTERVAL. I don't see this as an issue as, as stated below by Acee, the HOLDDOWN_INTERVAL is defined as related to the period with no IGP events. It's not defined as the period with no SPF computations (not to mention further computations e.g., from BGP).
- Perhaps more  importantly, if an IGP event occurs at t1 in the interval [LONG_SPF_DELAY;HOLDDOWN_INTERVAL], then the SPF computation would be triggered HOLDDOWN_INTERVAL-t1 after the IGP event. While the intuition is that it should be triggered after INITIAL_SPF_DELAY.

 >     but I think it would avoid other people making

[Bruno] ok, I would propose the following change:

OLD:  In order to satisfy the goals stated in Section 2, operators are RECOMMENDED to configure delay intervals such that INITIAL_SPF_DELAY &lt;= SHORT_SPF_DELAY and SHORT_SPF_DELAY &lt;= LONG_SPF_DELAY.
NEW: In order to satisfy the goals stated in Section 2, operators are RECOMMENDED to configure delay intervals such that INITIAL_SPF_DELAY &lt;= SHORT_SPF_DELAY, SHORT_SPF_DELAY &lt;= LONG_SPF_DELAY and HOLDDOWN_TIMER greater than INITIAL_SPF_DELAY, SHORT_SPF_DELAY, LONG_SPF_DELAY.

 > Perhaps, it should be restated in the third paragraph of section 6.
 
[Bruno] ok, added.
 
 > I believe this was brought up before and we will make this change. If not, we'll change to the RFC
 
[Bruno] ok, changed.
 
 > I would expect those implementing IGPs to know the different between an "example" and a
 
[Bruno] I agree with Acee.

In s3, I think the examples are useful for the understanding.
In s6, I think the text is clear enough:

"   This document does not propose default values for the parameters
   because these values are expected to be context dependent.
   Implementations are free to propose their own default values."

 > This is certainly out scope and I'm not sure why anyone would deduce from this draft that an
 > implementation should or shouldn't do this.
 
[Bruno] I agree with Acee. I think adding some text on this may bring confusion.
 
 > Yes. This will be added as reference.

[Bruno] done.
 
 > While implemented from the beginning, SPF Backoff is not specified by the IGP protocol
 
[Bruno] Personally, I'd argue that one does not really need to understand any specific link state protocol in order to add a delay before its SPF computation. In all cases, I'd leave this to the IESG.
 
 > Bruno and I will look at the editorial comments and let you know which ones we do and don't
 
Under way....

Thanks,
--Bruno
 
 >     provides ways to implement these settings.

_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Fri, 16 Feb 2018 15:43:00 +0000

Hi Bruno, 

On 2/16/18, 9:00 AM, "[email-masked]" <[email-masked]> wrote:

    Hi Elwyn, Acee,

    

     

    

[Acee] Looks good to me. 

Thanks,
Acee

     
    [Bruno] ok, added.

    [Bruno] ok, changed.

    

     

    [Bruno] done.

     
     
    Under way....
    
    Thanks,
    --Bruno

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Fri, 16 Feb 2018 18:39:55 +0000

Hi Elwyn, 

Also thank you much for your editorial comments. I must say I'm surprised that we didn’t catch some of these before. We will adopt most of them. One thing I'm not clear on is why you believe we should change RECOMMENDED to lowercase in the deployment recommendations. Unless convinced otherwise, we'll leave this for the IESG to decide. See inline. 

    Nits/editorial comments:
    
    Abstract:
    OLD:

This is a rather long sentence. I don't have a problem with adding "to temporarily postpone or".  However, I'd split the second clause into a new sentence and shorten it. 

       This reduces the computational load on IGP nodes when multiple network events trigger multiple SPF computations over a short time interval. 

Or simply: 
   
            This reduces the computational load on IGP nodes when multiple temporally close network events trigger multiple SPF computations. 

Ok,  

We already changed this as per Benjamin's comment. 
    
    s1, para 2:
    OLD:

I'm not sure "eventuating" is any clearer than "temporally" and the latter is more precise. 

This should be
 (e.g.,  Loop Free Alternates   (LFA) [RFC5286], FIB updates, etc.). 

    s1, para 2: the phrase

We definitely want the explicitly reference to microloops and this clause sets the correct context. Perhaps, we could shortend to "This also reduces network churn and".

    s1, para 3:
    OLD:

I think if you have read paragraph 2, the motivation is clear. However, I'd be okay with: 

     To allow for this, IGPs usually implement an SPF back-off algorithm that
     postpones or backs-off the SPF computation. 

Ok. 

Lets do a reference to section 3 instead "SPF_DELAY [Section 3]" 

Ok. As you noticed, this wasn't really a complete sentence. 

Ok.

Already handled in Benjamin's comments. 

Ok. 

Ok. 

Ok - I'd replace "running" with "execution" or "scheduling". 

Ok. 

Ok. 

Ok. 

Ok with the rewording but why not uppercase "RECOMMENDED". 

Same question on uppercase. 

Ok with wording but same question on uppercase. 

No. I can you tell you with a fair amount of certainty that vendors will not change their existing SPF delay/backoff configuration definitions (at least not vendors with large deployments). Rather, this algorithm will be added as a new configuration options and, hopefully, become the default after some period of adoption and operational experience. Configuration of this standard algorithm is reflected as  a separate container in the OSPF and IS-IS YANG models. 

Thanks,
Acee