Skip to content

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.

Sign up for GitHub

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

Closed
earies wants to merge 1 commit into from
Closed

Mark /system/alarms list entries as atomic #1400

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion release/models/system/openconfig-alarms.yang
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,13 @@ module openconfig-alarms {
Section 4.c of the IETF Trust's Legal Provisions Relating
to IETF Documents (http://trustee.ietf.org/license-info).";

oc-ext:openconfig-version "0.3.2";
oc-ext:openconfig-version "0.4.0";

revision "2025-11-18" {
description
"Mark alarm list entries as telemetry-atomic";
reference "0.4.0";
}

revision "2019-07-09" {
description
Expand Down Expand Up @@ -176,6 +182,7 @@ module openconfig-alarms {
config false;

list alarm {
oc-ext:telemetry-atomic;
Copy link
Copy Markdown
Contributor

@rgwilton rgwilton Nov 19, 2025

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?

Copy link
Copy Markdown
Contributor

@rgwilton rgwilton Nov 19, 2025

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.

Copy link
Copy Markdown

@jsterne jsterne Nov 19, 2025

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:

  1. please return the entire list atomically (every list entry and their contents), vs
  2. 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.

Copy link
Copy Markdown
Contributor Author

@earies earies Nov 19, 2025

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)

Copy link
Copy Markdown
Contributor

@ElodinLaarz ElodinLaarz Nov 21, 2025

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?

Copy link
Copy Markdown
Contributor Author

@earies earies Nov 21, 2025

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

key "id";
description
"List of alarms, keyed by a unique id";
Expand Down
Loading