Hi John,
thanks for your comments, please see inline (##PP):
On 13/05/2021 17:43, John Scudder via Datatracker wrote:
John Scudder has entered the following ballot position for
draft-ietf-lsr-isis-srv6-extensions-14: No Objection
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 DISCUSS and COMMENT positions.
The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-lsr-isis-srv6-extensions/
----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------
Below are some questions and comments. I'd appreciate a reply, thanks.
1. Abstract
The Segment Routing (SR) allows for a flexible definition of end-to-
You either have too many, or not enough words here. Either “segment routing“
(remove “the“), or something like “the segment routing architecture“.
called "segments". Segment routing architecture can be implemented
And here you do need “the“ in front of “segment”.
over an MPLS data plane as well as an IPv6 data plane. This document
“An” -> “the” (two places)
over an IPv6 data plane.
“An” -> “the”
This documents updates RFC 7370 by modifying an existing registry.
“Documents” -> “document”
##PP
I reworded the abstract. This text was written prior to me taking over
the document...
Also, this doesn’t seem to me like an update to RFC 7370. It’s normal for an
RFC to update an IANA registry, without saying it updates a previous RFC that
established that registry. I think the “updates” just confuses matters and
clutters things up, and should be removed.
##PP
looks like we converged towards keeping it.
2. Section 1
This documents updates [RFC7370] by modifying an existing registry
(Section 11.1.2).
“Documents” -> “document”
##PP
fixed
See also comment #1 regarding update.
3. Section 4.3
A non-zero SRH Max H.encaps MSD indicates that the headend can insert
an SRH up to the advertised value.
“Up to the advertised value” doesn’t make sense. I guess you mean “can insert
an SRH with up to the advertised number of SIDs”?
##PP
fixed
4. Section 4.4
The Maximum End D MSD Type specifies the maximum number of SIDs
present in an SRH when performing decapsulation.
That makes sense.
These includes, but
not limited to, End.DX6, End.DT4, End.DT46, End with USD, End.X with
USD as defined in [RFC8986].
That doesn’t. How can a number include all those specific things? A number’s
just an integer, right? Maybe you are providing some helpful context about the
types of SIDs that are permitted to be present?
##PP
yes, it was specifically requested by Alvaro during his AD review.
If so, I expect this is
specified elsewhere already, and this sentence is not helping; I would suggest
removing it. If it does need to stay, it needs a rewrite for grammar and
clarity. (Maybe you want something like “As specified in [RFC 8986] the
permitted SID types include, but are not limited to, <list>.”)
##PP
I changed to:
"As specified in [RFC 8986] the permitted SID types include, but are not
limited to, End.DX6, End.DT4, End.DT46, End with USD, End.X with USD."
If the advertised value is zero or no value is advertised
then the router cannot apply any behavior that results in
decapsulation and forwarding of the inner packet if the
other IPv6 header contains an SRH.
What “other” IP header? Do you mean the outer header? The inner header?
Something else?
##PP
should be "outer" instead of "other", it was a typo, I fixed it.
5. Section 5
In cases where a locator advertisement is received in both a Prefix
Reachability TLV and an SRv6 Locator TLV - (e.g. prefix, prefix-
length, MTID all being equal and Algorithm being 0 in Locator TLV),
Since “e.g.” means “for example” you’re saying the thing in the parentheses is
only one example of a locator advertisement received in both? What’s another
example? (Maybe the algo being 1 in both cases?)
But in any case the text above appears redundant with the text that immediately
follows. Can’t the text above be deleted?
##PP
yeah, that has been pointed out by other reviewers, I have left the old
text there by mistake. I have removed the above.
In case where the same prefix, with the same prefix-length, MTID, and
algorithm is received in both a Prefix Reachability TLV and an SRv6
Locator TLV, the Prefix Reachability advertisement MUST be preferred
when installing entries in the forwarding plane. This is to prevent
inconsistent forwarding entries between SRv6 capable and SRv6
incapable routers.
“In case” -> “in the case” or “in cases”
##PP
fixed.
6. Section 7.2
Supported behavior values together with parent TLVs in which they
area advertised are specified in Section 10 of this document. Please
“Area” -> “are”
##PP
fixed.
Length: variable.
Flags: 1 octet. No flags are currently defined.
When you write “length: variable“, I think you mean “length: a 1 octet field,
whose value is variable“, don’t you? Just like you write “flags: 1 octet“? I
get that the value placed into the length field is variable, but the way you’ve
written it, it says that the length of the length field is itself variable,
which would make no sense.
##PP
It refers to the value of the Length field. In the case it is static, we
specify exact value, if it is variable we say "variable". We use this in
many ISIS RFCs (e.g. RFC8667, section 2.2.2.)
This is not the only place in the document you specify a length field this way,
but I guess you should fix all of them. I’m only noting it here.
The SRv6 End SID MUST be a subnet of the associated Locator. SRv6
End SIDs which are not a subnet of the associated locator MUST be
ignored.
Is a host route (which is what a SID is) technically a “subnet”? Can something
which is not a net, be a subnet? It reads funny that way. If you change it,
possibly “MUST be covered by the associated Locator“? (Although then for
clarity you might need to define “covered“, which might not be any better.)
(Same comment applies to Section 8 which also mentions “subnet”.)
##PP
what about:
"The SRv6 End SID MUST be allocated from its associated locator's
prefix. SRv6 End SIDs that are not allocated from the associated
locator's prefix MUST be ignored."
7. Section 8.1
B-Flag: Backup flag. If set, the SID is eligible for
protection (e.g., using IPFRR) as described in [RFC8355].
Please expand IPFRR on first use. And since you say “e.g.” meaning “for
example”, do you contemplate some other kind of protection which is not IPFRR?
If not, I think you could delete the parenthesis without loss of clarity.
##PP
done
8. Section 9
SRv6 SID Structure Sub-Sub-TLV is used to advertise the as defined in
[RFC8986]. It has the following format:
##PP
fixed.
You’re missing something between “advertise the” and “as defined”.
Length: 4 octets.
Similar to my earlier comment, I think what you mean is something like “Length:
a 1 octet field, whose value is 4”.
associated with it. It's usage is outside of the scope of this
document.
“It’s” -> “its”
##PP
fixed
9. Section 11
and sub-sub-TLVs as well updating the ISIS TLV registry and defining
“As well as”
##PP
fixed
> a new registry.
Doesn’t it define several new registries?
##PP
indeed, after we added registries for flag fields
10. Section 11.2
Shouldn’t there be a new subsection for the registry creation?
##PP
added
thanks,
Peter
_______________________________________________
Lsr mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/lsr
