Subject: Gen-art review: draft-ietf-dhc-triggered-reconfigure-05

From: Robert Sparks <[email-masked]>
Date: Fri, 26 Apr 2013 10:42:09 -0500

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-dhc-triggered-reconfigure-05
Reviewer: Robert Sparks
Review Date: April 26, 2013
IETF LC End Date: May 6, 2013
IESG Telechat date: May 16, 2013

Summary: This draft is on the right track but has open issues, described in
       the review.

Major issues:

Overall, this document is solid, but I think there is a bug in section 6.3.
I could be wrong, but If I'm right, this paragraph:

    When retransmission is required, the relay may decide to correct the
    content of RECONFIGURE-REQUEST message it issues (e.g., update the
    Client Identifier list).  This decision is local to the relay (e.g.,
    it may be based on observed events such as one or more clients were
    reconfigured on their own).

introduces a race-condition that could lead to an erroneous state. If a 
relay's first message included client A, then the retransmission 
included clients A and B, but that retransmission crosses with a success 
RECONFIGURE-REPLY to the request that only included client A, the relay 
will think it succeeded in asking A and B to be reconfigured.

Minor issues:

This sentence:

    Furthermore, means to recover state in failure events must be
    supported, but are not discussed in this document.

places a requirement on a relay (even though it's not using a 2119 
MUST). Is there some other document that defines this requirement that 
you can reference? If not, the requirement should be discussed in this 
document. Alternatively, you could change the sentence to talk about the 
consequences of not having a proprietary means for recovering state.

Nits/editorial comments:

---

From: Robert Sparks <[email-masked]>
Date: Fri, 26 Apr 2013 11:19:28 -0500

On 4/26/13 10:58 AM, [email-masked] wrote:
> Dear Robert,
>
> Thank you for the review.
> Please see inline.
>
> Cheers,
> Med
> ________________________________________
> De : [email-masked] [[email-masked]] de la part de Robert Sparks [[email-masked]]
> Date d'envoi : vendredi 26 avril 2013 17:42
> À : [email-masked]; [email-masked]; General Area Review Team; [email-masked]
> Objet : [dhcwg] Gen-art review: draft-ietf-dhc-triggered-reconfigure-05
>
> <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq><http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>
> When retransmission is required, the relay may decide to correct the content of RECONFIGURE-REQUEST message it issues (e.g., update the Client Identifier list).  This decision is local to the relay (e.g., it may be based on observed events such as one or more clients were reconfigured on their own).
>
> introduces a race-condition that could lead to an erroneous state. If a relay's first message included client A, then the retransmission included clients A and B, but that retransmission crosses with a success RECONFIGURE-REPLY to the request that only included client A, the relay will think it succeeded in asking A and B to be reconfigured.
>
> Med: This example does not apply for that text.
Really? What text constrains how you change what's in the retransmission?
>   In fact, the example should be the other way around. Perhaps, this can be made clearer if we change "(e.g., update the Client Identifier list)" to "(e.g., remove a client from the Client Identifier list)".
If I understand you correctly, you need more than just changing a 
parenthetical e.g.. I think you're saying that you are constraining the 
message changes to be such that if anything earlier in the 
retransmission sequence succeeded, the request in the retransmission 
would also have succeeded. But why do you need that much complexity? Do 
you have it already with any other request?
>
> Minor issues:
>
> This sentence:
>
> Furthermore, means to recover state in failure events must be supported, but are not discussed in this document.
> places a requirement on a relay (even though it's not using a 2119 MUST). Is there some other document that defines this requirement that you can reference?
>
> Med: I'm not aware of any; but if there is one we can cite it.
>
>   If not, the requirement should be discussed in this document. Alternatively, you could change the sentence to talk about the consequences of not having a proprietary means for recovering state.
>
> Med: Will consider that option if you think this is really needed.
>
> Nits/editorial comments:

---

From: Robert Sparks <[email-masked]>
Date: Mon, 29 Apr 2013 08:45:18 -0500

On 4/29/13 12:59 AM, [email-masked] wrote:
> Robert,
>
> Ted suggested a wording which is better than the one I proposed. I made the following changes my local copy:
>
> (1)
>
> OLD:
>     Client Identifier list).
> NEW:
>     The relay MAY remove clients from the client identifier list in
>     subsequent retransmissions, but MUST NOT add clients to the client
>     identifier list.
>
> (2)
>
> OLD:
> NEW:
>     Furthermore, means to recover state in failure events (e.g.,
>     [RFC5460]) must be supported, but are not discussed in this document.
>
> Is this better?
1 is better.

2 is an improvement, I might say "such as" instead of e.g. to even more
strongly avoid someone thinking you are _requiring_ implementation of
RFC5460.  Even then, it's still not clear whether you're trying to say

"Something that doesn't do this is does not conform to this specification"

or

"Something that doesn't do this isn't as good a tool as it can be and 
may create a condition that operators and users will not like much."

