Copilot commented on code in PR #13930:
URL: https://github.com/apache/cloudstack/pull/13930#discussion_r3820919223
##########
plugins/storage/sharedfs/storagevm/src/main/java/org/apache/cloudstack/storage/sharedfs/lifecycle/StorageVmSharedFSLifeCycle.java:
##########
@@ -180,8 +180,13 @@ private UserVm deploySharedFSVM(Long zoneId, Account
owner, List<Long> networkId
for (final Iterator<Hypervisor.HypervisorType> iter =
hypervisors.iterator(); iter.hasNext();) {
final Hypervisor.HypervisorType hypervisor = iter.next();
VMTemplateVO template =
templateDao.findSystemVMReadyTemplate(zoneId, hypervisor,
preferredArchitecture);
- if (template == null && !iter.hasNext()) {
- throw new CloudRuntimeException(String.format("Unable to find
the systemvm template for %s or it was not downloaded in %s.",
hypervisor.toString(), zone.toString()));
+ if (template == null) {
+ if (iter.hasNext()) {
+ continue;
+ } else {
Review Comment:
The new behavior (skipping hypervisors where no SystemVM template is found
and continuing to the next) is not covered by a regression test. Current tests
only cover the single-hypervisor happy path and the “no template at all” path,
but not the original failure mode where the first hypervisor has no template
and a later one does (previously leading to an NPE). Please add a unit test
that returns multiple hypervisors (e.g., [External, KVM] or [Hyperv, KVM]) and
stubs templateDao.findSystemVMReadyTemplate to return null for the first and a
template for the second, asserting deploySharedFS succeeds and doesn’t throw.
##########
plugins/storage/sharedfs/storagevm/src/main/java/org/apache/cloudstack/storage/sharedfs/lifecycle/StorageVmSharedFSLifeCycle.java:
##########
@@ -180,8 +180,13 @@ private UserVm deploySharedFSVM(Long zoneId, Account
owner, List<Long> networkId
for (final Iterator<Hypervisor.HypervisorType> iter =
hypervisors.iterator(); iter.hasNext();) {
final Hypervisor.HypervisorType hypervisor = iter.next();
VMTemplateVO template =
templateDao.findSystemVMReadyTemplate(zoneId, hypervisor,
preferredArchitecture);
- if (template == null && !iter.hasNext()) {
- throw new CloudRuntimeException(String.format("Unable to find
the systemvm template for %s or it was not downloaded in %s.",
hypervisor.toString(), zone.toString()));
+ if (template == null) {
+ if (iter.hasNext()) {
+ continue;
+ } else {
+ throw new CloudRuntimeException(String.format("Unable to
find the systemvm template for any of the available hypervisors in the Zone:
%s", zone.toString()));
Review Comment:
Error message uses inconsistent casing/wording (“systemvm”, “Zone: ...”).
CloudStack elsewhere consistently uses “SystemVM”. Consider updating this
message for clarity and consistency.
--
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]
