Re: [i2rs] Review of draft-ietf-i2rs-ephemeral-state-11

[Robert Wilton <[e-mail]>]

Hi Sue,

All fine with me.  A couple of comments inline ...


On 30/06/2016 17:15, Susan Hares wrote:
>
> Robert:
>
> Thank you for your comments.  See resolution of comments below. 
> Version -12 will be sent to the list to handle most of these comments.
>
> Sue
>
> *From:*i2rs [mailto:[e-mail]] *On Behalf Of *Robert Wilton
> *Sent:* Monday, June 27, 2016 11:30 AM
> *To:* [e-mail]
> *Subject:* [i2rs] Review of draft-ietf-i2rs-ephemeral-state-11
>
> Hi,
>
> I've reviewed draft-ietf-i2rs-ephemeral-state-11 and given a few minor 
> comments below.  Generally I think that I understand the requirements 
> stated in this document.
>
> Minors comments:
>
> 1) Ephemeral-REQ-01 (page 5):
>
>    Ephemeral-REQ-01: I2RS requires ephemeral state; i.e. state that does
>    not persist across reboots.  If state must be restored, it should be
>    done solely by replay actions from the I2RS client via the I2RS
>    agent.
>
> The architecture document indicates that the ephemeral state would (or 
> is that may) also be lost on other circumstances such as process 
> restart of the I2RS agent.  Does this need to be clarified in the 
> requirement?  E.g.
>
> Sue: In our previous discussions,  other people suggest that “reboots” 
> covered hardware and software reboot of the I2RS agent.   And that the 
> specific nature of the reboot was too-much information for the 
> requirement.
>
Rob: OK.

>    Ephemeral-REQ-01: I2RS requires ephemeral state; i.e. state that MUST
>    NOT persist across reboots of the device or I2RS Agent subsystem. If
>    state must be restored, it should be done solely by replay actions
>    from the I2RS client via the I2RS agent.
>
> 2) Hierarchy: (page 5)
> Based on the previous description, I would possibly split 
> Ephemeral-REQ-06 up into the following requirements:
>
> Old (from ephemeral-state:10):
>
>    Ephemeral-REQ-06: The ability to augment an object with appropriate
>    YANG structures that have the property of being ephemeral.  An object
>    defined as any one of the following: yang module, submodule or
>    components of submodule, or schema node.
>
> Old (from ephemeral-state-11):
>
>    Ephemeral-REQ-06: The ability to augment Yang schema nodes with
>    additional Yang Schema nodes that have the property of being
>    ephemeral.
>
> Proposed:
>
>    Ephemeral-REQ-06:
>    1. The ability to define a YANG module or submodule schema that
>    only contains data nodes with the property of being ephemeral.
>    2. The ability to augment a YANG data model with additional YANG
>    schema nodes that have the property of being ephemeral.
>
> I will accept this change and release in it version-12.
>
> 3) Ephemeral-REQ-07: (page 6):
>
>    Ephemeral-REQ-07: Ephemeral configuration state could override
>    overlapping local configuration state, or vice-versa.
>    Implementations MUST provide a mechanism to choose which takes
>    precedence.  This mechanism MUST include local configuration (policy)
>    and MAY be provided via the I2RS protocol mechanisms.
>
> I note that this requirement doesn't specify the scope of whether the 
> override mechanism should operate globally or on a per data node 
> basis.  I'm not sure whether this needs to be clarified - since the 
> text in the architecture document makes it pretty clear that a global 
> level resolution is sufficient.
>
> Sue:  Again, we were cautioned to not specify the design, but only 
> provide a result.  The global is sufficient.
>
Rob: OK

> 4) Ephemeral-REQ-08: (page 6):
> Similar to Juergen's comments, I'm concerned about the 
> writable/non-writable requirement.
>
>    Ephemeral-REQ-08: Yang MUST have a way to indicate in a data model
>    that schema nodes have the following properties: ephemeral, writable/
>    not-writable, and status/configuration.
>
> I'm somewhat adverse to writable operational state, and hence I would 
> prefer if this requirement was watered down to something like:
>
>    Ephemeral-REQ-08: In addition to config true/false, there MUST be a
>    way to indicate that YANG schema nodes represent ephemeral state.
>    It is desirable to allow for, and have to way to indicate, config
>    false YANG schema nodes that are writable operational state.
>
> Sue: You are Juergen are concerned about writeable/non-writeable.  
>  Martin is concerned about status/configuration.  The I2RS authors 
> believe we are running into the different operational state models. 
>  Each time we change this requirement we get another response.   The 
> requirement will stay for now.  I suspect the we will be working on 
> the design of the solution after we have settled on the operational 
> state models.
>
Rob:
My actual concern is that what I have proposed as a datastore solution 
cannot meet this requirement, because I think that only configuration 
should be writable, and hence the writable/non-writable property is 
implicit.


> 5) Ephemeral-Req-12, page 7:
> Presumably the requirement is that the notification must indicate the 
> node that we involved in the collision?  I.e. it isn't sufficient to 
> just signal to the client that there has been a collision with some of 
> their configuration?
>
>    Ephemeral-REQ-12: When a collision occurs as two clients are trying
>    to write the same data node, this collision is considered an error
>    and priorities were created to give a deterministic result.  When
>    there is a collision, a notification MUST BE sent to the original
>    client to give the original client a chance to deal with the issues
>    surrounding the collision.  The original client may need to fix their
>    state.
>
> Should this be made explicit?  E.g. perhaps:
>
>    Ephemeral-REQ-12: When a collision occurs as two clients are trying
>    to write the same data node, this collision is considered an error
>    and priorities were created to give a deterministic result.  When
>    there is a collision, a notification (indicating which data node the
>    collision occurred on) MUST BE sent to the original client to give
>    the original client a chance to deal with the issues surrounding
>    the collision.  The original client may need to fix their state.
>
> Sue: For some data models, it may be important to provide more than 
> just that.  How about:
>
>   When there is a collision, a notification  (which includes an 
> indication of the data node the collision occurred on)
>
> MUST BE sent to the original client to give the original client a 
> chance to deal with the issues surrounding the collision.
>
Rob: Yes, OK.


