alert messages restructure - #13775
Open
DaanHoogland wants to merge 20 commits into
Open
Conversation
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.
added 15 commits
August 16, 2026 16:59
DaanHoogland
force-pushed
the
ghi7297-messages
branch
from
August 16, 2026 14:59
bcd5b3b to
61f1c3d
Compare
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Some new alert-message paths can regress to misleading/less-actionable output when lookups return null, and a new test installs an ExecutorService without teardown which can leak threads across the suite.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:7807
- Similar to the account dedication alert: if a host is dedicated to a domain but that domain lookup returns null, the alert currently prints
nulland may misrepresent the destination as “not dedicated to a specific domain”. Consider falling back to ids while preserving whether the host is domain-dedicated.
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");
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:7819
- The implicit-dedication alerts now log the Account object; if the account lookup returns null, these messages become much less actionable (they just print
null). Consider using a stable fallback likeid: <accountId>when the Account cannot be resolved.
Account accountOfVmObj = _accountDao.findById(accountOfVm);
String msg = String.format("VM of account %s with implicit deployment planner being migrated to host %s", accountOfVmObj, destHost);
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:7832
- If the account lookup returns null, this alert will render as “Account null…”, losing the numeric account id that’s still available. Consider falling back to the account id when formatting the message.
msg = String.format("Instance of Account %s with strict implicit deployment planner being migrated to host %s not having all Instances strict implicitly dedicated to Account %s", accountOfVmObj, destHost, accountOfVmObj);
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:7840
- Same as the strict-implicit alert above: if the account cannot be resolved, the message becomes
nulland drops the account id. Consider falling back to the numeric id for readability/actionability.
msg = String.format("Instance of Account %s with preferred implicit deployment planner being migrated to host %s not having all Instances implicitly dedicated to Account %s", accountOfVmObj, destHost, accountOfVmObj);
- Files reviewed: 31/31 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
+97
to
+106
| @BeforeClass | ||
| public static void setUpStaticFields() throws Exception { | ||
| Field cacheField = OutOfBandManagementServiceImpl.class.getDeclaredField("hostAlertCache"); | ||
| cacheField.setAccessible(true); | ||
| cacheField.set(null, CacheBuilder.newBuilder().build()); | ||
|
|
||
| Field executorField = OutOfBandManagementServiceImpl.class.getDeclaredField("backgroundSyncBlockingExecutor"); | ||
| executorField.setAccessible(true); | ||
| executorField.set(null, Executors.newSingleThreadExecutor()); | ||
| } |
Comment on lines
+537
to
+539
| } else { | ||
| hostDesc = "host id: " + hostId; | ||
| } |
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces at least one confirmed NPE risk in alert formatting and a test that leaks mutated static state across the JVM, both of which can cause order-dependent failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 31/31 changed files
- Comments generated: 4
- Review effort level: Lite
Comment on lines
2007
to
2011
| final DataCenterVO dcVO = _dcDao.findById(host.getDataCenterId()); | ||
| final HostPodVO podVO = _podDao.findById(host.getPodId()); | ||
| final String hostDesc = "name: " + host.getName() + " (id:" + host.getId() + "), availability zone: " + dcVO.getName() + ", pod: " + podVO.getName(); | ||
| _alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_HOST, host.getDataCenterId(), host.getPodId(), "Migration Complete for host " + hostDesc, | ||
| _alertMgr.sendAlert(AlertService.AlertType.ALERT_TYPE_HOST, host.getDataCenterId(), host.getPodId(), "Migration Complete for host " + hostDesc, | ||
| "Host [" + hostDesc + "] is ready for maintenance"); |
Comment on lines
+98
to
+118
| private OutOfBandManagementServiceImpl service; | ||
|
|
||
| @BeforeClass | ||
| public static void setUpStaticFields() throws Exception { | ||
| Field cacheField = OutOfBandManagementServiceImpl.class.getDeclaredField("hostAlertCache"); | ||
| cacheField.setAccessible(true); | ||
| cacheField.set(null, CacheBuilder.newBuilder().build()); | ||
|
|
||
| Field executorField = OutOfBandManagementServiceImpl.class.getDeclaredField("backgroundSyncBlockingExecutor"); | ||
| executorField.setAccessible(true); | ||
| executorField.set(null, Executors.newSingleThreadExecutor()); | ||
| } | ||
|
|
||
| @AfterClass | ||
| public static void tearDownStaticFields() throws Exception { | ||
| Field executorField = OutOfBandManagementServiceImpl.class.getDeclaredField("backgroundSyncBlockingExecutor"); | ||
| executorField.setAccessible(true); | ||
| ExecutorService executor = (ExecutorService) executorField.get(null); | ||
| executor.shutdownNow(); | ||
| executor.awaitTermination(5, TimeUnit.SECONDS); | ||
| } |
Comment on lines
165
to
167
| if (!answer.getResult()) { | ||
| String msg = String.format("Unable to attach storage pool %s to the host %d", pool, hostId); | ||
| 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); |
Comment on lines
299
to
303
| if (!answer.getResult()) { | ||
| String msg = String.format("Unable to attach storage pool %s to host %d", storagePool, hostId); | ||
| 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); |
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?