CNF-15902: notify alarms subscriptions #431
Conversation
pixelsoccupied
commented
Jan 2, 2025
•
pixelsoccupied
commented
- After capturing all the alerts we notify any subscriber using alarm sequence, event cursor and filter to only send the latest applicable.
- Triggers are combined to managed the events during insert or update.
- Some cleanup to use existing http client.
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pixelsoccupied
force-pushed
the
subscribe-notif
branch
from
8e76100 to
7c5a8d7
Compare
|
/test all |
pixelsoccupied
force-pushed
the
subscribe-notif
branch
from
7c5a8d7 to
406afe2
Compare
pixelsoccupied
changed the title
|
/test all |
pixelsoccupied
force-pushed
the
subscribe-notif
branch
from
890726a to
4f71704
Compare
|
/test all |
pixelsoccupied
force-pushed
the
subscribe-notif
branch
from
4f71704 to
37465c8
Compare
openshift-merge-robot
added
the
needs-rebase
pixelsoccupied
force-pushed
the
subscribe-notif
branch
2 times, most recently
from
f060893 to
95c0d22
Compare
openshift-merge-robot
removed
the
needs-rebase
pixelsoccupied
force-pushed
the
subscribe-notif
branch
2 times, most recently
from
6c810d2 to
6bb49c6
Compare
pixelsoccupied
changed the title
|
@pixelsoccupied: This pull request references CNF-15902 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@pixelsoccupied: This pull request references CNF-15902 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
pixelsoccupied
force-pushed
the
subscribe-notif
branch
from
146e9ae to
7850c30
Compare
pixelsoccupied
force-pushed
the
subscribe-notif
branch
from
4b91582 to
7c09fe5
Compare
|
/test all |
pixelsoccupied
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| @@ -502,12 +514,120 @@ func (a *AlarmsServer) AmNotification(ctx context.Context, request api.AmNotific | |||
| return nil, fmt.Errorf("%s: %w", msg, err) | |||
| } | |||
|
|
|||
| //TODO: Get subscriber | |||
| // Return 200 before subscription processing to free up AM conn | |||
| a.Wg.Add(1) | |||
There was a problem hiding this comment.
IMO, it would be cleaner if this part was decoupled and moved to a separate component of the alarm server to behave more like the way it was setup for the resource and cluster servers. There is some duplication here with what the Notifier was intended to do.
This could be aligned easily if the code above materialized each of the alarm changes into a pending notifications table and sent an event over to the Notifier. If the proper hooks were implemented then the subscription filtering and cleanup of completed notifications would be handled in the same way that this is done for the resource/cluster data change notifications...including the cursor handling.
There was a problem hiding this comment.
/hold
looking into it!
| } | ||
| } | ||
|
|
||
| func CallUrl(ctx context.Context, logger *slog.Logger, client *http.Client, url string, event Notification) error { |
There was a problem hiding this comment.
As mentioned in another comment, rather than exposing this and using it directly from places other than directly within the Notifier I'd prefer if we align the alarm functionality to how the resource/cluster components work so that the Notifier handles the subscriber jobs and sending of the notifications. One reason for this is that I plan on refactoring this to allow for sending to different types of subscribers based on the callback URL (i.e., special handling for SMO notifications vs internal notifications between our servers that wouldn't require (or support) OAuth tokens).
openshift-ci
bot
added
the
do-not-merge/hold
| END IF; | ||
| END IF; | ||
|
|
||
| RETURN NEW; |
There was a problem hiding this comment.
We've discussed this briefly in the past, but I feel like it is worth mentioning again ... I'm a bit concerned that we're using the AlarmEventRecord table to do double-duty in also storing event/notification type information. If two or more changes occur to any given alarm in a short period of time then we may not be able to send off both notifications and the client would only get the last event. Clients may want to see all transitions as they may want to take action on each transition.
There was a problem hiding this comment.
(just noting it here what we discussed)
we do track all the changes back to back...partially. e.g if we have a subscriber that has a filter "CHANGE" (so notify me for everything that's NOT change), and then we have a notification that was firing, and the resolved and then firing again and now the subscriber is added, the subscriber will receive a payload with two events - one for the resolved and one for firing. The first one resolved (updated from firing to resolved) and then finally the second (firing with the new start time but same fingerprint, resulting in a new row/event)
But its true 3 things happened but the client is getting 2. But given that its currently resolved..it somewhat implied that originally it was firing?
