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

[John Scudder <[e-mail]>]

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$
>>