Allow full clone volumes with thin provisioning #11177
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11177 +/- ##
============================================
- Coverage 16.57% 16.57% -0.01%
Complexity 13988 13988
============================================
Files 5745 5745
Lines 510847 510853 +6
Branches 62140 62142 +2
============================================
- Hits 84696 84682 -14
- Misses 416677 416705 +28
+ Partials 9474 9466 -8
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:
|
sureshanaparti
requested review from
slavkap,
DaanHoogland,
weizhouapache,
winterhazel,
bernardodemarco and
Copilot
|
@blueorangutan package |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new create.full.clone configuration to allow creating THIN-provisioned QCOW2 volumes as full clones instead of linked clones.
- Introduces a boolean agent property
CREATE_FULL_CLONE(defaultfalse). - Updates
createDiskFromTemplateto branch on the new property for THIN provisioning. - Adds default entry in
agent.propertiesand Javadoc inAgentProperties.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java | Conditional full-clone logic added for THIN volumes based on new property |
| agent/src/main/java/com/cloud/agent/properties/AgentProperties.java | Defined CREATE_FULL_CLONE property with Javadoc and default value |
| agent/conf/agent.properties | Added commented default for create.full.clone |
Comments suppressed due to low confidence (1)
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:1327
- There are no unit or integration tests verifying the new full-clone path; adding tests to cover both true and false scenarios for CREATE_FULL_CLONE would ensure the behavior remains correct.
if (createFullClone) {
| @@ -1315,14 +1317,22 @@ public KVMPhysicalDisk createDiskFromTemplate(KVMPhysicalDisk template, | |||
| passphraseObjects.add(QemuObject.prepareSecretForQemuImg(format, QemuObject.EncryptFormat.LUKS, keyFile.toString(), "sec0", options)); | |||
| disk.setQemuEncryptFormat(QemuObject.EncryptFormat.LUKS); | |||
| } | |||
|
|
|||
| QemuImgFile srcFile = new QemuImgFile(template.getPath(), template.getFormat()); | |||
| Boolean createFullClone = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.CREATE_FULL_CLONE); | |||
There was a problem hiding this comment.
Reading the agent properties on each disk creation may incur unnecessary overhead; consider caching the CREATE_FULL_CLONE value or loading it once at adaptor initialization.
| Boolean createFullClone = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.CREATE_FULL_CLONE); |
Copilot uses AI. Check for mistakes.
| @@ -1315,14 +1317,22 @@ public KVMPhysicalDisk createDiskFromTemplate(KVMPhysicalDisk template, | |||
| passphraseObjects.add(QemuObject.prepareSecretForQemuImg(format, QemuObject.EncryptFormat.LUKS, keyFile.toString(), "sec0", options)); | |||
| disk.setQemuEncryptFormat(QemuObject.EncryptFormat.LUKS); | |||
| } | |||
|
|
|||
| QemuImgFile srcFile = new QemuImgFile(template.getPath(), template.getFormat()); | |||
| Boolean createFullClone = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.CREATE_FULL_CLONE); | |||
There was a problem hiding this comment.
[nitpick] Prefer using a primitive boolean instead of the Boolean wrapper to avoid potential null unboxing issues and reduce overhead.
| Boolean createFullClone = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.CREATE_FULL_CLONE); | |
| boolean createFullClone = Boolean.TRUE.equals(AgentPropertiesFileHandler.getPropertyValue(AgentProperties.CREATE_FULL_CLONE)); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
took me a while of looking at this to have the penny drop. Code looks good @JoaoJandre , but what is the use case for this? I’d say one would want thin to be thin and fat to be fat. It seems now thin still makes a full clone given this setting and effectively becomes fat.
@DaanHoogland When using So, if you convert an image with virtual size as 50 GB, but real size 5 GB, using FULL, you'll get an image with 50 GB of real size. Using OFF, you'll end up with an image of 5GB of real size. This is the use case of THIN and full-clone. You copy the template but do not preallocate the full disk size. |
ok, so this is not a thick provisioned copy but just a byte by byte copy. |
|
For me, the code looks good, but my only concern here is that the configuration |
makes sense |
Description
This PR adds a configuration called
create.full.cloneto the agent.properties file. When set to true, all QCOW2 volumes created will be full-clone. If false (default), the current behavior remains, where only FAT and SPARSE volumes are full-clone and THIN volumes are linked-clone.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Without setting the property, I deployed a VM, which used linked-clone to create the volume.
Afterward, I added the
create.full.cloneproperty toagent.properties, restarted the agent service, and created another VM. This time, the volume was full-clone.How did you try to break this feature and the system with this change?