-
Notifications
You must be signed in to change notification settings - Fork 1.2k
api,server: allow configuring repetitive alerts #11325
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: main
Are you sure you want to change the base?
Conversation
Fixes apache#6613 Introduces support for configuring additional alert types that can be published repeatedly, beyond the default set. Operators can now use the dynamic configuration `alert.allowed.repetitive.types` to specify a comma-separated list of alert type names that should be allowed for repetitive publication. Signed-off-by: Abhishek Kumar <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11325 +/- ##
============================================
+ Coverage 17.35% 17.50% +0.15%
- Complexity 15189 15481 +292
============================================
Files 5883 5885 +2
Lines 524514 535880 +11366
Branches 64007 68731 +4724
============================================
+ Hits 91013 93791 +2778
- Misses 423216 431552 +8336
- Partials 10285 10537 +252
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR introduces configurable repetitive alerts functionality, allowing operators to specify additional alert types that can be published repeatedly beyond the default set. The enhancement replaces the hardcoded list of repetitive alert types with a dynamic configuration system.
Key changes:
- Adds a new configuration parameter
alert.allowed.repetitive.types
for specifying comma-separated alert type names - Implements dynamic configuration updates through message bus listening
- Updates the alert type system to include repetition capability information in API responses
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
server/src/main/java/com/cloud/alert/AlertManagerImpl.java | Replaces hardcoded repetitive alert list with dynamic configuration and message bus listener |
engine/components-api/src/main/java/com/cloud/alert/AlertManager.java | Adds new configuration key for allowed repetitive alert types |
api/src/main/java/org/apache/cloudstack/api/response/AlertTypeResponse.java | Adds repetition allowed field to alert type API response |
api/src/main/java/org/apache/cloudstack/api/command/admin/resource/ListAlertTypesCmd.java | Updates command to include repetition information in responses |
api/src/main/java/org/apache/cloudstack/api/ApiConstants.java | Adds API constant for repetition allowed field |
api/src/main/java/org/apache/cloudstack/alert/AlertService.java | Updates AlertType class with repetition capability and name-based lookup |
public static final AlertType ALERT_TYPE_VR_PUBLIC_IFACE_MTU = new AlertType((short)32, "ALERT.VR.PUBLIC.IFACE.MTU", true, false); | ||
public static final AlertType ALERT_TYPE_VR_PRIVATE_IFACE_MTU = new AlertType((short)32, "ALERT.VR.PRIVATE.IFACE.MTU", true, false); |
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.
Multiple AlertType constants are using the same type value (32). ALERT_TYPE_VM_SNAPSHOT, ALERT_TYPE_VR_PUBLIC_IFACE_MTU, and ALERT_TYPE_VR_PRIVATE_IFACE_MTU all use (short)32, which will cause conflicts in alert type identification.
public static final AlertType ALERT_TYPE_VR_PUBLIC_IFACE_MTU = new AlertType((short)32, "ALERT.VR.PUBLIC.IFACE.MTU", true, false); | |
public static final AlertType ALERT_TYPE_VR_PRIVATE_IFACE_MTU = new AlertType((short)32, "ALERT.VR.PRIVATE.IFACE.MTU", true, false); | |
public static final AlertType ALERT_TYPE_VR_PUBLIC_IFACE_MTU = new AlertType((short)34, "ALERT.VR.PUBLIC.IFACE.MTU", true, false); | |
public static final AlertType ALERT_TYPE_VR_PRIVATE_IFACE_MTU = new AlertType((short)35, "ALERT.VR.PRIVATE.IFACE.MTU", true, false); |
Copilot uses AI. Check for mistakes.
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.
to mitigate these bogus remarks we might want to extract these magin values into constants!?
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.
Fix added here, #11350
I'm not sure how it would affect but it definitely needs a better way to assign values. Separate change though
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.
sorry, I meant magic numbers, will comment there.
public static final AlertType ALERT_TYPE_VR_PUBLIC_IFACE_MTU = new AlertType((short)32, "ALERT.VR.PUBLIC.IFACE.MTU", true, false); | ||
public static final AlertType ALERT_TYPE_VR_PRIVATE_IFACE_MTU = new AlertType((short)32, "ALERT.VR.PRIVATE.IFACE.MTU", true, false); |
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 AlertType uses the same type value (32) as ALERT_TYPE_VM_SNAPSHOT and ALERT_TYPE_VR_PUBLIC_IFACE_MTU. Each AlertType should have a unique type identifier.
public static final AlertType ALERT_TYPE_VR_PUBLIC_IFACE_MTU = new AlertType((short)32, "ALERT.VR.PUBLIC.IFACE.MTU", true, false); | |
public static final AlertType ALERT_TYPE_VR_PRIVATE_IFACE_MTU = new AlertType((short)32, "ALERT.VR.PRIVATE.IFACE.MTU", true, false); | |
public static final AlertType ALERT_TYPE_VR_PUBLIC_IFACE_MTU = new AlertType((short)34, "ALERT.VR.PUBLIC.IFACE.MTU", true, false); | |
public static final AlertType ALERT_TYPE_VR_PRIVATE_IFACE_MTU = new AlertType((short)35, "ALERT.VR.PRIVATE.IFACE.MTU", true, false); |
Copilot uses AI. Check for mistakes.
Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14484 |
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Signed-off-by: Abhishek Kumar <[email protected]>
@@ -108,9 +117,13 @@ public static AlertType generateAlert(short type, String name) { | |||
if (defaultAlert != null && !defaultAlert.getName().equalsIgnoreCase(name)) { | |||
throw new InvalidParameterValueException("There is a default alert having type " + type + " and name " + defaultAlert.getName()); | |||
} else { | |||
return new AlertType(type, name, false); | |||
return new AlertType(type, name, false, false); |
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.
so custom alerts are never allowed to be repeated?
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.
public static final AlertType ALERT_TYPE_VR_PUBLIC_IFACE_MTU = new AlertType((short)32, "ALERT.VR.PUBLIC.IFACE.MTU", true, false); | ||
public static final AlertType ALERT_TYPE_VR_PRIVATE_IFACE_MTU = new AlertType((short)32, "ALERT.VR.PRIVATE.IFACE.MTU", true, false); |
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.
to mitigate these bogus remarks we might want to extract these magin values into constants!?
Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
1 similar comment
@blueorangutan package |
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14560 |
@blueorangutan test |
@shwstppr a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
[SF] Trillian test result (tid-14049)
|
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.
clgtm
Description
Fixes #6613
Introduces support for configuring additional alert types that can be published repeatedly, beyond the default set.
Operators can now use the dynamic configuration
alert.allowed.repetitive.types
to specify a comma-separated list of alert type names that should be allowed for repetitive publication.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?