alert messages restructure - #13775
Open
DaanHoogland wants to merge 16 commits into
Open
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13775 +/- ##
============================================
+ Coverage 19.64% 19.68% +0.03%
- Complexity 19786 19826 +40
============================================
Files 6368 6369 +1
Lines 574889 574921 +32
Branches 70353 70365 +12
============================================
+ Hits 112957 113173 +216
+ Misses 449660 449462 -198
- Partials 12272 12286 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Restructures several alert/log messages to be more consistent and information-rich, and introduces a shared formatter for host location descriptions (fixing CLOUDSTACK-7297).
Changes:
- Added
AlertFormatUtils.describeHostLocation(...)and reused it in HA/agent/orchestration alert call sites. - Updated many alerts/logs to include object context (e.g.,
host,domain,account) rather than only IDs/UUIDs. - Tweaked multiple failure/permission messages to improve operator readability.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java | Updates host degraded/removed debug message formatting. |
| server/src/main/java/org/apache/cloudstack/ha/provider/host/HAAbstractHostProvider.java | Simplifies HA fencing/recovery alert subject/body to include host object. |
| server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java | Improves SSH provisioning error context by logging the host object. |
| server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | Reworks several VM/host dedication alert messages; adds account/domain lookups for richer output. |
| server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java | Updates snapshot limit alert to reference account object. |
| server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java | Updates permission-denied exception messages to include domain object. |
| server/src/main/java/com/cloud/ha/KVMFencer.java | Updates fencing failure alerts to include host object. |
| server/src/main/java/com/cloud/ha/HighAvailabilityManagerImpl.java | Uses AlertFormatUtils for host location in HA alerts; improves VM-stopped alert host description. |
| server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java | Makes several validation exceptions include full entity objects (domain/network offering). |
| server/src/main/java/com/cloud/alert/AlertManagerImpl.java | Changes alert logging to structured logging with entity objects. |
| plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java | Updates ScaleIO host/pool alerts/logs to include host object. |
| plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/driver/ScaleIOPrimaryDataStoreDriver.java | Updates ScaleIO disconnection checks/alerts to include host/pool objects. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java | Improves Ontap attach-storage error messages by using host object. |
| plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/provider/DateraHostListener.java | Improves Datera attach-storage alert message by using fetched host object. |
| engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java | Improves default host connect failure message by including host object. |
| engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java | Updates upload/register/copy failure alerts to include object string. |
| engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/ScaleIOVMSnapshotStrategy.java | Clarifies VM snapshot failure message to include snapshot object context. |
| engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java | Uses AlertFormatUtils to include host location in agent network-setup failure alert. |
| engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java | Uses AlertFormatUtils for consistent host location strings in disconnect/alert-state messages. |
| engine/components-api/src/main/java/com/cloud/alert/AlertFormatUtils.java | New shared helper for consistent host/zone/pod alert formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:2869
- The conditional uses
hostId != nullbut printshost(object). IfhostIdis set but the host lookup failed, this will produceon host [null]and loses the only reliable identifier (the host id). Consider usinghost != nullfor the host-object rendering, and fall back tohostIdwhenhostis null so the alert remains actionable.
String subject = String.format("Failed to deploy Instance [%s]", vm);
String body = String.format("Failed to deploy [%s]%s. To troubleshoot, please check the logs with [logid:%s].",
vm,
hostId != null ? String.format(" on host [%s]", host) : "",
ThreadContext.get("logcontextid"));
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:7769
- These alerts now depend on
Account.toString()/Domain.toString()for human-readable names, and will rendernullif the DAO lookup returns null—where previously the IDs were always present. To keep alerts stable and actionable, consider formatting explicitly (e.g., account name + id, domain name + id) with a fallback to the raw IDs when the DAO lookup fails, instead of relying ontoString().
Long srcAccountId = accountOfDedicatedHost(srcHost);
Long destAccountId = accountOfDedicatedHost(destHost);
if (!((srcAccountId == null) || (srcAccountId.equals(destAccountId)))) {
Account srcAccount = _accountDao.findById(srcAccountId);
Account destAccount = _accountDao.findById(destAccountId);
String msg = String.format("VM is being migrated from host %s explicitly dedicated to account %s to host %s explicitly dedicated to account %s",
srcHost, srcAccount, destHost, destAccount);
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:7779
- These alerts now depend on
Account.toString()/Domain.toString()for human-readable names, and will rendernullif the DAO lookup returns null—where previously the IDs were always present. To keep alerts stable and actionable, consider formatting explicitly (e.g., account name + id, domain name + id) with a fallback to the raw IDs when the DAO lookup fails, instead of relying ontoString().
Long srcDomainId = domainOfDedicatedHost(srcHost);
Long destDomainId = domainOfDedicatedHost(destHost);
if (!((srcDomainId == null) || (srcDomainId.equals(destDomainId)))) {
Domain srcDomain = _domainDao.findById(srcDomainId);
Domain destDomain = _domainDao.findById(destDomainId);
String msg = String.format("VM is being migrated from host %s explicitly dedicated to domain %s to host %s explicitly dedicated to domain %s",
srcHost, srcDomain, destHost, destDomain);
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java:4597
- This test hard-codes expectations around
toString()output forAccountVOandDomainVO, which is typically not a stable API and can change without functional impact. If the production intent is to include account/domain names, it would be more robust to have production code format using explicit getters (e.g., account name / domain name) and have the test stub those getters and assert on them.
AccountVO srcAccount = Mockito.mock(AccountVO.class);
when(srcAccount.toString()).thenReturn("Account {accountName=account-a}");
AccountVO destAccount = Mockito.mock(AccountVO.class);
when(destAccount.toString()).thenReturn("Account {accountName=account-b}");
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java:4604
- This test hard-codes expectations around
toString()output forAccountVOandDomainVO, which is typically not a stable API and can change without functional impact. If the production intent is to include account/domain names, it would be more robust to have production code format using explicit getters (e.g., account name / domain name) and have the test stub those getters and assert on them.
DomainVO srcDomain = Mockito.mock(DomainVO.class);
when(srcDomain.toString()).thenReturn("Domain {name=domain-a}");
DomainVO destDomain = Mockito.mock(DomainVO.class);
when(destDomain.toString()).thenReturn("Domain {name=domain-b}");
plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/provider/DateraHostListener.java:301
- If
_hostDao.findById(hostId)returns null, the message will become... to host null, which is less actionable than the previous host id-based message. Consider falling back tohostIdwhenhostis null (or rendering both: host + id) to preserve diagnostic value.
HostVO host = _hostDao.findById(hostId);
String msg = String.format("Unable to attach storage pool %s to host %s", storagePool, host);
engine/components-api/src/main/java/com/cloud/alert/AlertFormatUtils.java:26
- The PR description is still the default template (type-of-change checkboxes and testing details are not filled in). Since repository automation relies on the description for labeling/documentation, please update the PR description to clearly state the functional behavior change and how it was tested.
* Shared formatting for the host/zone/pod description that recurs, independently
* hand-rolled and inconsistently worded (and occasionally mislabelled), across the
* HA and agent-management alert call sites. See CLOUDSTACK-7297.
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.52% |
| Branch coverage | 18.69% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.53% |
| Branch coverage | 18.70% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.55% |
| Branch coverage | 18.71% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (2)
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:4650
- This error path explicitly allows domainId to be null, but the exception message still calls _entityMgr.findById(Domain.class, domainId). If domainId is null this can throw a NullPointerException (GenericDaoBase.findById uses cache.get(id)/lockRow(id) without null checks), preventing the intended InvalidParameterValueException from being returned.
if (domainId == null || !_domainDao.isChildDomain(account.getDomainId(), domainId)) {
throw new InvalidParameterValueException(String.format("Unable to create disk offering by another domain-admin: %s for domain: %s", user, _entityMgr.findById(Domain.class, domainId)));
}
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:3598
- The exception message calls _entityMgr.findById(Domain.class, domainId) without guarding against domainId being null. DomainHelper.filterChildSubDomains can return null entries if the input list contains nulls, and GenericDaoBase.findById(null) can throw a NullPointerException (e.g., via cache access), masking the intended InvalidParameterValueException.
This issue also appears on line 4648 of the same file.
if (!_domainDao.isChildDomain(account.getDomainId(), domainId)) {
throw new InvalidParameterValueException(String.format("Unable to create service offering by another domain-admin: %s for domain: %s", user, _entityMgr.findById(Domain.class, domainId)));
}
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.54% |
| Branch coverage | 18.70% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (5)
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:7787
- Same as the account alert above: if domain lookup returns null, the message becomes "... dedicated to domain null ..." and loses the domain id. Adding a simple fallback keeps the alert actionable even with missing domain records.
Domain srcDomain = _domainDao.findById(srcDomainId);
Domain destDomain = destDomainId != null ? _domainDao.findById(destDomainId) : null;
String msg = String.format("VM is being migrated from host %s explicitly dedicated to domain %s to host %s %s",
srcHost, srcDomain, destHost, destDomain != null ? "explicitly dedicated to domain " + destDomain : "not dedicated to a specific domain");
plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/provider/DateraHostListener.java:303
- The failure alert message can lose the host identifier when the host record is missing:
hostmay be null, resulting in "host null" with nohostId. This makes the alert less actionable in exactly the scenarios where DB lookup fails.
if (!answer.getResult()) {
HostVO host = _hostDao.findById(hostId);
String msg = String.format("Unable to attach storage pool %s to host %s", storagePool, host);
_alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_HOST, storagePool.getDataCenterId(), storagePool.getPodId(), msg, msg);
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java:167
- If
hostDao.findById(hostId)returns null, the alert will say "host null" and omit the host id. Keeping a fallback tohostIdpreserves actionable context during attach failures.
if (!answer.getResult()) {
String msg = String.format("Unable to attach storage pool %s to the host %s", pool, host);
alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_HOST, pool.getDataCenterId(), pool.getPodId(), msg, msg);
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:7777
- These alerts now rely on
Account.toString()/Domain.toString(). If_accountDao.findById(...)returns null (e.g., stale dedication records), the message becomes "... dedicated to account null ..." and loses the id that previously made the alert actionable. Consider falling back to the id when the entity lookup fails.
This issue also appears on line 7784 of the same file.
Account srcAccount = _accountDao.findById(srcAccountId);
Account destAccount = destAccountId != null ? _accountDao.findById(destAccountId) : null;
String msg = String.format("VM is being migrated from host %s explicitly dedicated to account %s to host %s %s",
srcHost, srcAccount, destHost, destAccount != null ? "explicitly dedicated to account " + destAccount : "not dedicated to a specific account");
server/src/main/java/com/cloud/ha/HighAvailabilityManagerImpl.java:525
- When
hostIdis null and the host record can't be resolved, the alert body will includehost id: null. It would be clearer to omit the id segment or use an explicit "unknown" value whenhostIdis null.
} else {
hostDesc = "host id: " + hostId;
}
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.55% |
| Branch coverage | 18.71% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR...
Fixes: #7297
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?