Re: [PR] Check for null host before proceeding with VM volume operations in managed storage while restoring VM [cloudstack]

2026-03-26 Thread via GitHub 


blueorangutan commented on PR #12879:
URL: https://github.com/apache/cloudstack/pull/12879#issuecomment-4138132463

   [SF] Trillian test result (tid-15744)
   Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
   Total time taken: 49202 seconds
   Marvin logs: 
https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12879-t15744-kvm-ol8.zip
   Smoke tests completed. 148 look OK, 1 have errors, 0 did not run
   Only failed and skipped tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   ContextSuite context=TestClusterDRS>:setup | `Error` | 0.00 | 
test_cluster_drs.py
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Check for null host before proceeding with VM volume operations in managed storage while restoring VM [cloudstack]

2026-03-26 Thread via GitHub 


nvazquez merged PR #12879:
URL: https://github.com/apache/cloudstack/pull/12879


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Check for null host before proceeding with VM volume operations in managed storage while restoring VM [cloudstack]

2026-03-26 Thread via GitHub 


kiranchavala commented on PR #12879:
URL: https://github.com/apache/cloudstack/pull/12879#issuecomment-4131996829

   @blueorangutan test


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Check for null host before proceeding with VM volume operations in managed storage while restoring VM [cloudstack]

2026-03-25 Thread via GitHub 


blueorangutan commented on PR #12879:
URL: https://github.com/apache/cloudstack/pull/12879#issuecomment-4132007429

   @kiranchavala a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been 
kicked to run smoke tests


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Check for null host before proceeding with VM volume operations in managed storage while restoring VM [cloudstack]

2026-03-24 Thread via GitHub 


blueorangutan commented on PR #12879:
URL: https://github.com/apache/cloudstack/pull/12879#issuecomment-4116975202

   Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 
17228


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Check for null host before proceeding with VM volume operations in managed storage while restoring VM [cloudstack]

2026-03-24 Thread via GitHub 


Copilot commented on code in PR #12879:
URL: https://github.com/apache/cloudstack/pull/12879#discussion_r2980152809


##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -1194,21 +1194,21 @@ private UserVm rebootVirtualMachine(long userId, long 
vmId, boolean enterSetup,
 List vmNetworks = _vmNetworkMapDao.getNetworks(vmId);
 List routers = new 
ArrayList();
 //List the stopped routers
