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

Does gNMI Set allow for a Replace on a subtree (aka. non-root) node? #135

Closed wenovus closed 3 years ago

wenovus commented 3 years ago

Hi, I did not find in the proto definition, nor in this section (https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-specification.md#344-modes-of-update-replace-versus-update), whether this is allowed.

So, is it right that as long as a single replace operation is provided, that any other path in the entire data tree that's not specified as a replace or update in the same SetRequest will be erased?

robshakir commented 3 years ago

If you have:

aashaikh commented 3 years ago

+1 If this isn't clear in the spec, we should add it. Set.replace isn't limited to / and it's not intuitive to me that paths outside the subtree specified in the replace operation should be modified / erased.

robshakir commented 3 years ago

I suspect this could be made clearer by ensuring that we clarify that the rules in 3.4.4 apply to the paths that are specified in an Update in replace not to the tree outside it. I think we implicitly say this since the operation is scoped to the path, but we could be more explicit.

wenovus commented 3 years ago

I think the reason I'm confused is because I'm assuming that the Update messages might only ever contain leaf values (say the user wanted to use ygot's TogNMINotifications, which serializes a GoStruct into leaf notifications), so I incorrectly guessed that the only way replace did anything different than update was that everything else got erased.

Therefore, it sounds to me that in order to replace on a subtree, the val field for Update must contain the data for a container? A leaf update is indistinguishable from a leaf replace? If so, what ygot operation is currently recommended for doing a replace after populating a non-root GoStruct? I'm guessing EncodeTypedValue with JSON_IETF as the encoding type? Alternatively, it seems functionally equivalent to do a delete on the non-root node first if we choose to serialize the GoStruct into leaf notifications (assuming no conflicts with other operations exist).

robshakir commented 3 years ago

For Subscribe, Update messages (generally) only contain leaf values. For Set, the path can be any path in the tree.

You're right, if the path in the Update corresponds to a subtree, not a leaf, then the val must contain data that corresponds to a serialised-format of that container (e.g., JSON). A leaf update is indistinguishable from a replace since the difference here is in nodes that are part of the subtree that is referenced by the path but not included in the data supplied in the val, so as soon as there is a leaf, and the subtree can only be 1 node, then we must change that node's value, and hence the difference is moot.

For ygot operations for doing a replace: I don't quite follow the question here. For replacing a non-leaf node, I'd expect that you serialise using EmitJSON (or similiar) and then put this into a SetRequest with the path to the node, and then JSON value as the payload.

I think TogNMINotifications is a bit of a red-herring here -- this is specifically intended to give you serialised values that correspond to what is expected in Subscribe. IMHO, we should avoid sending Set operations that include <leaf-path, leaf> values (although they are of course possible), in favour of sending <node-path, json> since this gives us the most understandable change. One method of doing this is to always serialise the root to /, and then determine whether the user wants to update or replace during the Set transaction.

wenovus commented 3 years ago

Thanks, that answers my question.

greg-dennis commented 3 years ago

I do think the spec should be clarified with respect to this. In section 3.4.4 it says:

A replace operation is issued where e and f are set, and all other elements are omitted. In this scenario, b MUST be reverted to its default setting of True and the configuration of c MUST be deleted from the tree, and returned to its original un-configured setting.

It reads to me that a Set request consisting of replace /e and replace /f would revert /b, which was surprising to read but I'm glad to hear in this thread is not true.

charanjith-anet commented 2 years ago

+1 If this isn't clear in the spec, we should add it. Set.replace isn't limited to / and it's not intuitive to me that paths outside the subtree specified in the replace operation should be modified / erased.

The spec isn't updated yet to clarify this point. If there are three paths /a/b/c, /a/b/d/e and /a/b/d/f, will Set.replace( path: /a/b/d/f val: 'v')

  1. Replace f with the value 'v' (which is equivalent to Set.update of the same)
  2. Remove e
  3. Remove b/c

I think only 1. should be true. If the request is Set.replace( path: /a/b/d val: {'f': 'v'}), then only 1 and 2 will be true. The alternate subtrees of each of the intermediate nodes in the path shouldn't be affected i.e. b/c in the above example. If the request is Set.update( path: /a/b/d val: {'f': 'v'}), then only 1 will be true. That's the distinction between the two request types. If node 'f' doesn't exist, it should be created in either requests.

The section https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-specification.md#344-modes-of-update-replace-versus-update of the spec doesn't clarify on this. If you agree to the above, the example used in the section needs to be corrected.

hellt commented 2 years ago

I think the ambiguity in 3.4.4 comes from the fact that this paragraph

A replace operation is issued where e and f are set, and all other elements are omitted. In this scenario, b MUST be reverted to its default setting of True and the configuration of c MUST be deleted from the tree, and returned to its original un-configured setting.

doesn't say which path this Set.Replace is issued on. Therefore it sources confusion. And I also like the idea of providing more examples (like the ones by @charanjith-anet) in the spec covering the most common cases.