Subject: [Netconf] Ben Campbell's Discuss on draft-ietf-netconf-yang-patch-12: (with DISCUSS and COMMENT)

From: "Ben Campbell" <[email-masked]>
Date: Wed, 02 Nov 2016 12:15:12 -0700

Ben Campbell has entered the following ballot position for
draft-ietf-netconf-yang-patch-12: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-netconf-yang-patch/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

Hopefully this is easy to resolve, and is probably just confusion on my
part. But I am confused by whether or not the edits in a single patch are
expected to be atomic. I assumed no, since the results can speak to
multiple edits, but section 5 mentions atomicity as a RESTCONF
requirement. Is atomicity not required by yang-patch in general, but
required when using RESTCONF?

Section 5 goes on to talk about disruption due to partial processing,
which further confuses me if RESTCONF requires atomicity.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

-2, 2nd paragraph, last sentence: is the message body mentioned in the
last sentence the same as the one described by the media type in the
previous sentence? That is, are we talking about one body part, or two?
If one, the ordering of the 2nd and 3rd sentence is a bit confusing to
me.

-2.2, tree diagram:
If edit-id is optional, how are errors identified if it is not present?

-2.6, first paragraph: "...RESTCONF server SHOULD return a
"yang-patch-status" message."

What if it doesn't? (I.e. Why not MUST?)

-2.7, 2nd paragraph: "... SHOULD return a "yang-patch-status" message."

What if it doesn't?

Editorial:
-2, first bullet: s/at within/within

-2, Accept-Patch example: The example seems misplaced, as it seems to
apply to the text two paragraphs back, not the immediately proceeding
paragraph.

---

From: Andy Bierman <[email-masked]>
Date: Wed, 2 Nov 2016 12:20:45 -0700

> https://datatracker.ietf.org/doc/draft-ietf-netconf-yang-patch/

I don't know if atomic is the right word.
If any edit fails the server MUST leave the datastore
in an unchanged state (i.e., state at the start of the PATCH request)

I will look at the text in question to see if there are clarifications
needed

Andy

---

From: Benoit Claise <[email-masked]>
Date: Thu, 3 Nov 2016 14:39:44 +0100

