PIM Assert Message Packing
draft-ietf-pim-assert-packing-12

# Internet AD comments for draft-ietf-pim-assert-packing-10
CC @ekline

## Nits

### S2

* "with more than 2 PIM routers"

  Should this actually read "with 2 or more PIM routers"?

# 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://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

        M: The number of Assert Records in the message. Derived from the length of the packet carrying the message.

I find it a bit odd that this section describes packet fields, and then lists a "field"
that is not a real field but a derivation. Can this not be written without the "M:" classifier
and outside of the list describing the packet fields? This occurs a number of times in the document.

Like Robert Sparks, I am interested his question from the secdir review:

   There's discussion of not using packing unless all PIM
   routers in the same subnet have announced the Assert Packing Hello Option. What
   happens in a running environment that is busy using packing when a new PIM
   router is added? If traffic from that PIM router is seen that is not yet the
   needed Hello Option, should all senders stop packing until the needed Hello
   Option arrives, and maybe resend recently packed asserts as unpacked?

Thank you to Robert Sparks for the SECDIR review.

Thanks for working on this specification. Thanks Tommy Pauly for the TSVART review, I also agree that congestion control for PIM would be a good exercise specially when we are expecting negative impact of loss due to buffer overflow and repetitive burst of traffic.

I agree with John's discuss point that it is a bit confusing to see the mix of normative and non-normative text. for example - it says.

    Routers may support a sufficient
   amount of packing to minimize the negative impacts of loss of
   PackedAssert packets without loss of performance of minimizing IP
   multicast packet duplication.

According to the description preceding that line, this seems like at the level where we should normative language but it was not.

# Éric Vyncke, INT AD, comments for draft-ietf-pim-assert-packing-10
CC @evyncke

Thank you for the work put into this document. 

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and one nit.

Special thanks to Stig Venaas for the shepherd's detailed write-up including the WG consensus **but** it lacks the justification of the intended status. 

I hope that this review helps to improve the document,

Regards,

-éric

## COMMENTS

### Section 1

`there is typically more than one upstream router` unsure whether it is really the common case... Suggest to s/there is typically/Sometimes there is/

### Section 2

Should there be references for assertions such as `something not possible equally with an L3 subnet based ring`?

### Section 4.2

*Strongly* suggest to add a reference to section 4.9.1 of RFC 7761 for the `encoded-* format` (as my heart missed a beat when seeing a 32-bit field).

## NITS

### Units

s/100msec/100 msec/ and other similar issues.

## 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://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

I found the last paragraph of Section 3.3.1 extremely difficult to parse. Perhaps reorganizing it into a series of normative statements?

Thanks for this document.

The comments that I would have raised on the section 3.3.1 text being a bit confusing have already been raised by John, so I've not repeated them here.

Regards,
Rob