Re: [pim] John Scudder's No Objection on draft-ietf-pim-assert-packing-10: (with COMMENT)

[Toerless Eckert <[e-mail]>]

Nits from this email (and IANA assignments) fixed in just uploaded -12 version.
On to RFC editor.

Thanks!
    Toerless

On Sun, Mar 26, 2023 at 12:11:03AM +0000, John Scudder wrote:
> Hi Toerless,
> 
> The disappearing inline comments notwithstanding, I reviewed the diff, seems relatively straightforward. A last few comments top-posted here.
> 
> In response to this one:
> 
> > ### Section 3.3.1
> > 
> >   *  Implementations that introduce support for PackedAsserts after
> >      support for Asserts SHOULD support configuration that disables
> >      PackedAssert operations.
> > 
> > I get what you are trying to do here, mainly because you explained it in email.
> > Let me suggest some different wording. Maybe something like,
> > 
> >   *  A configuration option SHOULD be available to disable PackedAssert
> >      operations. An exception exists for implementations that do not
> >      have any native support for legacy Assert operations, these
> >      implementations MAY omit the configuration option.
> 
> You apparently picked both — kept the first, but added the second. Which, OK that’s good enough, but my meaning was really that the second would replace the first, i.e. to remove this one:
> 
> >   *  Implementations that introduce support for PackedAsserts after
> >      support for Asserts SHOULD support configuration that disables
> >      PackedAssert operations.
> 
> To be a little more specific, the problem I have with that one is that "introduce support for PackedAsserts after support for Asserts” is IMO not easy to understand for a reader who doesn’t already know that you mean to distinguish between newly-written implementations and legacy implementations. My goal was that the replacement text would cover your intent. But, anyway, if you want to keep both, I don’t think it’s a showstopper.
> 
> Nits:
> 
> - continueing -> continuing
> - wich -> which
> 
> And finally, what’s a “L5 subnet”? A session layer subnet?!? Should this be reverted to “L3"?
> 
> Otherwise, I’d say “good enough, ship it”, and thanks.
> 
> —John
> 
> > On Mar 26, 2023, at 8:31 AM, Toerless Eckert <[e-mail]> wrote:
> > 
> > [External Email. Be cautious of content]
> > 
> > 
> > reply to ballot position/details. Inline.
> > 
> > On Mon, Mar 20, 2023 at 02:18:24PM -0700, John Scudder via Datatracker wrote:
> >> John Scudder has entered the following ballot position for
> >> draft-ietf-pim-assert-packing-10: 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://urldefense.com/v3/__https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/__;!!NEt6yMaO-gk!AamjbC1JDxyb8E4i9kuvcO7PGXay6ylzymujjZimtkQ08aQTy60N1bsPKHgIBGYbNu-lstVk4Q$
> >> for more information about how to handle DISCUSS and COMMENT positions.
> >> 
> >> 
> >> The document, along with other ballot positions, can be found here:
> >> https://urldefense.com/v3/__https://datatracker.ietf.org/doc/draft-ietf-pim-assert-packing/__;!!NEt6yMaO-gk!AamjbC1JDxyb8E4i9kuvcO7PGXay6ylzymujjZimtkQ08aQTy60N1bsPKHgIBGYbNu8pl3U8aQ$
> >> 
> >> 
> >> 
> >> ----------------------------------------------------------------------
> >> COMMENT:
> >> ----------------------------------------------------------------------
> >> 
> >> # John Scudder, RTG AD, comments for candidate draft-ietf-pim-assert-packing-11
> >> commit fc9d64f CC @jgscudder
> >> 
> >> ## COMMENTS
> >> 
> >> (And some nits mixed in, marked as such.)
> >> 
> >> Thanks for addressing my earlier DISCUSS! I have a few residual comments, only
> >> one of which I think is problematic (my comment on §3.3.1.1). Although I hope
> >> you'll address that comment, I'm not keeping it as a DISCUSS point.
> >> 
> >> ### Section 3.3.1
> >> 
> >>   When using assert packing, the regular [RFC7761] Assert message
> >>   encoding with A=0 and P=0 is still allowed to be sent.  Routers are
> >>   free to choose which PackedAssert message format they send.
> >> 
> >> Do you mean "free to choose which Assert message format they send"?
> >> 
> >> ### Section 3.3.1
> >> 
> >>   *  Implementations SHOULD be able to indicate to the operator (such
> >>      as through a YANG model) how many Assert and PackedAssert messages
> >>      were sent/received and how many assert records where sent/
> >>      received.
> >> 
> >> Nit: where -> were
> >> 
> >> ### Section 3.3.1
> >> 
> >>   *  Implementations that introduce support for PackedAsserts after
> >>      support for Asserts SHOULD support configuration that disables
> >>      PackedAssert operations.
> >> 
> >> I get what you are trying to do here, mainly because you explained it in email.
> >> Let me suggest some different wording. Maybe something like,
> >> 
> >>   *  A configuration option SHOULD be available to disable PackedAssert
> >>      operations. An exception exists for implementations that do not
> >>      have any native support for legacy Assert operations, these
> >>      implementations MAY omit the configuration option.
> >> 
> >> ### Section 3.3.1.1
> >> 
> >> I'm still having difficulty following this section, sorry.
> >> 
> >>   Asserts/PackedAsserts in this case are scheduled for serialization
> >>   with highest priority, such that they bypass any potentially earlier
> >>   scheduled other packets.
> >> 
> >> I guess this means, maintain a queue with at least two priority levels, with
> >> the high priority being reserved for reception-triggered assert records. (I
> >> know in your email you expressed allergy to the word "queue" but that's exactly
> >> what it seems like you're describing, without using the word.)
> >> 
> >>                             When there is no such Assert/PackedAssert
> >>   message scheduled for or being serialized,
> >> 
> >> The "such" is a point of difficulty here -- its referent seems to be "earlier
> >> scheduled other packets" which are eligible to be bypassed. Or does it mean
> >> reception-triggered asserts? I think you can fix this problem by spelling it
> >> out instead of indirecting through "such".
> >> 
> >>                                              the router immediately
> >>   serializes an Assert or PackedAssert message without further assert
> >>   packing.
> >> 
> >> I'm also wondering what exactly you intend by "immediately serializes". Control
> >> plane software I'm familiar with lives up in userspace and uses the socket
> >> interface to push stuff in the direction of the network; for the messages to
> >> get written out to the wire it's not uncommon for several additional layers to
> >> be involved, different processors, a router backplane which itself may look
> >> like a network interface, etc. I think/hope you just intend that the control
> >> plane software should manage the order in which it writes things to the socket
> >> interface, but your use of phrases like "the router... serializes" makes it
> >> smell a little like you think this tuning is going to be applied all the way
> >> down to writing the packet to the wire.
> >> 
> >>   If there are one or more reception triggered Assert/PackedAssert
> >>   messages already serializing and/or scheduled to be serialized on the
> >>   outgoing interface,
> >> 
> >> ... the concern continues with the above. In cases I'm familiar with, I don't
> >> have that level of detail available to me about the interface queues when I'm
> >> sitting up in the routing daemon.
> >> 
> >>                       then the router can use the time until the last
> >>   of those messages will have finished serializing for PIM processing
> >>   of further conditions that may result in additional reception
> >>   triggered assert records as well as packing of these assert records
> >>   without introducing additional delay.
> >> 
> >> But no matter what the paradigm is I still don't get what the fragment above is
> >> telling me. The best guess I have right now (but it is not a very strong guess)
> >> is that you're saying, during the time a message is being written to the wire,
> >> we can aggregate records, according to priority order, into the next-in-line
> >> message buffer. Although of course, I don't know the details of every control
> >> plane implementation, I find it somewhat difficult to believe that this level
> >> of optimization will ever see the light of day.
> >> 
> >> More colloquially, maybe what you're saying is, "please pack the buffers as
> >> full as can be done without inducing unnecessary latency"? And putting all my
> >> guesses on this point together, maybe what this subsection is saying is,
> >> "reception-triggered asserts should be prioritized above other asserts; when
> >> constructing messages to send, please pack the buffers as full as can be done
> >> without inducing unnecessary latency"?
> >> 
> >> Also, nit: I would use a hyphen in "reception-triggered" since the two words
> >> are being used together as an adjective. Similarly in 3.3.1.2, "timer
> >> expiry-triggered".
> >> 
> >> ### Section 3.3.1.3
> >> 
> >> Nit, subsectons -> subsections
> >> Nit, be achieve -> be achieved
> >> 
> >> ### Section 3.3.1.5
> >> 
> >> Nit, througput -> throughput
> >> 
> >> ### Section 6
> >> 
> >> I guess it may be so obvious as to not need spelling out, but greenfield
> >> PackedAssert-only implementations (bullet 5 of §3.1.1) would be rendered
> >> useless if placed on a LAN with a legacy Assert-only implementation, because of
> >> the MUST in the first bullet of §3.3.1:
> >> 
> >>   *  When any PIM routers on the LAN have not signaled support for
> >>      Assert Packing, implementations MUST send only Asserts and MUST
> >>      NOT send PackedAsserts under any condition.
> >> 
> >> I wonder if this (the ability to completely silence such an implementation by
> >> advertising non-support for PackedAssert) represents an interesting enough
> >> attack that it's worth calling out in the Security Considerations? Perhaps this
> >> is already captured in some of the underlying spec SecCons, but I'm not sure.
> >> 
> >> ## Notes
> >> 
> >> This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
> >> [`ietf-comments` tool][ICT] to automatically convert this review into
> >> individual GitHub issues.
> >> 
> >> [ICMF]: https://urldefense.com/v3/__https://github.com/mnot/ietf-comments/blob/main/format.md__;!!NEt6yMaO-gk!AamjbC1JDxyb8E4i9kuvcO7PGXay6ylzymujjZimtkQ08aQTy60N1bsPKHgIBGYbNu98RlYpLQ$
> >> [ICT]: https://urldefense.com/v3/__https://github.com/mnot/ietf-comments__;!!NEt6yMaO-gk!AamjbC1JDxyb8E4i9kuvcO7PGXay6ylzymujjZimtkQ08aQTy60N1bsPKHgIBGYbNu_MNq4P4w$
> >> 
> 

-- 
---
[e-mail]