-
Notifications
You must be signed in to change notification settings - Fork 1.3k
api/server: support deploy-as-is template as VNF template #12499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.20
Are you sure you want to change the base?
api/server: support deploy-as-is template as VNF template #12499
Conversation
|
@blueorangutan package |
|
@weizhouapache 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✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12499 +/- ##
=============================================
- Coverage 17.10% 4.03% -13.08%
=============================================
Files 5255 402 -4853
Lines 466415 32721 -433694
Branches 54746 5832 -48914
=============================================
- Hits 79763 1319 -78444
+ Misses 377768 31247 -346521
+ Partials 8884 155 -8729
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:
|
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm, one (dumb luser) question .
| if (registerCmd.isDeployAsIs() && CollectionUtils.isNotEmpty(registerCmd.getVnfNics())) { | ||
| throw new InvalidParameterValueException("VNF Template cannot be registered with VNF nics as Template settings are read from OVA."); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this always to be true? I could imagine a VNF getting a default gateway… Probably my lack of knowledge. I hope this is in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (registerCmd.isDeployAsIs() && CollectionUtils.isNotEmpty(registerCmd.getVnfNics())) { | |
| throw new InvalidParameterValueException("VNF Template cannot be registered with VNF nics as Template settings are read from OVA."); | |
| } | |
| if (registerCmd.isDeployAsIs() && CollectionUtils.isNotEmpty(registerCmd.getVnfNics())) { | |
| throw new InvalidParameterValueException("VNF nics cannot be specified when register a deploy-as-is Template. Please wait until Template settings are read from OVA."); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it clear now ? @DaanHoogland
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for deploying VNF appliances using deploy-as-is templates (OVA files) by allowing the VNF template functionality to work with templates that have network configurations already defined in the OVA. Previously, deploying such templates failed with "VNF nics list is empty" error.
Changes:
- Added conditional validation logic to handle both regular VNF templates (using
networkIdslist) and deploy-as-is VNF templates (usingvmNetworkMap) - Modified UI to disable network selection when deploy-as-is template networks are pre-configured
- Added validation to prevent registering/updating VNF nics on deploy-as-is templates since settings are read from the OVA
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/compute/wizard/VnfNicsSelection.vue | Adds templateNics prop and disables network selection for deploy-as-is templates |
| ui/src/views/compute/DeployVnfAppliance.vue | Passes templateNics to VnfNicsSelection component and updates validation logic to handle deploy-as-is template networks |
| api/src/main/java/org/apache/cloudstack/storage/template/VnfTemplateManager.java | Adds new interface method validateVnfApplianceNetworksMap for deploy-as-is template validation |
| server/src/main/java/org/apache/cloudstack/storage/template/VnfTemplateManagerImpl.java | Implements validateVnfApplianceNetworksMap and adds check to prevent VNF nics on deploy-as-is templates |
| api/src/main/java/org/apache/cloudstack/storage/template/VnfTemplateUtils.java | Adds validation methods for deploy-as-is templates: prevents VNF nics registration and validates OVF networks |
| server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | Routes to appropriate validation method based on whether template is deploy-as-is |
| server/src/main/java/com/cloud/template/TemplateManagerImpl.java | Adds validation for VNF nics updates on deploy-as-is templates using OVF network data |
| server/src/test/java/com/cloud/template/TemplateManagerImplTest.java | Adds mock bean for TemplateDeployAsIsDetailsDao to support new validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (vnfNic.getDeviceId() < ovfNetworks.size() && !vnfNic.isRequired()) { | ||
| throw new InvalidParameterValueException(String.format("The VNF nic [device ID: %s ] is required as it is defined in the OVA template.", vnfNic.getDeviceId())); | ||
| } |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic has a flaw. It checks if vnfNic.getDeviceId() < ovfNetworks.size() && !vnfNic.isRequired(), which throws an error if the vnfNic is NOT required. This is backwards - the error message says "The VNF nic [device ID: %s ] is required" but the condition checks !vnfNic.isRequired(). The logic should be: if the device ID corresponds to an OVF network, the VNF nic MUST be required. So the condition should be checking if it's defined in OVF but marked as not required in the VNF configuration.
server/src/main/java/org/apache/cloudstack/storage/template/VnfTemplateManagerImpl.java
Show resolved
Hide resolved
| @Override | ||
| public void validateVnfApplianceNetworksMap(VirtualMachineTemplate template, Map<Integer, Long> vmNetworkMap) { | ||
| if (MapUtils.isEmpty(vmNetworkMap)) { | ||
| throw new InvalidParameterValueException("VNF networks map is empty"); | ||
| } | ||
| List<VnfTemplateNicVO> vnfNics = vnfTemplateNicDao.listByTemplateId(template.getId()); | ||
| for (VnfTemplateNicVO vnfNic : vnfNics) { | ||
| if (vnfNic.isRequired() && vmNetworkMap.size() <= vnfNic.getDeviceId()) { | ||
| throw new InvalidParameterValueException("VNF nic is required but not found: " + vnfNic); | ||
| } | ||
| } | ||
| } |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new method validateVnfApplianceNetworksMap is not covered by any unit tests. Since this is a critical validation method for deploy-as-is VNF templates and similar validation methods in the codebase have test coverage, unit tests should be added to verify the validation logic works correctly.
| public static void validateDeployAsIsTemplateVnfNics(List<OVFNetworkTO> ovfNetworks, List<VNF.VnfNic> vnfNics) { | ||
| if (CollectionUtils.isEmpty(vnfNics)) { | ||
| return; | ||
| } | ||
| if (CollectionUtils.isEmpty(ovfNetworks)) { | ||
| throw new InvalidParameterValueException("The list of networks read from OVA is empty. Please wait until the template is fully downloaded and processed."); | ||
| } | ||
| for (VNF.VnfNic vnfNic : vnfNics) { | ||
| if (vnfNic.getDeviceId() < ovfNetworks.size() && !vnfNic.isRequired()) { | ||
| throw new InvalidParameterValueException(String.format("The VNF nic [device ID: %s ] is required as it is defined in the OVA template.", vnfNic.getDeviceId())); | ||
| } | ||
| } | ||
| } |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new method validateDeployAsIsTemplateVnfNics is not covered by any unit tests. This is a new validation method for deploy-as-is VNF templates, and given that similar validation methods in the codebase have test coverage, unit tests should be added to ensure the validation logic works as expected.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16480 |
server/src/main/java/org/apache/cloudstack/storage/template/VnfTemplateManagerImpl.java
Show resolved
Hide resolved
| if (cmd instanceof UpdateVnfTemplateCmd) { | ||
| VnfTemplateUtils.validateApiCommandParams(cmd, template); | ||
| UpdateVnfTemplateCmd updateCmd = (UpdateVnfTemplateCmd) cmd; | ||
| if (template.isDeployAsIs() && CollectionUtils.isNotEmpty(updateCmd.getVnfNics())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this parameter allowed on update but not on VM creation? (https://github.com/apache/cloudstack/pull/12499/files#diff-f5ab861d900497f6f7c83d03a840a7994f407c2dd52c237a4b560aaf9e75c202R128)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nvazquez
when register a deploy-as-is template, the template NICs are not available until the template is downloaded successfully.
I think it is better that user configures VNF nics only when template NICs are fetched from OVA template.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…fTemplateManagerImpl.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…plate/VnfTemplateManagerImpl.java" This reverts commit 0bfc65a.
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
NOTE #12436 (comment) -> check tooltip fix while verifying this PR |
|
@blueorangutan package |
|
thanks @DaanHoogland @nvazquez can you please review ? thanks |
Description
This PR fixes #12186
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?