-for(long vmNetworkId : vmNetworks) {
+for (long vmNetworkId : vmNetworks) {
 List router = 
_routerDao.listStopped(vmNetworkId);
 routers.addAll(router);
 }
 //A vm may not have many nics attached and even fewer 
routers might be stopped (only in exceptional cases)
 //Safe to start the stopped router serially, this is 
consistent with the way how multiple networks are added to vm during deploy
 //and routers are started serially ,may revisit to make 
this process parallel
-for(DomainRouterVO routerToStart : routers) {
+for (DomainRouterVO routerToStart : routers) {
 logger.warn("Trying to start router {} as part of vm: 
{} reboot", routerToStart, vm);
 
_virtualNetAppliance.startRouter(routerToStart.getId(),true);
 }
 }
 } catch (ConcurrentOperationException e) {
 throw new CloudRuntimeException("Concurrent operations on 
starting router. " + e);
-} catch (Exception ex){
+} catch (Exception ex) {
 throw new CloudRuntimeException("Router start failed due to" + 
ex);

Review Comment:
   The exception message concatenation is missing a space ("due to" + ex) and 
the thrown CloudRuntimeException drops the original cause/stack trace. Consider 
including a separating space and passing the caught exception as the cause so 
callers/logs preserve details.



##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -9221,8 +9219,14 @@ private void handleManagedStorage(UserVmVO vm, VolumeVO 
root) {
 Long hostId = vm.getHostId() != null ? vm.getHostId() : 
vm.getLastHostId();
 
 if (hostId != null) {
-VolumeInfo volumeInfo = volFactory.getVolume(root.getId());
+// default findById() won't search entries with removed field 
not null
 Host host = _hostDao.findById(hostId);
+if (host == null) {
+logger.warn("Host {} not found", hostId);

Review Comment:
   This new log line only prints the hostId; when debugging restore/re-image 
issues it would be helpful to include the VM and root volume identifiers (e.g., 
vm UUID/id and root volume UUID/id) in the warning so operators can correlate 
the skip with the affected resources.
   ```suggestion
   logger.warn("Host {} not found for vm id: {}, uuid: {}, 
root volume id: {}, uuid: {}", hostId, vm.getId(), vm.getUuid(), root.getId(), 
root.getUuid());
   ```



##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -9221,8 +9219,14 @@ private void handleManagedStorage(UserVmVO vm, VolumeVO 
root) {
 Long hostId = vm.getHostId() != null ? vm.getHostId() : 
vm.getLastHostId();
 
 if (hostId != null) {
-VolumeInfo volumeInfo = volFactory.getVolume(root.getId());
+// default findById() won't search entries with removed field 
not null
 Host host = _hostDao.findById(hostId);
+if (host == null) {
+logger.warn("Host {} not found", hostId);
+return;
+}

Review Comment:
   There are unit tests for restoreVirtualMachine() in 
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java, but none appear 
to cover the managed-storage restore path where vm.getLastHostId() is set and 
_hostDao.findById(hostId) returns null (deleted host). Adding a test for this 
scenario would help prevent regressions (e.g., ensure restore proceeds without 
leaving an extra ROOT volume in Allocated state).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Check for null host before proceeding with VM volume operations in managed storage while restoring VM [cloudstack]

2026-03-24 Thread via GitHub 


codecov[bot] commented on PR #12879:
URL: https://github.com/apache/cloudstack/pull/12879#issuecomment-4116610844

   ## 
[Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/12879?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 3.70%. Comparing base 
([`bce5594`](https://app.codecov.io/gh/apache/cloudstack/commit/bce55945ece8ee7453b2ee42426e823b31643d43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
 to head 
([`2d51a71`](https://app.codecov.io/gh/apache/cloudstack/commit/2d51a710c21f3a25463a01994c1b1f690035c4d3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   > :exclamation:  There is a different number of reports uploaded between 
BASE (bce5594) and HEAD (2d51a71). Click for more details.
   > 
   > HEAD has 1 upload less than BASE
   >
   >| Flag | BASE (bce5594) | HEAD (2d51a71) |
   >|--|--|--|
   >|unittests|1|0|
   >
   
   Additional details and impacted files
   
   
   
   ```diff
   @@  Coverage Diff  @@
   ##   4.22   #12879   +/-   ##
   =
   - Coverage 17.61%3.70%   -13.91% 
   =
 Files  5917  448 -5469 
 Lines53146138028   -493433 
 Branches  64977 7036-57941 
   =
   - Hits  93608 1409-92199 
   + Misses   42729536432   -390863 
   + Partials  10558  187-10371 
   ```
   
   | 
[Flag](https://app.codecov.io/gh/apache/cloudstack/pull/12879/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Coverage Δ | |
   |---|---|---|
   | 
[uitests](https://app.codecov.io/gh/apache/cloudstack/pull/12879/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `3.70% <ø> (ø)` | |
   | 
[unittests](https://app.codecov.io/gh/apache/cloudstack/pull/12879/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click 
here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment)
 to find out more.
   
   
   [:umbrella: View full report in Codecov by 
Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/12879?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   
   :loudspeaker: Have feedback on the report? [Share it 
here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:rocket: New features to boost your workflow: 
   
   - :snowflake: [Test 
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, 
report on failures, and find test suite problems.
   - :package: [JS Bundle 
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save 
yourself from yourself by tracking and limiting bundle sizes in JS merges.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Check for null host before proceeding with VM volume operations in managed storage while restoring VM [cloudstack]

2026-03-24 Thread via GitHub 


blueorangutan commented on PR #12879:
URL: https://github.com/apache/cloudstack/pull/12879#issuecomment-4116607628

   @sureshanaparti 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Check for null host before proceeding with VM volume operations in managed storage while restoring VM [cloudstack]

2026-03-24 Thread via GitHub 


sureshanaparti commented on PR #12879:
URL: https://github.com/apache/cloudstack/pull/12879#issuecomment-4116594463

   @blueorangutan package


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]