Re: [Dime] Last Call: <draft-ietf-dime-agent-overload-08.txt> (Diameter Agent Overload and the Peer Overload Report) to Proposed Standard

[Steve Donovan <[e-mail]>]

See my comments inline...

Steve

On 1/17/17 2:23 PM, Misha Zaytsev wrote:
> Hi All,
>
> Here are my comments/questions to an agent overload draft.
> This the first part. Later on I will complete my review and send out 
> the second portion of the comments.
>
> 1. section 1 (editorial) removed "is" before "feasible".
>
> In the base specification, the goal is to handle abatement of the
>     overload occurrence as close to the source of the Diameter traffic as
>     feasible.
>
SRD> I think both wordings are correct but I'll make the change.
> "scenaios" -> "scenarios"
>
> The Peer overload report type is
>     defined in a generic fashion so that it can also be used for other
>     Diameter overload*scenarios*.
>
SRD> Done
> 2. section 3.1.1 (editorial) replaced "were"-> "was"
>
> In both of these cases, the occurrence of overload in the single
>     agent must by handled by the client in a similar fashion as if the
>     client*was*  handling the overload of a directly connected server.
>
SRD> I agree with Janet, I kept were.
> 3. section 3.1.1 (question)
>
> An appropriate error response is sent back to the originator
>     of the request.
> Who sends "an appropriate" error response" in this case?
SRD> The client sends the error response.  I've changed the wording to:

"The client sends and appropriate error response to the originator of 
the request."
>
> 4. section 3.1.2 (editorial) changed "to"->"the"
>
> When the client has an active and a standby connection to the two
>     agents then an alternative strategy for responding to an overload
>     report from an agent is to change*the *standby connection to active and
>     route all traffic through the new active connection.
>
SRD> Changed
> 5. section 3.1.3 (editorial)
>
> An example of this type of deployment include*s*  when there are Diameter
>     agents between administrative domains.
SRD> Changed
> 6. section 3.1.3
>
> There is no section 2.2. I guess section 3.1.2 was meant here, right?
>
> Handling of overload of one or both of agents a11 or a12 in this case
>     is equivalent to that discussed in section 2.2.
>
SRD> Good catch, changed to be an xref to section 3.1.2.
> 7. section 3.2.1
>
> It is not clear which usage scenario is meant here.
>
>     It is envisioned that abatement algorithms will be defined that will
>     support the option for Diameter Endpoints to send peer reports.  For
>     instance, it is envisioned that one usage scenario for the rate
>     algorithm, [I-D.ietf-dime-doic-rate-control], which is being worked
>     on by the DIME working group as this document is being written, will
>     involve abatement being done on a hop-by-hop basis.
SRD> This is outlining at least one scenario where the peer report type 
will be used by Diameter endpoints.  When talking about agent overload, 
the peer report is only used by agents.  It is never sent by endpoints.  
Endpoints will send peer reports for things like the rate abatement 
algorithm.
> 8. section 4
> Why is throttling to be applied and not diversion (like in case of 
> redundant agents)?
> In this scenario the reacting node should first handle the throttling of the
>     overloaded host or realm.
SRD> Abatement can be either throttling or diversion.  In the case of a 
Host or Realm report, the only abatement that is possible is throttling 
as diversion just changes the path a request takes to the same 
destination.  That destination is saying send me less traffic.  The only 
way to achieve this is by throttling requests.
> "LOSS" Is it a new type defined in the scope of this draft?
> Note: The goal is to avoid traffic oscillations that might result
>        from throttling of messages for both the HOST/REALM overload
>        reports and the PEER overload reports.  This is especially a
>        concern if*both reports are of type LOSS*.
SRD> Loss is defined in the DOIC specification (RFC 7683)
> 9. section 5.1.1
> Probably it is better to describe OC_PEER_REPORT feature in section 5.1?
> Otherwise, it is used as a well-known one while it is the first place 
> where it is mentioned.
SRD> I've added a reference to the section where it is defined based on 
comments from Stephen.
> Also I think it is better to add more specific in this draft related 
> to peer report handling:
> - define Peer Report Reacting Node and Peer Report Reporting Node 
> terms explicitly and use them through the draft and especially 
> starting from section 5.1
> - add "Peer Report" prefix to all the described procedures
> Example: Capability Announcement -> Peer Report Capability Announcement
SRD> Given that the draft is dealing with handling of the Peer report, I 
don't see the need to qualify Reacting Node and Reporting Node.  If 
there is a push for it, I can see adding the terms Reacting Node and 
Reporting Node to section 2 to make it clear that they refer to Peer 
report handling in this document.
> 10. section 5.1.1/general
> "DiameterIdentity" and "Diameter identity"
> My proposal is to use one term through the spec.
SRD> There are two uses.  DiameterIdentity is referring to the RFC6733 
AVP.  The use of Diameter identity is a generic reference to the concept 
of Diameter identity.  I found two places where "Diameter Identity" was 
used instead of "DiameterIdentity".  I have corrected these occurrences.
> Under "DOIC node", an agent is meant here?
>   When an agent relays a request that includes a SourceID AVP in the
>     OC-Supported-Features AVP, a DOIC node that supports the
>     OC_PEER_REPORT feature MUST remove the received SourceID AVP and
>     replace it with a SourceID AVP containing its own Diameter identity.
SRD> Yes, an agent would be the only one that would need to worry about 
changing SourceID.  I've changed the text to the following:    When a 
Diameter Agent relays a request that includes a SourceID AVP in the 
OC-Supported-Features AVP,             if the Diameter Agent supports 
the OC_PEER_REPORT feature then it MUST remove the received SourceID 
             AVP and replace it with a SourceID AVP containing its own 