You chose not to use MUST in that sentence. Can you make it less likely 
that someone will assume you meant to?
>
> Cheers,
> Med
>
>
>> -----Message d'origine-----
>> De : Bernie Volz (volz) [mailto:[email-masked]]
>> Envoyé : samedi 27 avril 2013 06:24
>> À : Robert Sparks; BOUCADAIR Mohamed OLNC/OLN
>> Cc : [email-masked]; General Area Review Team; [email-masked]; draft-ietf-
>> [email-masked]
>> Objet : RE: [dhcwg] RE : Gen-art review: draft-ietf-dhc-triggered-
>> reconfigure-05
>>
>> Robert:
>>
>> The reason to allow this is that otherwise client A will be unnecessarily
>> reconfigured many times. (It is also possible that a client might Renew on
>> its own just as this is happening and thus it can also be removed from the
>> Reconfigure.)
>>
>> I think the text should be cleaned up to indicate that allowing removal of
>> already reconfigured clients is recommended (to prevent unnecessary
>> reconfigures) when retransmitting the Reconfigure-Request.
>>
>> Note that if clients are added, that is not a retransmission but requires a
>> "new" message (new XID).
>>
>> - Bernie
>>
>> -----Original Message-----
>> From: [email-masked] [mailto:[email-masked]] On Behalf Of
>> Robert Sparks
>> Sent: Friday, April 26, 2013 12:19 PM
>> To: [email-masked]
>> Cc: [email-masked]; General Area Review Team; [email-masked]; draft-ietf-
>> [email-masked]
>> Subject: Re: [dhcwg] RE : Gen-art review: draft-ietf-dhc-triggered-
>> reconfigure-05
>>
>>> De : [email-masked] [[email-masked]] de la part de
>>> Robert Sparks [[email-masked]] Date d'envoi : vendredi 26 avril
>>> 2013 17:42 À : [email-masked]; [email-masked]; General Area Review
>>> Team; [email-masked]
>> content of RECONFIGURE-REQUEST message it issues (e.g., update the Client
>> Identifier list).  This decision is local to the relay (e.g., it may be
>> based on observed events such as one or more clients were reconfigured on
>> their own).
>> relay's first message included client A, then the retransmission included
>> clients A and B, but that retransmission crosses with a success
>>>    In fact, the example should be the other way around. Perhaps, this can
>> be made clearer if we change "(e.g., update the Client Identifier list)" to
>> "(e.g., remove a client from the Client Identifier list)".
>> message changes to be such that if anything earlier in the retransmission
>> sequence succeeded, the request in the retransmission would also have
>> succeeded. But why do you need that much complexity? Do you have it already
>> with any other request?
>>> Minor issues:
>>>
>>> This sentence:
>>>
>>> places a requirement on a relay (even though it's not using a 2119 MUST).
>> Is there some other document that defines this requirement that you can
>> reference?
>>>    If not, the requirement should be discussed in this document.
>> Alternatively, you could change the sentence to talk about the consequences
>> of not having a proprietary means for recovering state.
>> _______________________________________________
>> dhcwg mailing list
>> [email-masked]
>> https://www.ietf.org/mailman/listinfo/dhcwg

---

From: <[email-masked]>
Date: Fri, 26 Apr 2013 17:58:38 +0200

Dear Robert,

Cheers,
Med

Major issues:

Med: This example does not apply for that text. In fact, the example should be the other way around. Perhaps, this can be made clearer if we change "(e.g., update the Client Identifier list)" to "(e.g., remove a client from the Client Identifier list)".

Minor issues:

This sentence:

Nits/editorial comments:

---

From: "Bernie Volz (volz)" <[email-masked]>
Date: Sat, 27 Apr 2013 04:23:39 +0000

Robert:

The reason to allow this is that otherwise client A will be unnecessarily reconfigured many times. (It is also possible that a client might Renew on its own just as this is happening and thus it can also be removed from the Reconfigure.)

I think the text should be cleaned up to indicate that allowing removal of already reconfigured clients is recommended (to prevent unnecessary reconfigures) when retransmitting the Reconfigure-Request.

Note that if clients are added, that is not a retransmission but requires a "new" message (new XID).

- Bernie

-----Original Message-----
From: [email-masked] [mailto:[email-masked]] On Behalf Of Robert Sparks
Sent: Friday, April 26, 2013 12:19 PM
To: [email-masked]
Cc: [email-masked]; General Area Review Team; [email-masked]; [email-masked]
Subject: Re: [dhcwg] RE : Gen-art review: draft-ietf-dhc-triggered-reconfigure-05

>   In fact, the example should be the other way around. Perhaps, this can be made clearer if we change "(e.g., update the Client Identifier list)" to "(e.g., remove a client from the Client Identifi…
If I understand you correctly, you need more than just changing a parenthetical e.g.. I think you're saying that you are constraining the message changes to be such that if anything earlier in the retransmission sequence succeeded, the request in the retransmission would also have succeeded. But why do you need that much complexity? Do you have it already with any other request?

---

From: <[email-masked]>
Date: Mon, 29 Apr 2013 07:59:50 +0200

Robert,

(1)

OLD:

NEW:

(2)

OLD:

NEW:

Is this better?

Cheers,
Med

---

From: "Bernie Volz (volz)" <[email-masked]>
Date: Mon, 29 Apr 2013 14:18:33 +0000

FYI - Stable storage or some kind of redundancy solution may be another way to recover state. So, I don't think there is a hard requirement for RFC 5460.

I don't think there is a strong reason to avoid e.g., it is "for example" so it is only one of many possible. And "such as" is basically saying the same thing.

---

But then we come to why this is even important ... The original text was:

   This setup assumes the relay has a record of the client, so that it
   has enough information to send the Reconfigure-Request message to the
   server.  How the state is recorded in the relay is out of scope.
   Furthermore, means to recover state in failure events must be
   supported, but are not discussed in this document.

I'm a bit skeptical why this draft even brings the 'recover state' up. Yes, the relay does need to record the client in order to initiate a Reconfigure-Request. But I think the sentence "Furthermore, means to recover state in failure events must be supported, but are not discussed in this document." is not needed. Sure, if the relay has no way of recovering this state, traffic to/from clients will likely not work and certainly if the relay is reconfigured, it will be unable to generate a Reconfigure-Request. But I don't really see this draft needing this requirement. Reconfigure-request could still work until the state is lost. And if the state is lost, there is a lot more than is potentially broken.

But I don't really see this as a big issue and the "must" is the lower-case variant anyway.

- Bernie

-----Original Message-----
From: Robert Sparks [mailto:[email-masked]] 
Sent: Monday, April 29, 2013 9:45 AM
To: [email-masked]
Cc: Bernie Volz (volz); [email-masked]; General Area Review Team; [email-masked]; [email-masked]
Subject: Re: [dhcwg] RE : Gen-art review: draft-ietf-dhc-triggered-reconfigure-05

1 is better.

2 is an improvement, I might say "such as" instead of e.g. to even more strongly avoid someone thinking you are _requiring_ implementation of RFC5460.  Even then, it's still not clear whether you're trying to say

or

"Something that doesn't do this isn't as good a tool as it can be and may create a condition that operators and users will not like much."

You chose not to use MUST in that sentence. Can you make it less likely that someone will assume you meant to?
>> De : Bernie Volz (volz) [mailto:[email-masked]] Envoyé : samedi 27 
>> avril 2013 06:24 À : Robert Sparks; BOUCADAIR Mohamed OLNC/OLN Cc : 
>> [email-masked]; General Area Review Team; [email-masked]; draft-ietf- 
>> The reason to allow this is that otherwise client A will be 
>> unnecessarily reconfigured many times. (It is also possible that a 
>> client might Renew on its own just as this is happening and thus it 
>> can also be removed from the
>> Reconfigure.)
>>
>> I think the text should be cleaned up to indicate that allowing 
>> removal of already reconfigured clients is recommended (to prevent 
>> unnecessary
>> Cc: [email-masked]; General Area Review Team; [email-masked]; 
>> draft-ietf- [email-masked]
>> <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq><http://wiki
>> .tools .ietf.org/area/gen/trac/wiki/GenArtfaq>.
>>> Please resolve these comments along with any other Last Call 
>>> comments you may receive.
>>>
>>> Summary: This draft is on the right track but has open issues, 
>>> described
>> in
>>>         the review.
>>>
>>> Major issues:
>>>
>>> Overall, this document is solid, but I think there is a bug in 
>>> section
>> 6.3.
>> included clients A and B, but that retransmission crosses with a 
>> success RECONFIGURE-REPLY to the request that only included client A, 
>> the relay will think it succeeded in asking A and B to be reconfigured.
>> be made clearer if we change "(e.g., update the Client Identifier 
>> list)" to "(e.g., remove a client from the Client Identifier list)".
>> parenthetical e.g.. I think you're saying that you are constraining 
>> the message changes to be such that if anything earlier in the 
>> would also have succeeded. But why do you need that much complexity? 
>> Do you have it already with any other request?
>>> Minor issues:
>>>
>>> This sentence:
>>>
>> Is there some other document that defines this requirement that you 
>> can reference?
>> Alternatively, you could change the sentence to talk about the

---

From: Ted Lemon <[email-masked]>
Date: Mon, 29 Apr 2013 16:38:03 +0000

> But I don't really see this as a big issue and the "must" is the lower-case variant anyway.

There's a big debate about whether this makes any difference.   It's generally thought to be better not to say must if you don't mean MUST.

I think the idea of just saying e.g. is the right thing, and stable storage and bulk leasequery would be two really great examples to use.