Re: [tsvwg] [OPS-DIR] Opsdir last call review of draft-ietf-tsvwg-datagram-plpmtud-17

[Warren Kumari <[e-mail]>]

[ - last-call@ for clutter ]

Thank you Tim for such a clear and thorough review; these are really helpful.

Authors / WG - please review and address these; I rely on the
directorate to help me evaluate documents...

W


On Tue, Mar 31, 2020 at 10:20 AM Tim Chown via Datatracker
<[e-mail]> wrote:
>
> Reviewer: Tim Chown
> Review result: Has Nits
>
> Hi,
>
> I have reviewed this document as part of the Operational directorate's ongoing
> effort to review all IETF documents being processed by the IESG.  These
> comments were written with the intent of improving the operational aspects of
> the IETF drafts. Comments that are not addressed in last call may be included
> in AD reviews during the IESG review.  Document editors and WG chairs should
> treat these comments just like any other last call comments.
>
> This document defines Datagram Packetization Layer Path MTU Discovery
> (DPLPMTUD) as a new, robust mechanism for applications or transport protocols
> to handle path MTU discovery, rather than relying on the somewhat problematic
> classic PMTUD.
>
> Overall, the document is well written with a good structure.  In my view it is
> Ready for publication, subject to some nits being considered and addressed.  I
> had held off on this review at Gorry's request until -17 was out.  My comments
> follow below; these are all generally nit-level.
>
> General comments:
>
> None.
>
> There are some ordering issues, esp. around Sec 4.6.2 where terms defined later
> in the doc are used before they are defined.  But that should be easy to fix.
>
> I did notice a few extra commas in the text; I’ve not called these out, and
> assume the RFC editor will pick these up, probably better than me!
>
> Specific comments:
>
> Sec 1.1
>
> Is there anything to be added here regarding problematic PTB delivery wrt
> protocols that insert headers into packets, rathe than encapsulating them, such
> as the case of one variant of IPv6 segment routing?
>
> Sec 1.2
>
> Para 3 - if there is no response to the probe, might the PLPMTU remain the same
> rather than being reduced?  Just steady state operation?  There is a separate
> case of a larger probe failing, as against the path MTU changing (becoming
> smaller) over time?  (This is implicitly mentioned in case 2 of section 4.3)
>
> Sec 2
>
> The first mention here of DPLPMTUD doesn’t actually say what the acronym is.
> It’s only expanded later in the document.
>
> Definition of Link - is that “layer and tunnels” or :”layer tunnels” ?
>
> Sec 4.1
>
> First mention of AEAD not expanded.
>
> Last para - maybe add “Maximum Segment Lifetime” to the definitions in section
> 2?
>
> Sec 4.3
>
> I think it would be clearer if
> “There are three ways to detect black holes:”
> read
> “There are three such indicators that allow detection of black holes:”
>
> The three methods may have different responsiveness to a change in underlying
> MTU; should that be mentioned and briefly discussed?   There’s a brief mention
> of this issue in the last para of 4.6.1.
>
> Perhaps this section could have - “Reducing PLPMTU” appended to the subject.
>
> Section 4.6.1
>
> First line, maybe “utilization and validation of”
>
> Penultimate para, maybe say “as discussed in the Security Considerations”
> section or similar.
>
> Section 4.6.2
>
> Many of the terms here are only defined later in Sec 5.1.2, e.g., BASE_PLPMTU.
> So you need to either add a foreword pointer, or change the order of things
> around.  Took me a little while to find that the terms were not yet mentioned.
>
> Sec 5
>
> Second para - is there any way to detect if both layers are running DPLPMTUD?
> Also, here you say “SHOULD NOT”, later the doc says it’s “desired” that this
> doesn’t happen.
>
> Section 5.1.2
>
> Last term, BASE_PLPMTU - why is 1200 RECOMMENDED for IPv4?  Any reference to
> point to to explain?
>
> Section 5.1.4
>
> Figure 4 is useful as a “light” version of Figure 5, but there are one or two
> inconsistencies, or omissions.   In 5.2 you say there are omissions to Fig 5
> for simplicity, maybe the same note should be here too for Fig 4? e.g., there
> is no “black hole detection” path from “Search Complete” to “Base” (which I
> *think* is different to “PLPMTU confirmation failed”. I’d also suggest saying
> “PMTU Raise Timer” rather than just “raise timer”, or even use the proper
> “PMTU_RAISE_TIMER” in the figure.
>
> I also feel the “search complete” state is a little misleading; search complete
> is how you get to this state, not so much what it is, i.e., the search is
> complete, but this is the “DPLPMTUD steady state” or maintenance state, isn’t
> it?  From it, every so often you probe to see if you can raise the MTU, or test
> that the current MTU is OK, or if you detect a PTB you MAY trigger a probe.
> Maybe consider renaming this state?
>
> Last para of Base: spec - change “If this phase” to “if the Base Phase” else it
> could be read that “this” is the Search Phase.
>
> Sec 5.2
>
> In Fig 5, should there be a path from “Search Complete” to “Base” about PLPMTU
> Confirmation Failed, as per Fig 4?
>
> Definition of SEARCHING, 2nd para, I’d suggest adding “as described in Section
> 5.3” at the end.
>
> Definition of SEARCH COMPLETE - is it really always a “successful end”?   It
> may be that PROBE_COUNT = MAX_PROBES that gets you there.  Is that successful?
> Maybe just say after the completion, and not say successful?  Depends on the
> definition of success I guess :)
>
> Sec 5.3.2
>
> Maybe details of the advice here could be a separate Informational RFC?  I’m
> curious now.
>
>
>
> _______________________________________________
> OPS-DIR mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/ops-dir



-- 
I don't think the execution is relevant when it was obviously a bad
idea in the first place.
This is like putting rabid weasels in your pants, and later expressing
regret at having chosen those particular rabid weasels and that pair
of pants.
   ---maf