Diameter identity.
> My proposal is to use peer report reacting node here re-phrasing this 
> statement below in the following way:
>   When relaying a request that includes a SourceID AVP in the
>     OC-Supported-Features AVP, a peer report reacting node MUST remove the received SourceID AVP and
>     replace it with a SourceID AVP containing its own DiameterIdentity.
> 11. section 5.1.2
> added the missed "to"
> changed "PEER_REPORT"-> "PEER"
> Note: The transaction state is used when the DOIC node is acting
>        as a peer-report reporting node and needs*to *send OC-OLR reports of
>        type*PEER *in answer messages.  The peer overload reports
>        are only included in answer messages being sent to peers that
>        support the OC_PEER_REPORT feature.
SRD> Changed
> "Diameter ID" term is not clarified anywhere.
> Re-phrased the appropriate statement a little bit, changed "Diameter 
> ID"->"value"
> Also there are other places in the draft where "Diameter ID" term is used.
> The peer supports the OC_PEER_REPORT feature if the received request
>     contains an OC-Supported-Features AVP with the OC-Feature-Vector with
>     the OC_PEER_REPORT feature bit set and with a SourceID AVP with a
>     value that matches the DiameterIdentity of the peer from which
>     the request was received.
SRD> Agreed, changing Diameter ID to value is better.  I found one other 
place where DiameterID was used and changed it to DiameterIdentity.
> Agent is meant under "reporting node" here?
> Should not SourceID AVP not just stripped from the relayed answer, but 
> replaced with the SourceID AVP containing the DiameterIdentity of the 
> agent supporting OC_PEER_REPORT feature?
> When an agent relays an answer message, a reporting node that
>     supports the OC_PEER_REPORT feature MUST strip any SourceID AVP from
>     the OC-Supported-Features AVP.
> Hard to follow what was wanted to say here:
> Corrected the statement, but this is just my best guess.
> The OC-Peer-Algo AVP MUST indicate the overload abatement
>     algorithm that the reporting node wants the reacting nodes to use
>     *when *the reporting node send*s*  a peer overload report as a result of
>     becoming overloaded.
SRD> I think both wording are equivalent.
> Should not we add a separate if- statement for the case when the peer 
> does not support OC_PEER_REPORT feature when sending an answer message?
SRD>  The first statement above, that the agent strips the SourceID AVP 
is the only thing required if the peer does not support OC
> 12. section 5.1.1 and 5.1.2
> Probably it is more helpful to illustrate OC_PEER_REPORT feature CA 
> using sequence diagrams like in the load info conveyance draft.
SRD> This might have been helpful in the non normative section but I 
don't think it is necessary to understand the draft.
> 13. general.
> What about to use the writing for the same terms through the spec?
> Example1: "DOIC node" and "DOIC Node"
> Example2: "peer-report reporting node" and "peer report reporting node"
SRD> Consistency is a good thing.  I've modified all DOIC node 
references to DOIC Node and all "peer report reporting node"s to 
"peer-report reporting node"
> 14. section 5.2.1.2, 5.2.2, 5.2.3 and general
> "peer-type OCS" and "peer report OCS" define the same term?
> Why not to use only one? 
SRD> I don't understand the confusion.  These sections are talking about 
OCS handing for Peer Reports.  It describes a new OCS record type of 
peer-type.
> Another example: "peer report" and "peer report-type" and "report of 
> type PEER" 
SRD> The "peer report-type" should be "peer report" The last one should 
be peer.
> 15. section 5.2.3
> Probably it is better to re-phrase this statement a little bit + 
> corrected the misprints.
> If a*peer report*  reacting node receives an OC-OLR AVP of type peer and the
> SourceID matches the*DiameterIdentity *of the peer from which the*report*
> was received then the report was*generated *by the peer.
SRD> I agree with the change of ID to DiameterIdentity.  I can also go 
along with the use of the term generated.   I don't agree with the other 
changes.  The fact that it is a peer report reacting node is implicit.  
The check is if the request is from the peer and, if true, the the 
report was generate by a peer.
> Similar comment + corrected misprints for the next statement:
> If a peer report reacting node receives an OC-OLR AVP of type peer and the
> SourceID does not match the*DiameterIdentity *of the peer from which the
> *report *was received then the reacting node MUST ignore the overload
> report.
SRD> Again, the check is from where the request is received.
> Also I think it is useful to use one wording for the same term:
> "Peer Report OLR", "OC-OLR AVP", "OLR"
> Let's use a generic one "peer report"?
SRD> Peer report is used in most places.  If there are specific places 
with use of other terms makes the document unclear then please point 
them out.
> Just minor comment: "the existing..." and "a new overload condition" 
> for all occurrences if my English is correct.
SRD> I don't understand your comment.
> 16. section 5.2.3
> How may it happen that peer report reacting node receives a peer 
> report not from the peer that generated it?
> Peer reports can be sent only to peer report reacting node, right? And 
> peer reports are not relayed, right?
SRD> This is to handle cases where there are agents in the network that 
do not support the peer report feature.  These agents would relay peer 
reports, as they would any other AVP they don't understand.
> Best regards,
> /Misha
> 2017-01-09 17:35 GMT+03:00 The IESG <[e-mail] 
> <mailto:[e-mail]>>:
>
>     The IESG has received a request from the Diameter Maintenance and
>     Extensions WG (dime) to consider the following document: -
>     'Diameter Agent Overload and the Peer Overload Report'  
>     <draft-ietf-dime-agent-overload-08.txt> as Proposed Standard The
>     IESG plans to make a decision in the next few weeks, and solicits
>     final comments on this action. Please send substantive comments to
>     the [e-mail] <mailto:[e-mail]> mailing lists by
>     2017-01-23. Exceptionally, comments may be sent to [e-mail]
>     <mailto:[e-mail]> instead. In either case, please retain the
>     beginning of the Subject line to allow automated sorting. Abstract
>        This specification documents an extension to RFC 7683 (Diameter
>        Overload Indication Conveyance (DOIC)) base solution.  The
>     extension    defines the Peer overload report type.  The initial
>     use case for the    Peer report is the handling of occurrences of
>     overload of a Diameter    agent. Requirements The file can be
>     obtained via
>     https://datatracker.ietf.org/doc/draft-ietf-dime-agent-overload/
>     <https://datatracker.ietf.org/doc/draft-ietf-dime-agent-overload/>
>     IESG discussion can be tracked via
>     https://datatracker.ietf.org/doc/draft-ietf-dime-agent-overload/ballot/
>     <https://datatracker.ietf.org/doc/draft-ietf-dime-agent-overload/ballot/>
>     No IPR declarations have been submitted directly on this I-D. The
>     document contains these normative downward references. See RFC
>     3967 for additional information:    
>     draft-roach-dime-overload-ctrl: A Mechanism for Diameter Overload
>     Control (None - ) Note that some of these references may already
>     be listed in the acceptable Downref Registry.
>     _______________________________________________ DiME mailing list
>     [e-mail] <mailto:[e-mail]>
>     https://www.ietf.org/mailman/listinfo/dime
>     <https://www.ietf.org/mailman/listinfo/dime> 
>