Ben,
>
>
> On Wed, Nov 2, 2016 at 12:15 PM, Ben Campbell <[email-masked] 
> <mailto:[email-masked]>> wrote:
>
>     Please refer to
>     https://www.ietf.org/iesg/statement/discuss-criteria.html
>     <https://www.ietf.org/iesg/statement/discuss-criteria.html>
>     <https://datatracker.ietf.org/doc/draft-ietf-netconf-yang-patch/>
>
>
>
The draft contains:

    YANG Patch is
    designed to support the RESTCONF protocol, defined in
    [I-D.ietf-netconf-restconf 
<https://tools.ietf.org/html/draft-ietf-netconf-yang-patch-12#ref-I-D.ietf-netconf-restconf>].

    It may be possible to use YANG Patch with other protocols besides
    RESTCONF.

Then the draft goes on with RESTCONF-related specifications.
In section 5, we see:

    It is important for RESTCONF server implementations to carefully
    validate all the edit request parameters in some manner.  If the
    entire YANG Patch request cannot be completed, then no configuration
    changes to the system are done.  A PATCH request MUST be applied
    atomically, as specified insection 2 of [RFC5789] <https://tools.ietf.org/html/rfc5789#section-2>.

It means that if the PATCH is ever applied to another protocol besides 
RESTCONF, and compliant to this RFC, all edits in a single MUST be atomic.

Now, I understand your confusion with this sentence:

    A RESTCONF server implementation SHOULD attempt to prevent system
    disruption due to partial processing of the YANG Patch edit list.

Could the authors clarify the sentence meaning or intent? Why a SHOULD 
versus the MUST in "A PATCH request MUST be applied atomically"?
Regards, Benoit
> _______________________________________________
> Netconf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netconf

---

From: "Ben Campbell" <[email-masked]>
Date: Thu, 03 Nov 2016 08:52:50 -0500

>> <mailto:[email-masked]>> wrote:

[...]
>    atomically, as specified insection 2 of [RFC5789] 
> <https://tools.ietf.org/html/rfc5789#section-2>.
>

I read that to say that a RESTCONF PATCH request must be atomic. Maybe I 
am misreading it, but I don't see how that says anything about the 
atomicity for use over other protocols.

>> I will look at the text in question to see if there are 
>> clarifications needed
>>
>>
>> Andy
>>
>>
>>     previous sentence? That is, are we talking about one body part, 
>> or
>>     two?

---

From: Mahesh Jethanandani <[email-masked]>
Date: Thu, 3 Nov 2016 10:57:48 -0700

> On Nov 3, 2016, at 6:52 AM, Ben Campbell <[email-masked]> wrote:
> 
>>> On Wed, Nov 2, 2016 at 12:15 PM, Ben Campbell <[email-masked] <mailto:[email-masked]>> wrote:
> 
> [...]
>>> 
>>   [I-D.ietf-netconf-restconf <https://tools.ietf.org/html/draft-ietf-netconf-yang-patch-12#ref-I-D.ietf-netconf-restconf>].
>> 
>> It means that if the PATCH is ever applied to another protocol besides RESTCONF, and compliant to this RFC, all edits in a single MUST be atomic.
> 
> I read that to say that a RESTCONF PATCH request must be atomic. Maybe I am misreading it, but I don't see how that says anything about the atomicity for use over other protocols.

Thanks Ben. We will clarify that YANG Patch edit MUST be atomic in protocols other than RESTCONF. For RESTCONF, as Andy has clarified, the server is required to leave the state in unchanged state (the state it was in before the Patch operation).

Thanks.

>> Could the authors clarify the sentence meaning or intent? Why a SHOULD versus the MUST in "A PATCH request MUST be applied atomically"?
>> Regards, Benoit
>>> 
>>> I will look at the text in question to see if there are clarifications needed
>>> 
>>> 
>>> Andy
>>> 
>>>

---

From: "Ben Campbell" <[email-masked]>
Date: Thu, 03 Nov 2016 13:01:15 -0500

On 3 Nov 2016, at 12:57, Mahesh Jethanandani wrote:

>>>>    Hopefully this is easy to resolve, and is probably just 
>>>> confusion
>>>>    on my
>>> It means that if the PATCH is ever applied to another protocol 
>>> besides RESTCONF, and compliant to this RFC, all edits in a single 
>>> MUST be atomic.
>>
>> I read that to say that a RESTCONF PATCH request must be atomic. 
>> Maybe I am misreading it, but I don't see how that says anything 
>> about the atomicity for use over other protocols.
>
> Thanks Ben. We will clarify that YANG Patch edit MUST be atomic in 
> protocols other than RESTCONF. For RESTCONF, as Andy has clarified, 
> the server is required to leave the state in unchanged state (the 
> state it was in before the Patch operation).

Thanks, I will clear my DISCUSS based on the assumption that people will 
figure out the "right thing" in the details. I suggest (but do not 
insist on) saying something about that early in the document.

Ben.

>>> Could the authors clarify the sentence meaning or intent? Why a 
>>> SHOULD versus the MUST in "A PATCH request MUST be applied 
>>> atomically"?
>>> Regards, Benoit
>>>>

---

From: Andy Bierman <[email-masked]>
Date: Thu, 3 Nov 2016 11:04:29 -0700

On Thu, Nov 3, 2016 at 10:57 AM, Mahesh Jethanandani <
[email-masked]> wrote:

> >>   [I-D.ietf-netconf-restconf <https://tools.ietf.org/html/
> draft-ietf-netconf-yang-patch-12#ref-I-D.ietf-netconf-restconf>].
> >>
> >>   atomically, as specified insection 2 of [RFC5789] <
> https://tools.ietf.org/html/rfc5789#section-2>.
> >>
> protocols other than RESTCONF. For RESTCONF, as Andy has clarified, the
> server is required to leave the state in unchanged state (the state it was
> in before the Patch operation).
>
>
I don't really want to add requirements for protocols that are out of scope
for this document.
RFC 5789 has requirements.  Any new media type for use with the PATCH method
needs to address those requirements.  Every time somebody asks me to
"clarify" RFC 5789
in the RESTCONF or YANG Patch drafts, somebody else demands (later on) that
the text be removed because  there is no need to redefine these
requirements in another RFC.

Do you want me to add text that states the redundant obvious fact that RFC
5789 requirements MUST be
met by all documents that use PATCH?

Thanks.
Andy

---

From: "Ben Campbell" <[email-masked]>
Date: Thu, 03 Nov 2016 13:14:39 -0500

On 3 Nov 2016, at 13:04, Andy Bierman wrote:

>>>> besides
>>> Maybe I
>> the

That depends. Is atomicity a requirement for yang-patch, or just a 
incidental feature because of underlying protocol? If someone later 
defines yang-patch on a different protocol, is it acceptable for it to 
be non-atomic?

(Although even if the answer is "yes", I think it would be helpful to 
mention earlier in the doc that yang-patch is atomic when used with 
restconf. Not all readers are going to know or remember all the details 
of HTTP PATCH, and that's a big detail.)

>>>> SHOULD
>>>>> clarifications
>>>>>    -2, 2nd paragraph, last sentence: is the message body mentioned 
>>>>> in
>> the
>>>>>    If one, the ordering of the 2nd and 3rd sentence is a bit 
>>>>> confusing
>> to
>>>>>    me.
>>>>>
>>>>>    -2.2, tree diagram:
>>>>>    -2, Accept-Patch example: The example seems misplaced, as it 
>>>>> seems
>> to

---

From: Mahesh Jethanandani <[email-masked]>
Date: Thu, 3 Nov 2016 11:24:10 -0700

> On Nov 3, 2016, at 11:04 AM, Andy Bierman <[email-masked]> wrote:
> 
> 
> 
>> On Thu, Nov 3, 2016 at 10:57 AM, Mahesh Jethanandani <[email-masked]> wrote:
>> 
> I don't really want to add requirements for protocols that are out of scope for this document.
> needs to address those requirements.  Every time somebody asks me to "clarify" RFC 5789
> the text be removed because  there is no need to redefine these requirements in another RFC.
> 
> Do you want me to add text that states the redundant obvious fact that RFC 5789 requirements MUST be

Andy, the confusion is coming from these lines. 

>> >>   RESTCONF

If we are talking about other protocols, and the behavior of Yang patch for those protocols is different, we need to call it out.

---

From: Andy Bierman <[email-masked]>
Date: Thu, 3 Nov 2016 11:47:35 -0700

On Thu, Nov 3, 2016 at 11:24 AM, Mahesh Jethanandani <
[email-masked]> wrote:

>> >>   [I-D.ietf-netconf-restconf <https://tools.ietf.org/html/d
>> raft-ietf-netconf-yang-patch-12#ref-I-D.ietf-netconf-restconf>].
>> >>
>> > I read that to say that a RESTCONF PATCH request must be atomic. Maybe
>> I am misreading it, but I don't see how that says anything about the
> If we are talking about other protocols, and the behavior of Yang patch
> for those protocols is different, we need to call it out.
>
>
>

OLD:

   It may be possible to use YANG Patch with other protocols besides
   RESTCONF.  This is outside the scope of this document.  It may be
   possible to use YANG Patch with datastore types other than a
   configuration datastore.  This is outside the scope of this document.

NEW:

It may be possible to use YANG Patch with other protocols besides RESTCONF.
This is outside the scope of this document.  The atomicity
requirements for such protocols is also outside the scope of this document.
It may bepossible to use YANG Patch with datastore types other than a
configuration datastore. This is outside the scope of this document.

Andy

>> confusing to

---

From: Andy Bierman <[email-masked]>
Date: Thu, 3 Nov 2016 15:56:50 -0700

On Thu, Nov 3, 2016 at 6:39 AM, Benoit Claise <[email-masked]> wrote:

>    atomically, as specified in section 2 of [RFC5789] <https://tools.ietf.org/html/rfc5789#section-2>.
>

This text is worded badly.  I really meant "incremental", not "partial".
The individual edits are conceptual.  The server is supposed to validate
the datastore on the whole set, not each one.  The intent of the sentence
was to apply all edits after validation.  e.g.

edit-list:
  1: create /foo value=42
  2: delete /foo
  3: create /foo value=43

(Although bad practice, allowed by YANG Patch)

If these were real objects, like interfaces, then the system and possibly
network could be impacted if the edits were applied 1 at a time.
Think of the edit list like a candidate datastore and the <commit>
operation.

Maybe this sentence should just be deleted, since these are implementation
details.

Andy

> Netconf mailing [email-masked]://www.ietf.org/mailman/listinfo/netconf
>
>
>

---

From: Mahesh Jethanandani <[email-masked]>
Date: Sat, 5 Nov 2016 05:41:08 +0700

> On Nov 4, 2016, at 5:56 AM, Andy Bierman <[email-masked]> wrote:
> 
> 
> 
>>    [I-D.ietf-netconf-restconf].
>> 
>>    atomically, as specified in section 2 of [RFC5789].
> Think of the edit list like a candidate datastore and the <commit> operation.
> 
> Maybe this sentence should just be deleted, since these are implementation details.

I am fine with removing the sentence not because it is implementation detail, but because you have already clarified in Section 5 that all patches must be validated before they are applied, and that they should not be applied if one of them fails.

Ben, does this deletion combined with new text around atomic behavior for other protocols address your concerns?

Mahesh Jethanandani
[email-masked]

---

From: Benoit Claise <[email-masked]>
Date: Sat, 5 Nov 2016 09:07:02 +0100

On 11/4/2016 11:41 PM, Mahesh Jethanandani wrote:
>
>
> On Nov 4, 2016, at 5:56 AM, Andy Bierman <[email-masked] 
> <mailto:[email-masked]>> wrote:
>
>>
>>
>> On Thu, Nov 3, 2016 at 6:39 AM, Benoit Claise <[email-masked] 
>> <mailto:[email-masked]>> wrote:
>>
>>     Ben,
>>>
>>>
>>>         confusion on my
>>     besides RESTCONF, and compliant to this RFC, all edits in a
>>     single MUST be atomic.
>>
Either deleted, or expanded. Your corrected sentence, along with the 
explanation, clarified it, and makes a lot of sense.
Let's hear from Ben.

Regards, B.
>
> I am fine with removing the sentence not because it is implementation 
> detail, but because you have already clarified in Section 5 that all 
> patches must be validated before they are applied, and that they 
> should not be applied if one of them fails.
>
> Ben, does this deletion combined with new text around atomic behavior 
> for other protocols address your concerns?
>
> Mahesh Jethanandani
> [email-masked] <mailto:[email-masked]>
>
>>
>>
>>
>>     Regards, Benoit
>>
>>
>>
>> Andy
>>
>>>
>>>         seems to
>>>     [email-masked] <mailto:[email-masked]>
>>>     <https://www.ietf.org/mailman/listinfo/netconf>
>>

---

From: "Ben Campbell" <[email-masked]>
Date: Mon, 07 Nov 2016 20:37:49 -0600

On 5 Nov 2016, at 3:07, Benoit Claise wrote:

[...]

> explanation, clarified it, and makes a lot of sense.

I'm okay with either. I think I personally would prefer the expanded 
explanation--but I gather the point is that this is up to RESTCONF to 
explain, and hopefully RESCONF already does that.

I would find it unfortunate if the write yang-patch semantics varied 
materially based on the protocol that carries it; but I am fine leaving 
that to other specifications that talk about how to do yang-patch over 
whatever non-RESTCONF protocol.

>> for other protocols address your concerns?

Yes.

Thanks!

Ben.

[...]

---

From: Andy Bierman <[email-masked]>
Date: Mon, 7 Nov 2016 19:00:02 -0800

> explain, and hopefully RESCONF already does that.
I will add some text to YANG Patch to clarify the issue.

> whatever non-RESTCONF protocol.
The use-case I have already implemented is CoAP
https://tools.ietf.org/html/draft-ietf-core-etch-03

The CoAP PATCH and iPATCH methods are atomic, like HTTP.

>>> patches must be validated before they are applied, and that they should not
>>> be applied if one of them fails.
>>>

Andy