Skip to content
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

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

Open
wenovus opened this issue Nov 3, 2022 · 4 comments
Assignees

Comments

@wenovus
Copy link
Collaborator

wenovus commented Nov 3, 2022

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 wenovus self-assigned this Nov 3, 2022
@wenovus
Copy link
Collaborator Author

wenovus commented Nov 4, 2022

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
Copy link
Contributor

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
Copy link
Collaborator Author

wenovus commented Nov 9, 2022

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
Copy link
Collaborator Author

wenovus commented Dec 21, 2022

The current proposal is that this is a low-priority feature since we would prefer gNMI PROTO encoding to not use JSON. openconfig/public#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants