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

[Misha Zaytsev <[e-mail]>]

Hi All,

Just more minor comments from me to have all in place :)

28. section 6.4.

6.4.  Attribute Value Pair *F*lag *R*ules

29. section 7.1

7.1.  AVP *C*odes


30. section 7.2

7.2.  New *R*egistries


/Misha

2017-01-19 16:18 GMT+03:00 Misha Zaytsev <[e-mail]>:

> Hi All,
>
> Just several small comment from my side:
>
> 24. section 6.1.1. "OC-Feature-Vector" -> "OC-Feature-Vector AVP" in the
> header.
>
> 25. section 6.1.2 "OC-Peer-Algo" -> "OC-Peer-Algo AVP" in the header
>
> 26. section 6.2 corrected AVP names
>
> This extension makes no changes to the *OC-Sequence-Number* or
> *OC-Validity-Duration* AVPs in the OC-OLR AVP.
>
> 27. section 6.3
>
> Probably, it is the matter of preference, but I would still propose to
> rename SourceID to Source-Identity.
>
>
> Thanks in advance!
>
> /Misha
>
>
> 2017-01-18 21:40 GMT+03:00 Misha Zaytsev <[e-mail]>:
>
>> Hi All,
>>
>> As promised here is the second part of the comments (numbering is kept):
>>
>> 17. section 5.2.3, 5.2.4 (and further if applicable)
>>
>> Why not to use "active OCS" instead of "existing OCS" or "existing
>> overload conditions"?
>> Just to be inline with the RFC7683 this draft is extending...
>>
>> 18. section 5.2.4
>>
>> What is meant here? Especially "other overload reporting node behaviors".
>> For me it says about all and about nothing at the same time. Please
>> clarify
>>
>> The reporting agent must follow all other overload reporting node
>>    behaviors outlined in the DOIC specification.
>>
>>
>> 19. section 5.2.5
>>
>> Seems to be a little bit grammatically incorrect, "on" -> "to"
>>
>> If the request matches an active OCS then the reacting node MUST
>>    apply abatement treatment *to *the request.
>>
>> Maybe "error response" sounds better than just an "error" in this case:
>>
>> ... agent MUST send an appropriate error response as defined in [RFC7683].
>>
>> Seems to be wrongly phrased:
>>
>>  ... then *abatement *associated with the overload *abatement *MUST be ended in a
>>    controlled fashion.
>>
>>
>> 20. section 6
>>
>> I do not know why but I do not like the following wording:
>>
>> ... used as part of the CER/CEA base Diameter capabilities exchange.
>>
>>
>> Probably this version is better:
>>
>> ... used as part of the Diameter Capabilities Exchange procedure defined in [RFC6733].
>>
>>
>> 21. section 6.1.1
>>
>> The peer report feature defines a new feature bit *that (?)*is added for the OC-Feature-Vector AVP.
>>
>>
>> 22. section 6.2, similar to comment#16
>>
>> In the section 5.1.2 it says the following:
>>
>> When receiving a request a DOIC node that supports the OC_PEER_REPORT
>>    feature MUST update transaction state with *an indication of whether
>>    or not the peer from which the request was received supports the
>>    OC_PEER_REPORT feature.*
>>
>>       Note: The transaction state is used when the DOIC node is acting
>>       as a peer-report reporting node and needs send OC-OLR reports of
>>       type PEER_REPORT in answer messages.  The peer overload reports
>>       are only included in answer messages being sent to peers that
>>       support the OC_PEER_REPORT feature.
>>
>> So, my gut feeling is that it means that peer report can't be sent thru a
>> non-supporting agent???
>> If I'm wrong, please clarify that.
>>
>> Also, just several updates in wording + corrected misprints.
>>
>> The overload report MUST also include the Diameter identity of the
>>    agent that generated the report.  This is necessary to handle the
>>    case where there is a non*-*supporting agent between the reporting node
>>    and the reacting node.  Without the indication of the agent that
>>    generated the overload *report*, the reacting node could erroneously
>>    assume that the report applied to the non-supporting node.  This
>>    could, in turn, result in unnecessary traffic being either
>>    *diverged* or throttled.
>>
>>
>> 23. section 6.3 "SourceID" -> "SourceID AVP" in the section header.
>>
>> Seems to be all from my side, but probably I have still missed something.
>> Anyway, I'm ready to re-check the new version of the draft when available.
>>
>> Best regards,
>>
>> /Misha
>>
>> 2017-01-17 23:23 GMT+03:00 Misha Zaytsev <[e-mail]>:
>>
>>> 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.
>>>
>>>
>>> "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*.
>>>
>>>
>>> 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.
>>>
>>>
>>> 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?
>>>
>>> 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.
>>>
>>>
>>> 5. section 3.1.3 (editorial)
>>>
>>> An example of this type of deployment include*s* when there are Diameter
>>>    agents between administrative domains.
>>>
>>> 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.
>>>
>>>
>>> 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.
>>>
>>>
>>> 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.
>>>
>>> "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*.
>>>
>>>
>>> 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.
>>>
>>> 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
>>>
>>> 10. section 5.1.1/general
>>>
>>> "DiameterIdentity" and "Diameter identity"
>>> My proposal is to use one term through the spec.
>>>
>>> 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.
>>>
>>>
>>> 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.
>>>
>>>
>>> "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.
>>>
>>>
>>> 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.
>>>
>>>
>>> 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?
>>>
>>> 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.
>>>
>>> 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"
>>>
>>> 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?
>>>
>>> Another example: "peer report" and "peer report-type" and "report of
>>> type 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.
>>>
>>>
>>> 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.
>>>
>>>
>>> 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"?
>>>
>>> Just minor comment: "the existing..." and "a new overload condition" for
>>> all occurrences if my English is correct.
>>>
>>> 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?
>>>
>>> Best regards,
>>>
>>> /Misha
>>>
>>>
>>> 2017-01-09 17:35 GMT+03:00 The IESG <[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] mailing lists by 2017-01-23. Exceptionally, comments may
>>>> be
>>>> sent to [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/
>>>>
>>>> IESG discussion can be tracked via
>>>> 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]
>>>> https://www.ietf.org/mailman/listinfo/dime
>>>>
>>>
>>>
>>
>