Fix duplicate dummy templates, and update guest os for dummy template#12780
Fix duplicate dummy templates, and update guest os for dummy template#12780sureshanaparti wants to merge 4 commits intoapache:4.22from
Conversation
…m existing id: 1 (CentOS 4.5 (32-bit))
|
@blueorangutan package |
|
@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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12780 +/- ##
============================================
- Coverage 17.61% 17.61% -0.01%
Complexity 15664 15664
============================================
Files 5917 5917
Lines 531402 531432 +30
Branches 64971 64975 +4
============================================
- Hits 93596 93595 -1
- Misses 427252 427281 +29
- Partials 10554 10556 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Stops creation of duplicate KVM VM-import dummy templates by selecting the correct template name per hypervisor, and updates the KVM dummy template’s guest OS to “Other Linux (64-bit)” (id 99).
Changes:
- Use a KVM-specific dummy template name when importing on KVM to avoid repeatedly creating new templates.
- Change the dummy template guest OS id from 1 to 99 in code.
- Add a DB migration to update existing KVM dummy templates’
guest_os_idto 99.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java | Uses KVM-specific template lookup/creation and updates dummy template guest OS to id 99. |
| engine/schema/src/main/resources/META-INF/db/schema-42200to42210.sql | Migrates existing KVM dummy template rows to guest_os_id = 99. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| 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())); |
There was a problem hiding this 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()).
| 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())); | |
| 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)); |
| 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); |
There was a problem hiding this 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.
| -- 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'; |
There was a problem hiding this 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).
| UPDATE `cloud`.`vm_template` SET guest_os_id = 99 WHERE name = 'kvm-default-vm-import-dummy-template'; | |
| UPDATE `cloud`.`vm_template` | |
| SET guest_os_id = 99 | |
| WHERE name = 'kvm-default-vm-import-dummy-template' | |
| AND type = 'SYSTEM' | |
| AND format = 'ISO'; |
There was a problem hiding this comment.
fine with name
| -- 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'; |
There was a problem hiding this 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.
| UPDATE `cloud`.`vm_template` SET guest_os_id = 99 WHERE name = 'kvm-default-vm-import-dummy-template'; | |
| -- 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; |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17066 |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17090 |
| removeDuplicateDummyTemplates(conn); | ||
| } | ||
|
|
||
| private void removeDuplicateDummyTemplates(Connection conn) { |
There was a problem hiding this comment.
| private void removeDuplicateDummyTemplates(Connection conn) { | |
| private void removeDuplicateKVMImportTemplates(Connection conn) { |
| } | ||
| } | ||
| } catch (Exception e) { | ||
| logger.warn("Failed to clean duplicate kvm-default-vm-import-dummy-template entries", e); |
There was a problem hiding this comment.
| logger.warn("Failed to clean duplicate kvm-default-vm-import-dummy-template entries", e); | |
| logger.warn("Failed to clean duplicate " + UnmanagedVMsManager.KVM_VM_IMPORT_DEFAULT_TEMPLATE_NAME + " entries", e); |
Description
This PR fixes duplicate dummy templates, and updates guest os to "Other Linux (64-bit)", id: 99 for dummy template from existing "CentOS 4.5 (32-bit)", id: 1.
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?