-
Notifications
You must be signed in to change notification settings - Fork 691
Mark /system/alarms list entries as atomic #1400
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
base: master
Are you sure you want to change the base?
Conversation
* (M) release/models/system/openconfig-alarms.yang
- Add telemetry-atomic to alarm list
- Increment version to 0.4.0
|
/gcbrun |
|
No major YANG version changes in commit 2837852 |
| config false; | ||
|
|
||
| list alarm { | ||
| oc-ext:telemetry-atomic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we reached a conclusion on what the definition & behaviour is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in addition, we need to ensure that when applied to a list that the context is consistent. I.e., it is individual entries of the list that are atomic, not the entire list itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definition/behavior is in a separate issue (it does need to be clarified, but maybe it is OK to go ahead and identify this as a reasonable place to use atomic in parallel?).
Good point about the list. I can't see a way for this extension to distinguish between:
- please return the entire list atomically (every list entry and their contents), vs
- please return any individual entry of the list atomically
In this case (and I think most cases) we'd want behavior #2. Maybe this is something to also raise back in the issue around generically clarifying that atomic extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - good points and yes, this PR is to at least consistently tag the obvious structs we all likely already treat as atomic (There may be more but this was an obvious one similar to that of syslog messages).
For issue: #1376 and PR: openconfig/reference#229, let me know if you agree and will add verbiage around how data is intended to be packed based on where the extension is annotated in the tree:
- If at the list, the expectation would be the entirety of a single list entry is to be returned atomically
- If at the container (whether or not there are child lists, etc..), all children are to be returned atomically
I believe this aligns w/ (2) above.
And another thing that came to mind is I believe most discussions to date of atomic bundling have been centered around ON_CHANGE (e.g. if any node changes, bundle and send) but another clarification that may be needed is intentions around SAMPLE (e.g. subscribe to single node, list or parent container) - should a Notification always pack that data atomically (and thus set atomic=true)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make the requirement that system alarm notifications (for each alarm id) must match the following form exactly:
update: {
timestamp: 1234567890
prefix: {
origin: "openconfig"
elem: {
name: "system"
}
elem: {
name: "alarms"
}
elem: {
name: "alarm"
key: {
key: "id"
value: "alarm-id"
}
}
target: "target"
}
...
### ... // all the updates for the paths under system/alarms/alarm[alarm-id]/... e.g. ###
update: {
path: {
elem: {
name: "state"
}
elem: {
name: "severity"
}
}
val: {
string_val: "openconfig-alarm-types:MAJOR"
}
}
...
atomic: true
}Presumably this is what we're requesting here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this is what we're requesting here?
Exactly
|
This is a breaking change. Unless we have very strong indication that all implementions are already behaving this way and that this is broadly what operators using |
Well this is a call for anyone to speak up that expects or has implemented anything other than atomic for this list. I'd like to understand how this could not be atomic.... this structure is very similar in nature to that of a decomposed syslog message which is treated as an atomic unit. An alarm entry is going to be an update or a delete as an atomic unit. Would a subset of data under this list ever be sent and does it give the receiver enough context? |
|
It is reasonable to implement this as atomic, but it isn't absolutely necessary. A collector could just subscribe to the key which would notify it of any newly added or deleted entries. The collector could then go explicitly read the rest of the leafs in that list entry. It doesn't need to be atomic since those other leafs won't change value after they are first created (so reading them later is also OK). |
Would really like to avoid that. Any implementations out there would then have to go do some busy work to now add a second copy of the tree? |
|
As an operator we agree that this container is not very useful with only a subset of the leaves streaming out for a given alarm event. It would be surprising if other operators do not concur with that sentiment. If other operators can chime in and say if they agree or if not, if they can explain how they find a subset useful. |
Definitely not the intent of this PR and do not see this warranted |
Could the behavior you describe above not apply for any case we have atomic tags already today? Whether or not a structure gets tagged as atomic should likely correlate with how a receiver intends to collect data consistently. If we pick on this struct, the list key is an opaque string ID that I'll venture to bet most implementations have some unique internal identifier (likely an integer) that is meaningless to the consumer by itself and unlikely to be consistent or persistent in correlation to the child content. By itself as you indicate is a signal that something was raised but we still need all child content to make sense of it. This could still be read at some later point in time, I don't think that is the question but rather should a list entry always be treated as an atomic unit. If we look at say AFT next-hops, same thing... you could subscribe to a list key and go query back based on keys but for consumer expectation purposes, we have this tagged as atomic (ref: https://github.com/openconfig/public/blob/master/release/models/aft/openconfig-aft-common.yang#L214) |
Change Scope
For reference, the current
/system/alarmssubtree:This PR does not propose any structural or type changes but rather introduces
the
oc-ext:telemetry-atomicannotation to thealarmlist structure to alignw/ current expectations and implementations.
Platform Implementations