Re: [core] Tsvart last call review of draft-ietf-core-new-block-11

Colin Perkins <csp@csperkins.org> Fri, 30 April 2021 11:02 UTC

Return-Path: <csp@csperkins.org>
X-Original-To: core@ietfa.amsl.com
Delivered-To: core@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id E05A83A1078; Fri, 30 Apr 2021 04:02:50 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.099
X-Spam-Level:
X-Spam-Status: No, score=-2.099 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_BLOCKED=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=csperkins.org
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4MVYF-Ztj4jw; Fri, 30 Apr 2021 04:02:45 -0700 (PDT)
Received: from balrog.mythic-beasts.com (balrog.mythic-beasts.com [IPv6:2a00:1098:0:82:1000:0:2:1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id AB4DB3A1075; Fri, 30 Apr 2021 04:02:45 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=csperkins.org; s=mythic-beasts-k1; h=Date:Subject:To:From; bh=LvphK9TTxb4T17YQnOFNZfgRTLUsGZfWB2KEyhn/w+s=; b=ywD8Kh8aiDTEpeLG9+3iA2Qafj SOPdYDXbJ52k+HLoc4SVbbh0Yx44YjntYuFYL3OucNP/IocypDvP3rczg21td9/zzguMmxYhXFDKu +oBLKCvErBvmtVB9/vtFln96Y85zrqNq7pKjSx7wheZpSL3nTjiY8oQioXm1HOSg8hqN4oKPgU54D 4xmktm6qCW9MPdmZjy/LDRcno36beVVFrctZw4PfPejfgaKtDpIeflsiikj6LnpnKCHcsPG7LrIwU NIKf5GyItZd66EsZjAbtsotdrBUwhHJ7dBjiDM3vS6+SMsgbZ3yhTcXrYwftXxhCmY7WPMXZRatb1 GVN07bBQ==;
Received: from [81.187.2.149] (port=42659 helo=[192.168.0.69]) by balrog.mythic-beasts.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92.3) (envelope-from <csp@csperkins.org>) id 1lcQus-0005PG-Bp; Fri, 30 Apr 2021 12:02:42 +0100
From: Colin Perkins <csp@csperkins.org>
To: mohamed.boucadair@orange.com
Cc: tsv-art@ietf.org, core@ietf.org, draft-ietf-core-new-block.all@ietf.org, last-call@ietf.org
Date: Fri, 30 Apr 2021 12:02:29 +0100
X-Mailer: MailMate Trial (1.14r5798)
Message-ID: <53FBD74C-D8CF-41B9-A67D-836760FE111B@csperkins.org>
In-Reply-To: <32001_1619679152_608A57B0_32001_95_1_787AE7BB302AE849A7480A190F8B93303537462E@OPEXCAUBMA2.corporate.adroot.infra.ftgroup>
References: <161964687448.26837.8092317722890333336@ietfa.amsl.com> <32001_1619679152_608A57B0_32001_95_1_787AE7BB302AE849A7480A190F8B93303537462E@OPEXCAUBMA2.corporate.adroot.infra.ftgroup>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
X-BlackCat-Spam-Score: 0
Archived-At: <https://mailarchive.ietf.org/arch/msg/core/Ssa_n46ovaqggLU_YSsiUzqyJ8I>
Subject: Re: [core] Tsvart last call review of draft-ietf-core-new-block-11
X-BeenThere: core@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "Constrained RESTful Environments \(CoRE\) Working Group list" <core.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/core>, <mailto:core-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/core/>
List-Post: <mailto:core@ietf.org>
List-Help: <mailto:core-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/core>, <mailto:core-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 30 Apr 2021 11:02:51 -0000

Hi,

[inline]

On 29 Apr 2021, at 7:52, mohamed.boucadair@orange.com wrote:
> Hi Colin,
>
> Thank you for the review.
>
> Please see inline.
>
> Cheers,
> Med
>
>> -----Message d'origine-----
>> De : Colin Perkins via Datatracker [mailto:noreply@ietf.org]
>> Envoyé : mercredi 28 avril 2021 23:55
>> À : tsv-art@ietf.org
>> Cc : core@ietf.org; draft-ietf-core-new-block.all@ietf.org; last-
>> call@ietf.org
>> Objet : Tsvart last call review of draft-ietf-core-new-block-11
>>
>> Reviewer: Colin Perkins
>> Review result: Ready with Issues
>>
>> This document has been reviewed as part of the transport area review
>> team's ongoing effort to review key IETF documents. These comments
>> were written primarily for the transport area directors, but are
>> copied to the document's authors and WG to allow them to address any
>> issues raised and also to the IETF discussion list for information.
>>
>> When done at the time of IETF Last Call, the authors should consider
>> this review as part of the last-call comments they receive. Please
>> always CC tsv-art@ietf.org if you reply to or forward this review.
>>
>> Thank you for preparing such a clearly written, precise,
>> specification. On the whole, this is very good. I just have some
>> minor issues to consider.
>
> [Med] Thanks.
>
>>
>> Section 4.1 says “To indicate support for Q-Block2 responses, the
>> CoAP client MUST include the Q-Block2 Option in a GET or similar
>> request (FETCH, for example), the Q-Block2 Option in a PUT or similar
>> request, or the Q-Block1 Option in a PUT or similar request so that
>> the server knows that the client supports this Q-Block 
>> functionality”
>> – It would be useful to enumerate what are “similar” requests.
>
> [Med] Argh, I thought we added an example as we did for GET. Thanks 
> for catching this. We can cite POST or PATCH.

Thanks.

>> Section 5: “Such messages must not be treated by the client as a
>> fatal error“
>> - I was surprised this is not a normative MUST NOT.
>
> [Med] We don't use the normative language here as we though this is 
> implicitly covered by the behavior in 4.3 where we indicate that the 
> client retransmits the missing blocks when such error is received.

Yes, it probably is implicitly covered.

My underlying concern here is that the draft seems a little inconsistent 
about when it uses the RFC 2119 normative language and when it 
doesn’t. There are a few places where I see “must not” and wonder 
why it’s not “MUST NOT”, and this was the most striking example.

>> Section 7.1: “For faster transmission rates, NSTART will need to be
>> increased from 1.  However, the other CON congestion control
>> parameters will need to be tuned to cover this change.  This tuning
>> is out of scope of this document as it is expected that all requests
>> and responses using Q-Block1 and Q-Block2 will be Non-confirmable
>> (Section 3.2).” - The way this is phrased is difficult to parse.
>> I can interpret it as saying that the transmission rate *does* need
>> to be faster, so implementations need to increase NSTART and tune the
>> other parameters.
>> Alternatively, I can interpret this as saying that *if* the
>> transmission needs to be faster, then NSTART and the other parameters
>> need to be tuned in some as-yet-unspecified way. The text would
>> benefit from being rephrased to clarify which meaning is intended.
>>
>> What happens when NSTART is increased beyond 1, and how the other
>> parameters are tuned, is unclear. The text would be better if it
>> either cross-referenced to the definition of how the parameters are
>> to be tuned, or explicitly stated that this is not yet supported and
>> will need to be defined in some future extension.
>
> [Med] Updated as follows:
>
> OLD:
>    Congestion control for CON requests and responses is specified in
>    Section 4.7 of [RFC7252].  For faster transmission rates, NSTART 
> will
>    need to be increased from 1.  However, the other CON congestion
>    control parameters will need to be tuned to cover this change.  
> This
>    tuning is out of scope of this document as it is expected that all
>    requests and responses using Q-Block1 and Q-Block2 will be Non-
>    confirmable (Section 3.2).
>
> NEW:
>    Congestion control for CON requests and responses is specified in
>    Section 4.7 of [RFC7252].  In order to benefit from faster
>    transmission rates, NSTART will need to be increased from 1.
>    However, the other CON congestion control parameters will need to 
> be
>    tuned to cover this change.  This tuning is not specified in this
>    document given that the applicability scope of the current
>    specification assumes that all requests and responses
>    using Q-Block1 and Q-Block2 will be Non-confirmable (Section 3.2).

Clearer, thanks.

>>
>> In Section 7.2, I’m not convinced by the argument to set 
>> MAX_PAYLOAD
>> to 10 for similar reasons to RFC 6928. The types of link layer that
>> CoAP runs over are very different to those measured to support the
>> increase in TCP’s initial window. An argument based on typical
>> properties of links and buffer space in networks used by CoAP would
>> be more convincing (I accept that using MAX_PAYLOAD of 10 is not
>> going to do any significant harm, even if it is higher than optimal).
>
> [Med] Actually we set it to 10 as the applicability scope of this spec 
> is DOTS which runs in environments similar to those of 6928. Please 
> see Section 3.2.

This would be a lot clearer if Section 3.2 were cross-referenced, and a 
reminder of the limited applicability of this specification was added to 
Section 7.2.

>> Section 7.2 also notes that “PROBING_RATE and other transmission
>> parameters are negotiated between peers”. It would be appropriate 
>> to
>> give some guidance on what are the bounds for safe values that can be
>> negotiated for these parameters.
>
> [Med] I'm afraid this is out of the scope of this spec. The intent of 
> this note is to provide an example of an application that negotiates 
> these parameters. Some of these details can be found in in 
> rfc8782#section-4.5.2 mentioned in the text you quoted.

Makes sense, although I wonder if the text in Section 7.2 might be more 
clearly written “The DOTS application uses customised defaults for 
PROBING_RATE and other transmission parameters, as discussed in Section 
4.5.2 of [RFC8782], that are negotiated between peers”?

>> Section 7.2 says:
>>
>>>   As the sending of many payloads of a single body may itself cause
>>>   congestion, it is RECOMMENDED that after transmission of every
>> set of
>>>   MAX_PAYLOADS payloads of a single body, a delay is introduced of
>>>   NON_TIMEOUT before sending the next set of payloads to manage
>>>   potential congestion issues.
>>
>> and the following paragraph has guidance for reducing MAX_PAYLOADS if
>> persistent congestion occurs “for at least a 24 hour period and it 
>> is
>> known that there are no other network issues over that period”. 
>> It’s
>> not clear how an implementation will know about other network issues,
>
> [Med] An example is a DDoS attack. Made this change: s/about other 
> network issues/about other network issues (e.g., DDoS attacks)

Makes sense, thanks.

>> and I would suggest that even if there are such issues, backoff would
>> be appropriate given persistent congestion.
>>
>> Finally, is there are mechanism for gradually recovering MAX_PAYLOADS
>> to its original value, if persistent loss ceases for some period?
>>
>
> [Med] This is covered by the configuration refresh/negotiation 
> mechanism. The peers will refresh the configuration parameters 
> following, for example, I-D.bosh-dots-quick-blocks.

Might be worth stating that in the draft?

Cheers,
Colin