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

CA-394109: Reduce number of alerts #5696

Merged
merged 6 commits into from
Jul 10, 2024

Conversation

Vincent-lau
Copy link
Contributor

@Vincent-lau Vincent-lau commented Jun 17, 2024

relatively large change, best reviewed by commit

@Vincent-lau Vincent-lau force-pushed the private/shul2/less-alert branch 2 times, most recently from c0fe06a to 2faefbe Compare June 19, 2024 21:16
@Vincent-lau Vincent-lau force-pushed the private/shul2/less-alert branch from 2faefbe to 74851ba Compare June 19, 2024 21:26
@Vincent-lau Vincent-lau marked this pull request as draft June 20, 2024 09:04
@Vincent-lau Vincent-lau force-pushed the private/shul2/less-alert branch 3 times, most recently from 6f55aea to 0dbfad9 Compare June 20, 2024 19:10
@Vincent-lau Vincent-lau marked this pull request as ready for review June 20, 2024 19:12
@Vincent-lau Vincent-lau force-pushed the private/shul2/less-alert branch from 1384b5e to 4685175 Compare June 25, 2024 14:03
ocaml/idl/datamodel_cluster.ml Outdated Show resolved Hide resolved
ocaml/idl/datamodel_cluster.ml Outdated Show resolved Hide resolved
ocaml/xapi/xapi_clustering.ml Outdated Show resolved Hide resolved
(fun h -> not (List.mem h quorum_hosts))
all_cluster_hosts
let dead_hosts =
List.filter (fun h -> not (List.mem h live_hosts)) all_cluster_hosts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use sets here? This is O(n^2).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the maximum number of hosts we support is 16, so maximum complexity is 256, should be fine, what do you think?

ocaml/xapi/xapi_clustering.ml Show resolved Hide resolved
@Vincent-lau Vincent-lau force-pushed the private/shul2/less-alert branch from 4685175 to 22ffd5e Compare July 4, 2024 15:40
@BengangY
Copy link
Contributor

BengangY commented Jul 5, 2024

Checking format failed. If you use VSCode, you can configure setting.json to auto-format when saving the code like this:

    "[ocaml]": {
        "editor.formatOnSave": true
    },

@Vincent-lau Vincent-lau force-pushed the private/shul2/less-alert branch 2 times, most recently from 9bf46ef to 9143797 Compare July 8, 2024 11:09
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change replaces the direct call to alerts from normal operation to trigger a stack synchronization. This should be caught by the watcher and generate the alert once, instead of twice like before

@Vincent-lau
Copy link
Contributor Author

Vincent-lau commented Jul 9, 2024

going to run a quick xrt test to make sure these changes are good, since it has been in the review fow a while

@Vincent-lau Vincent-lau force-pushed the private/shul2/less-alert branch from 9143797 to c66e731 Compare July 10, 2024 10:26
Use the Atomic module to track whether a watcher has been created.

Signed-off-by: Vincent Liu <[email protected]>
@Vincent-lau
Copy link
Contributor Author

Added a feature flag, and tests look ok, merge this one now.

@Vincent-lau Vincent-lau force-pushed the private/shul2/less-alert branch from c66e731 to cca43a4 Compare July 10, 2024 11:08
This allows one to force sync the state of xapi db with the cluster
stack, useful for cluster API methods change the state of the cluster.

Signed-off-by: Vincent Liu <[email protected]>
Previously there were two ways an alert for a cluster host join/leave
can be raised: 1. through the cluster change watcher; 2. through the api
call. These two can generate duplicate alerts as an API call can cause
the cluster change watcher to notice the change as well.

The idea of the fix here is still to let API and watcher raise alerts
separately, but now add synchronous API calls to allow API call
(cluster-host-join, etc) to call
the cluster change update code at the right time so that the cluster
change watcher won't see the change again, hence not generating duplicate
alerts.

Signed-off-by: Vincent Liu <[email protected]>
@Vincent-lau Vincent-lau merged commit 7279089 into xapi-project:master Jul 10, 2024
15 checks passed
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

Successfully merging this pull request may close these issues.

4 participants