> 6) Ephemeral-REQ-14, page 7:
> I would suggest potentially rewording this to make the requirement and 
> leeway on the solution more explicit.
>
>    Ephemeral-REQ-14: If two clients have the same priority, the
>    architecture says the first one wins.  The I2RS protocol has this
>    requirement to prevent oscillations between clients.  If one uses the
>    last wins scenario, you may oscillate.  That was our opinion, but a
>    design which prevents oscillation is the key point.
>
> Proposed alternative text:
>
>    Ephemeral-REQ-14: A deterministic conflict resolution mechanism MUST
>    be provided to handle the error scenario that two clients, with
>    the same priority, update the same configuration data node.  The I2RS
>    architecture gives one way that this could be achieved, by
>    specifying that the first update wins.  Other solutions, that prevent
>    oscillation of the config data node, are also acceptable.
>
> Sue: Accepted – this will be released with version 2.
>
> Cosmetic comments:
>
> 7) Section 1. Introduction.  The requirements are no longer version 
> specific, hence perhaps update the following text from:
>
>    1.  select features from YANG, NETCONF, and RESTCONF per version of
>        the I2RS protocol (See sections 4, 5, and 6)
>    2.  propose additions to YANG, NETCONF, and RESTCONF per version of
>        the I2RS protocol for key functions (ephemeral state, protocol
>        security, publication/subscription service, traceability),
>
> to:
>
>    1.  select features from YANG, NETCONF, and RESTCONF for the initial
>        I2RS protocol version (See sections 4, 5, and 6).
>    2.  propose additions to YANG, NETCONF, and RESTCONF for the initial
>        I2RS protocol version for key functions (ephemeral state, protocol
>        security, publication/subscription service, traceability).
>
> Sue: We will probably have a subsequent version of the I2RS protocol. 
>  I would prefer to leave this as stated so this is clear.
>
> 8) Section 1. introduction.  I'm not sure that this 3rd bullet is 
> relevant, and possibly could be removed, although equally it doesn't 
> seem to do any harm:
>
>    3.  suggest protocol strawman as ideas for the NETCONF, RESTCONF, and
>        YANG changes.
>
> Sue: There is a protocol strawman that will be submitted based on 
> experience.
>
>
> 9) The document uses a mix of "Yang" and "YANG", probably should just 
> use "YANG".
>
> Fixed in version 12.
>
> 10) Section 3. Ephemeral State Requirements:
>
> >The document refers to "ephemeral configured state" here, but 
> elsewhere (e.g. 3.4) "ephemeral configuration" or "ephemeral 
> configuration state" is used.  It might be helpful for use of these 
> terms to be consistent, I would suggest "ephemeral configuration" is 
> >sufficient.
>
> Sue: Ephemeral state is defined as ephemeral configuration state and 
> operational state in the
>
Rob: It was only a minor nit, I was suggesting consistency of the term, 
perhaps change "ephemeral configuration"and "ephemeral configuration 
state"  to "ephemeral configured state".

Thanks,
Rob

>
>     3
>     <https://tools.ietf.org/html/draft-ietf-i2rs-ephemeral-state-11#section-3>. 
>     Ephemeral State Requirements
>
>     In requirements Ephemeral-REQ-01 to Ephemeral-05, Ephemeral state is
>     defined as potentially including both ephemeral configured state and
>     operational state.
>
> 11) Ephemeral-REQ-13, page 7:
>
> Minor omission in the last sentence.
>
>   Ephemeral-REQ-13: The requirement to support multi-headed control is
>    required for collisions and the priority resolution of collisions.
>    Multi-headed control is not tied to ephemeral state.  I2RS is not
>    mandating how AAA supports priority.  Mechanisms which prevent
>    collisions of two clients trying the same node of data are the focus.
>
> Proposed:
>
>   Ephemeral-REQ-13: The requirement to support multi-headed control is
>    required for collisions and the priority resolution of collisions.
>    Multi-headed control is not tied to ephemeral state.  I2RS is not
>    mandating how AAA supports priority.  Mechanisms which prevent
>    collisions of two clients trying to modify the same node of data
>    are the focus.
>
> Sue: Thank you.  Version -12 will have this change.
>
> 12) Ephemeral-REQ-15, page 7:
> I would suggest that it might be better to refer to "Ephemeral state" 
> rather than the I2RS ephemeral data-store.
>
>    Ephemeral-REQ-15: Section 7.9 of the [I-D.ietf-i2rs-architecture]
>    states the I2RS architecture does not include multi-message atomicity
>    and roll-back mechanisms.  I2RS notes multiple operations in one or
>    more messages handling can handle errors within the set of operations
>    in many ways.  No multi-message commands SHOULD cause errors to be
>    inserted into the I2RS ephemeral data-store.
>
> Proposed:
>
>    Ephemeral-REQ-15: Section 7.9 of the [I-D.ietf-i2rs-architecture]
>    states the I2RS architecture does not include multi-message atomicity
>    and roll-back mechanisms.  I2RS notes multiple operations in one or
>    more messages handling can handle errors within the set of operations
>    in many ways.  No multi-message commands SHOULD cause errors to be
>    inserted into the ephemeral state.
>
> Accepted change for -15.
>
> Thanks,
> Rob
>