Last Call Review of draft-ietf-rtgwg-mrt-frr-algorithm-08
review-ietf-rtgwg-mrt-frr-algorithm-08-secdir-lc-housley-2016-01-28-00

I reviewed this document as part of the Security Directorate's ongoing
effort to review all IETF documents being processed by the IESG.  These
comments were written primarily for the benefit of the Security Area
Directors.  Document authors, document editors, and WG chairs should
treat these comments just like any other IETF Last Call comments.

Version reviewed: draft-ietf-rtgwg-mrt-frr-algorithm-08


I have a few comments.  None of them are related to security, but since
I read the document, I am passing these comment along.


Summary: Ready (from a Security Directorate perspective)


Major Concerns: None


Minor Concern:

I think the Abstract should be reworked so that it does not include a
reference to an Internet-Draft filename.  One solution is to replace it
with an RFC number after the other document is published.  Another
solution is to add a sentence that explains MRT-FRR.  Perhaps something
like:

   MRT-FRR provides link-protection and node-protection with 100%
   coverage in any network topology that is still connected after
   the failure.


Nits:

The Introduction says that the MRT Lowpoint algorithm is required for
implementation with the Default MRT profile.  It should say this once
in the first paragraph; it does not need repeating in several places.

In the Introduction, please talk about Figure 1 before talking about
Figure 2.  I notice that both of these figures are already part of
draft-ietf-rtgwg-mrt-frr-architecture, so it may be possible to
summarize the point being made and then point to the architecture
document for more details.

The phrases "this draft" and "that draft" are used.  I think it would
be better to select words that anticipate publication as an RFC.

Some of the figures contain algorithm pseudocode, but sometimes there
are comments for explanation and sometimes the comments appear to be
part of the code.  For example, see Figure 12:

            Get the current node, s.
            Compute an ear(either through lowpoint inheritance
            or by following dfs parents) from s to a ready node e.
            (Thus, s is not e, if there is such ear.)
            if s is e
               for each node x in the ear that is not s
                   x.localroot = s
            else
               for each node x in the ear that is not s or e
                   x.localroot = e.localroot

I think it would more clear is it looked something like:

            Get the current node, s.
            Compute an ear from s to a ready node e.
            // Compute either through lowpoint inheritance or
            // by following dfs parents.
            // Thus, s is not e, if there is such ear.
            if s is e
               for each node x in the ear that is not s
                   x.localroot = s
            else
               for each node x in the ear that is not s or e
                   x.localroot = e.localroot

It would be event better if this pseudocode used the Construct_Ear
function.