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

Undocumented use of gNMI Notification field atomic #180

Open
earies opened this issue Jul 16, 2024 · 7 comments
Open

Undocumented use of gNMI Notification field atomic #180

earies opened this issue Jul 16, 2024 · 7 comments

Comments

@earies
Copy link

earies commented Jul 16, 2024

A gNMI Notification message contains a boolean field named atomic per:

https://github.com/openconfig/gnmi/blob/master/proto/gnmi/gnmi.proto#L89-L91

  // This notification contains a set of paths that are always updated together
  // referenced by a globally unique prefix.
  bool atomic = 6;

However the comments above are where this definition ends and is left to interpretation. The gNMI specification does not carry any additional definition or desired usage.

Now this could be interpreted a few ways it seems:

  1. Set the value to true for any YANG model structure that carries the oc-ext:telemetry-atomic annotation essentially just reflecting a boolean signal according to the data content
  2. Set the value to true per the comment when a prefix is carried in the Notification message indicating that all Update messages as part of the Notification "conform" to the prefix concatenation

The latter is completely unnecessary as the existence of a prefix is enough to know that all subsequent Update messages packed into that Notification are part of the same prefix

The prior means that there is a direct correspondence to the data-model annotation.

Is this value necessary? How is it intended to be used or of value to a consumer?

Thx

@robshakir
Copy link
Contributor

A schema-unaware collector that receives an update with atomic = true can optimise its storage by not individually caching paths below this prefix. i.e., if /a/b/c is received with atomic = true then the collector can cache values at /a/b/c and return is value rather than storing /a/b/c/d and /a/b/c/e separately.

This is implemented in the collector in openconfig/gnmi.

@robshakir
Copy link
Contributor

The annotation that is used in OpenConfig models is telemetry-atomic.

telemetry-on-change is used to indicate that the data model expects that this value is sent as ON_CHANGE in TARGET_DEFINED mode of gNMI (or any other event-driven update model in a different RPC).

@earies
Copy link
Author

earies commented Jul 16, 2024

The annotation that is used in OpenConfig models is telemetry-atomic.

telemetry-on-change is used to indicate that the data model expects that this value is sent as ON_CHANGE in TARGET_DEFINED mode of gNMI (or any other event-driven update model in a different RPC).

sorry yes - typed one thing... meant the other

@earies
Copy link
Author

earies commented Jul 16, 2024

A schema-unaware collector that receives an update with atomic = true can optimise its storage by not individually caching paths below this prefix. i.e., if /a/b/c is received with atomic = true then the collector can cache values at /a/b/c and return is value rather than storing /a/b/c/d and /a/b/c/e separately.

This is implemented in the collector in openconfig/gnmi.

Would you then agree this goes into bucket (1) above where this is directly correlated to the annotation in the YANG model case (and up to however anyone wishes outside of those bounds)?

e.g.

/system/messages/state/message is meant to be delivered as a single Notification PDU with multiple Update messages comprising the contents of the subtree, the prefix could be any combination leading up to that container and the Notification is set to atomic=true

The collector need not know the YANG model contents but the producer is merely reflecting the intent

If so, I can draft up a PR to tighten this up in the spec + proto IDL

@robshakir
Copy link
Contributor

I'm not entirely sure I understand.

Technically, with atomic = true, I think a target is allowed (based on the current loose spec) to annotate this anywhere, and a collector can decide what to do based on this. It is likely better that this is something that is marked according to the schema, is that your point here?

@earies
Copy link
Author

earies commented Jul 16, 2024

I'm not entirely sure I understand.

Technically, with atomic = true, I think a target is allowed (based on the current loose spec) to annotate this anywhere, and a collector can decide what to do based on this. It is likely better that this is something that is marked according to the schema, is that your point here?

You got it...

For where YANG is used to describe the data content and in the case of OpenConfig modeling, we have a direct correlation to the oc-ext:telemetry-atomic annotation and when this boolean field should be flipped to true - no other expectation otherwise (e.g. this should not be flipped to true for random subtrees that do not carry this extension imo)

If native modeling in an implementation chooses to drive similar behavior, this can be purely internal (less desirable) or driven by way of a schema marking/annotation (more desirable to have a more definite contract)

If you agree - I think some mention of this behavior/expectation is warranted in the gNMI spec (which does not contain anything towards the usage of this field today) and likely comment rewording in the proto IDL

@robshakir
Copy link
Contributor

Makes sense -- happy to have this clarification. Randomly using atomic is indeed undesirable, since the aforementioned optimisation (caching at the prefix that is atomic) can also result in having no means to subtrees of that prefix.

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