openconfig / reference

This repository contains reference implementations, specifications and tooling related to OpenConfig-based network management.
Apache License 2.0
155 stars 88 forks source link

Clarify that updating a leaf-list is semantically equivalent to replace. #210

Open wenovus opened 1 month ago

wenovus commented 1 month ago

This makes sense because in general the order matters, and append is just one way of inserting elements to the list. There is also no way to delete elements if this weren't the semantic.

ygot currently follows this.

dplore commented 1 month ago

LGTM. I added to the OC Operator Review board.

wenovus commented 1 month ago

LGTM. I added to the OC Operator Review board.

Thanks, this looks nice.

earies commented 1 month ago

While I understand the problem stmts that leaf-lists impose with the current gNMI operations, I can't help but feel we are breaching the boundaries of the RPC operation intent by sub-classifying special behavior as such.

If we draw the analogy to the NETCONF <edit-config> RPC, the RPC itself carries an optional default-operation (merge, replace, none) with merge being the default.

Then within the encoded data payload is the ability to tag model elements with an operation attribute (merge, replace, create, delete, remove). This latter portion currently has no defined equivalent on behavior should this be encoded into payloads on gNMI operations today.

This would be like the prior (RPC level operation) with the intent to "update" all but an exception on a certain YANG type (leaf-list) in which it is a desired "replace".

Ordering doesn't matter on all leaf-lists but it very well may on others... this would negate any ability to append in any circumstance so all operations that contain leaf-list payloads must include the full replacement (prior knowledge) content.

Speaking for current JUNOS behavior, this is not how an "update" operation works as it is a global update without exceptions per type (e.g. in this case if you are adding an element to a leaf-list, it is appended)

earies commented 1 month ago

Some additional consideration and commentary:

This is an example of a seemingly subtle set of wording changes in a spec w/ no modification to the proto IDL. The spec is flagged as a semantic patch version update 0.10.0 -> 0.10.1 however it would be a backwards incompatible underlying behavior change that needs to be handled in reality (either brute force change or a vendor proprietary knob). For the wider community, there is generally the question "do you support gNMI?" which can be (but hardly ever) followed up w/ "a" version (which generally refers to the proto IDL version rather - e.g. 0.10.0). To date, I have not heard of any matrix relationship between the spec version and the protocol version being understood esp. with these types of behavioral changes and this cannot be conveyed programmatically today.

The API has not changed but the behavior has and thus how a client must interact and populate payload content w/ the server changes.

But back to the behavior change in general in that this would cross the boundary between the protocol and the data content (which should have a firm demarcation). gNMI as previously mentioned in other PR/issues is "schema agnostic" and there are various cases already where non-YANG modeled data could be represented (or overloaded) into various fields to leverage using the same protocol (either defined or undefined behavior). This operation now means the protocol must be "schema aware" and to take it further, to a specific YANG language construct.

wenovus commented 1 month ago

bumped to v0.11.0 -- agreed that since this can cause breaking change in existing implementations due to the ambiguity clarification.

In response to the argument that we're depriving the ability to append -- I agree that if we wanted the capability to do the various NETCONF operations on a list of items, that we might be going in the wrong direction. However, without that intention, either append or replace has its own argument. I'm proposing replace because,

  1. ygot tooling already implements this and has been in use for many years,
  2. The gNMI reference implementation cache has replace semantics for all leaf types (i.e. TypedValue scalars), and also has been in use for many years.
  3. If an insertion or deletion is required, then a replace is required anyways, even though it's intuitively also an "update".

So on the whole I think using replace semantics is the better trade-off.

Lastly, I don't think this clarification made gNMI schema-aware -- this is solely specifying the update behaviour for leaflist_val.

earies commented 1 month ago

bumped to v0.11.0 -- agreed that since this can cause breaking change in existing implementations due to the ambiguity clarification.

The more I think about the cases where there is a spec in addition to an API definition, the more I can't help but feel these need to be one in the same in lock step. The IETF analogy here is that data-models are part of the RFC document and have a tight 1:1 relationship on versioning.

Otherwise, there is no signal today to a client/consumer of this matrix. A capabilities() RPC will still yield the same version of the proto IDL, reflection will reflect the same contents but the behavior change is not signaled and increasingly harder to track.

What this would then entail is that the proto IDL undergo a revision bump w/ no content changes. The changelog in the spec can reference these changes and be referenced from the IDL (unless we want to build lengthy comments over time)

Curious on others thoughts here as well.... @dplore ?

In response to the argument that we're depriving the ability to append -- I agree that if we wanted the capability to do the various NETCONF operations on a list of items, that we might be going in the wrong direction. However, without that intention, either append or replace has its own argument. I'm proposing replace because,

1. ygot tooling already implements this and has been in use for many years,
2. The gNMI reference implementation cache [has replace semantics](https://github.com/openconfig/gnmi/blob/900c036ab0c17a4aa6547f173dfb33d367c6f57d/ctree/tree.go#L157) for all leaf types (i.e. TypedValue scalars), and also has been in use for many years.

I wasn't suggesting to copy NETCONF operations verbatim but merely drawing an example on similarities. While ygot has had this for various years, the underlying management daemon infrastructure on the elements responsible for these primitive operations have sometimes had this for nearly 2 decades. These load-type operations are foundational outside of the NBI so a change in behavior is a change in behavior for all (which is not feasible) thus the solution would need to be a new special operation/toggle to any implementation to preserve compatibility.

3. If an insertion or deletion is required, then a replace is required anyways, even though it's intuitively also an "update".

So on the whole I think using replace semantics is the better trade-off.

The only place where I'm hung up on this is the operation is no longer a true "update" - it is a hybrid operation w/ special handling (update-replace)

Lastly, I don't think this clarification made gNMI schema-aware -- this is solely specifying the update behaviour for leaflist_val.

It did in the sense that a protocol operation in gNMI defines specific implementation behavior dependent on YANG schema data-types. Once the gNMI RPC is de-serialized and processed from a management daemon pov, the implementation needs to be aware that a new operation is coming in as an "update" but any content that falls into a special type should replace vs. update. The management daemon operation needs special handling for this dependent on the schema.

This also means that mixed schema w/ cli origin is undefined for the equivalent. That data for each implementation could be modeled in such a way that the equivalent of a leaf-list either in YANG or another schema definition exists. Should the set() with mixed schema be sent as an "update", this origin does not assume YANG modeled data hence no leaf-list replace operation handling in that case (the implementation could choose to do either w/o definition).

I think a gNMI operation should stay true to it's definition but thinking off the top of my head...

dplore commented 1 week ago

Regarding the spec vs. API definition, I think we can create a PR in the gnmi repository to include bumping the version for the gnmi proto and merge these two together. @wenovus

wenovus commented 1 week ago

Regarding the spec vs. API definition, I think we can create a PR in the gnmi repository to include bumping the version for the gnmi proto and merge these two together. @wenovus

Yeah once this PR is merged we can bump the gNMI version so that there is a matching proto version.