Copilot commented on code in PR #12780:
URL: https://github.com/apache/cloudstack/pull/12780#discussion_r2910865242
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1550,9 +1550,11 @@ private ServiceOfferingVO
getServiceOfferingForImportInstance(Long serviceOfferi
protected VMTemplateVO getTemplateForImportInstance(Long templateId,
Hypervisor.HypervisorType hypervisorType) {
VMTemplateVO template;
if (templateId == null) {
- template = templateDao.findByName(VM_IMPORT_DEFAULT_TEMPLATE_NAME);
+ boolean isKVMHypervisor =
Hypervisor.HypervisorType.KVM.equals(hypervisorType);
+ String templateName = (isKVMHypervisor) ?
KVM_VM_IMPORT_DEFAULT_TEMPLATE_NAME : VM_IMPORT_DEFAULT_TEMPLATE_NAME;
+ template = templateDao.findByName(templateName);
if (template == null) {
- template =
createDefaultDummyVmImportTemplate(Hypervisor.HypervisorType.KVM ==
hypervisorType);
+ template = createDefaultDummyVmImportTemplate(isKVMHypervisor);
if (template == null) {
throw new
InvalidParameterValueException(String.format("Default VM import template with
unique name: %s for hypervisor: %s cannot be created. Please use templateid
parameter for import", VM_IMPORT_DEFAULT_TEMPLATE_NAME,
hypervisorType.toString()));
Review Comment:
The exception message always formats the template name using
`VM_IMPORT_DEFAULT_TEMPLATE_NAME`, even though the code now conditionally uses
`templateName` (e.g., `KVM_VM_IMPORT_DEFAULT_TEMPLATE_NAME` for KVM). This will
report the wrong template name for KVM and mislead operators. Use
`templateName` in the formatted message (and you can pass `hypervisorType`
directly instead of `hypervisorType.toString()`).
```suggestion
throw new
InvalidParameterValueException(String.format("Default VM import template with
unique name: %s for hypervisor: %s cannot be created. Please use templateid
parameter for import", templateName, hypervisorType));
```
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -326,7 +326,7 @@ private VMTemplateVO
createDefaultDummyVmImportTemplate(boolean isKVM) {
try {
template =
VMTemplateVO.createSystemIso(templateDao.getNextInSequence(Long.class, "id"),
templateName, templateName, true,
"", true, 64, Account.ACCOUNT_ID_SYSTEM, "",
- "VM Import Default Template", false, 1);
+ "VM Import Default Template", false, 99);
Review Comment:
The guest OS id `99` is a magic number here, which makes the intent harder
to understand and couples behavior to a specific seeded DB id. Prefer using a
named constant (e.g., `OTHER_LINUX_64_GUEST_OS_ID`) or resolving the guest OS
id via a DAO lookup by a stable key/name, so this remains readable and robust
across variations in guest OS seed data.
##########
engine/schema/src/main/resources/META-INF/db/schema-42200to42210.sql:
##########
@@ -33,3 +33,5 @@ UPDATE `cloud`.`alert` SET type = 34 WHERE name =
'ALERT.VR.PRIVATE.IFACE.MTU';
-- Update configuration 'kvm.ssh.to.agent' description and is_dynamic fields
UPDATE `cloud`.`configuration` SET description = 'True if the management
server will restart the agent service via SSH into the KVM hosts after or
during maintenance operations', is_dynamic = 1 WHERE name = 'kvm.ssh.to.agent';
+
+UPDATE `cloud`.`vm_template` SET guest_os_id = 99 WHERE name =
'kvm-default-vm-import-dummy-template';
Review Comment:
The PR description shows many duplicate rows for
`kvm-default-vm-import-dummy-template`. This migration updates `guest_os_id`
but does not address the existing duplicates, so the system can continue
operating with ambiguous data (and `findByName` may pick an arbitrary row
depending on query ordering). Consider adding a migration step to de-duplicate
(e.g., keep a single canonical row and mark others removed/unused) to make
behavior deterministic and align with the PR’s stated goal of fixing duplicates.
```suggestion
-- De-duplicate 'kvm-default-vm-import-dummy-template' entries and update
guest_os_id
-- Keep the template with the smallest id as the canonical one and mark
others as removed
UPDATE `cloud`.`vm_template` t
JOIN (
SELECT MIN(id) AS keep_id
FROM `cloud`.`vm_template`
WHERE name = 'kvm-default-vm-import-dummy-template'
) k ON t.id != k.keep_id
SET t.removed = NOW()
WHERE t.name = 'kvm-default-vm-import-dummy-template'
AND t.removed IS NULL;
-- Update guest_os_id only for the canonical template
UPDATE `cloud`.`vm_template` t
JOIN (
SELECT MIN(id) AS keep_id
FROM `cloud`.`vm_template`
WHERE name = 'kvm-default-vm-import-dummy-template'
AND removed IS NULL
) k ON t.id = k.keep_id
SET t.guest_os_id = 99;
```
##########
engine/schema/src/main/resources/META-INF/db/schema-42200to42210.sql:
##########
@@ -33,3 +33,5 @@ UPDATE `cloud`.`alert` SET type = 34 WHERE name =
'ALERT.VR.PRIVATE.IFACE.MTU';
-- Update configuration 'kvm.ssh.to.agent' description and is_dynamic fields
UPDATE `cloud`.`configuration` SET description = 'True if the management
server will restart the agent service via SSH into the KVM hosts after or
during maintenance operations', is_dynamic = 1 WHERE name = 'kvm.ssh.to.agent';
+
+UPDATE `cloud`.`vm_template` SET guest_os_id = 99 WHERE name =
'kvm-default-vm-import-dummy-template';
Review Comment:
This UPDATE targets rows by `name` only. To reduce the risk of
unintentionally modifying non-system templates (or future templates) that
happen to share the same name, consider narrowing the predicate (e.g., also
filter on `type = 'SYSTEM'` and `format = 'ISO'` if that matches the intended
rows).
```suggestion
UPDATE `cloud`.`vm_template`
SET guest_os_id = 99
WHERE name = 'kvm-default-vm-import-dummy-template'
AND type = 'SYSTEM'
AND format = 'ISO';
```
--
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]
