openconfig / ygot

A YANG-centric Go toolkit - Go/Protobuf Code Generation; Validation; Marshaling/Unmarshaling
Apache License 2.0
286 stars 107 forks source link

`EncodeTypedValue` and `findUpdatedLeaves` don't support keyless/unkeyed lists #740

Open wenovus opened 2 years ago

wenovus commented 2 years ago

As a result TogNMINotifications and ygot.Diff don't support any populated keyless list field.

The primary issue is that both of these utilities are meant to marshal values using "PROTO" encoding defined in gnmi.proto, and so keyless lists are undefined due to the inability to have a unique path on each element in the keyless list.

One solution is simply to marshal it using JSON_IETF format since scalar encoding is infeasible anyways. I'm having trouble thinking of another reasonable solution.

@robshakir interested in your thoughts on how to solve this problem.

wenovus commented 2 years ago

After some discussions with @greg-dennis where we considered "ordered-by" and implicitly indexing keyless lists, the current idea is we'd be essentially treating keyless lists as a leaf when it comes to marshalling, unmarshalling, and telemetry, such that all updates are replaces, and that if anything within the keyless list changes, then the entirety of it must be updated.

The implication is that for modelling, we should avoid any keyless lists that contains too much data, as that would place a heavier burden on the telemetry/config systems compared to regular keyed lists since the scalar option is not available. The receiver of the JSON must be able to be handle it, which makes sense as otherwise how would you deal with keyless lists? So I don't think encoding is really an issue.

Does this make sense to you?

robshakir commented 2 years ago

We should mark these lists as atomic in the schema, such that all leaves under them are already expected to be sent together. Then, to serialise them the solution space seems to be:

  1. Use JSON such that we send a blob which includes the entire serialised list.
  2. Allow for duplicate paths within the same Notification -- in the case that a schema-aware implementation can deserialise this, then it would need to recognise that <timestamp, path, value> where the path is something in a unkeyed list would need to create an entry within the unkeyed list. Schema unaware collectors would continue to operate as they do with atomic notifications today.

The first solution here seems to be cleaner. I'd like @gcsl to comment if he has other thoughts here.

wenovus commented 2 years ago

The complexity tradeoff with 2. is that it is impossible to know for a gNMI path deletion which of the keyless values is being deleted. This means that while it's possible to indicate partial updates, it's not possible to indicate partial deletes -- and the only way is to do a full delete followed by updating the unaffected values. For option 1. this would amount to a simple update of all the unaffected values, which seems cleaner.

wenovus commented 1 year ago

The current proposal is that this is a low-priority feature since we would prefer gNMI PROTO encoding to not use JSON. https://github.com/openconfig/public/pull/750 contains a discussion (link to conclusion) as to how we should treat situations in OpenConfig YANG where unkeyed lists are the most natural representation of data.