-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify that updating a leaf-list is semantically equivalent to replace. #210
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM. I added to the OC Operator Review board. |
Thanks, this looks nice. |
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 Then within the encoded data payload is the ability to tag model elements with an 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) |
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. |
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,
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 |
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 ?
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.
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)
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/ I think a gNMI operation should stay true to it's definition but thinking off the top of my head...
|
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. |
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.