Skip to content

Add Hypervisor default as cache mode for disk offerings #10282

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Oct 8, 2025
Merged

Add Hypervisor default as cache mode for disk offerings #10282

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e6d1680
Add disk offering Hypervisor default cache mode
Jan 15, 2025
5178bad
Resolve conflicts
hsato03 Feb 3, 2025
9275789
Resolve conflicts
hsato03 Feb 14, 2025
d42432c
Address Joao review
hsato03 Feb 14, 2025
a20b6d5
Change ProviderAdapterDiskOffering as well
hsato03 Feb 14, 2025
04b49bc
resolve conflicts
hsato03 Sep 11, 2025
77b85f0
Move query
hsato03 Sep 11, 2025
638cebd
Resolve conflicts
hsato03 Sep 26, 2025
391ffb6
Address reviews
hsato03 Sep 26, 2025
976f2e1
Remove unused import
hsato03 Sep 26, 2025
a0c2a9f
Resolve conflicts
hsato03 Oct 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/src/main/java/com/cloud/offering/DiskOffering.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
State getState();

enum DiskCacheMode {
NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");
NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough"), HYPERVISOR_DEFAULT("hypervisor_default");

Check warning on line 40 in api/src/main/java/com/cloud/offering/DiskOffering.java

View check run for this annotation

Codecov / codecov/patch

api/src/main/java/com/cloud/offering/DiskOffering.java#L40 

Added line #L40 was not covered by tests

private final String _diskCacheMode;

Expand Down
7 changes: 6 additions & 1 deletion core/src/main/java/org/apache/cloudstack/storage/to/VolumeObjectTO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.apache.cloudstack.storage.to;

import com.cloud.agent.api.LogLevel;
import com.cloud.offering.DiskOffering;
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;

import com.cloud.agent.api.to.DataObjectType;
Expand Down Expand Up @@ -112,8 +113,8 @@
iopsWriteRate = volume.getIopsWriteRate();
iopsWriteRateMax = volume.getIopsWriteRateMax();
iopsWriteRateMaxLength = volume.getIopsWriteRateMaxLength();
cacheMode = volume.getCacheMode();
hypervisorType = volume.getHypervisorType();
setCacheMode(volume.getCacheMode());

Check warning on line 117 in core/src/main/java/org/apache/cloudstack/storage/to/VolumeObjectTO.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/apache/cloudstack/storage/to/VolumeObjectTO.java#L117 

Added line #L117 was not covered by tests
setDeviceId(volume.getDeviceId());
this.migrationOptions = volume.getMigrationOptions();
this.directDownload = volume.isDirectDownload();
Expand Down Expand Up @@ -337,6 +338,10 @@
}

public void setCacheMode(DiskCacheMode cacheMode) {
if (DiskCacheMode.HYPERVISOR_DEFAULT.equals(cacheMode) && !Hypervisor.HypervisorType.KVM.equals(hypervisorType)) {
this.cacheMode = DiskOffering.DiskCacheMode.NONE;
Copy link
Contributor

@DaanHoogland DaanHoogland Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit confusing. DiskCacheMode.HYPERVISOR_DEFAULT ?-> DiskOffering.DiskCacheMode.NONE. are these the same enum? and if so, why not keep it as HYPEVISOR_DEFAULT and handle it in the background.

Copy link
Contributor

@DaanHoogland DaanHoogland Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, it tests ok, but this is still strange.

Copy link
Member

@weizhouapache weizhouapache Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I understand, this code block affects other hypervisors , not KVM.
for KVM, it uses DiskCacheMode.HYPERVISOR_DEFAULT in the backend
for other hypervisors, it uses DiskCacheMode.NONE instead in the backend. I think we should mention it in API param description and documentation

Copy link
Contributor

@DaanHoogland DaanHoogland Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but also make clear these are the same enum/value and not accidentally similar named values from different locations. I.e. remove the DiskOffering bit:

Suggested change
if (DiskCacheMode.HYPERVISOR_DEFAULT.equals(cacheMode) && !Hypervisor.HypervisorType.KVM.equals(hypervisorType)) {
this.cacheMode = DiskOffering.DiskCacheMode.NONE;
if (DiskCacheMode.HYPERVISOR_DEFAULT.equals(cacheMode) && !Hypervisor.HypervisorType.KVM.equals(hypervisorType)) {
this.cacheMode = DiskCacheMode.NONE;

Copy link
Collaborator Author

@hsato03 hsato03 Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weizhouapache @DaanHoogland

I added some changes with my last commit. Could you please take a look and check if your concerns have been addressed?

return;

Check warning on line 343 in core/src/main/java/org/apache/cloudstack/storage/to/VolumeObjectTO.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/apache/cloudstack/storage/to/VolumeObjectTO.java#L342-L343 

Added lines #L342 - L343 were not covered by tests
}
this.cacheMode = cacheMode;
}

Expand Down
3 changes: 3 additions & 0 deletions engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,7 @@ FROM `cloud`.`role_permissions` rp
WHERE rp.rule = 'quotaStatement'
AND NOT EXISTS(SELECT 1 FROM cloud.role_permissions rp_ WHERE rp.role_id = rp_.role_id AND rp_.rule = 'quotaCreditsList');

-- Increase the cache_mode column size from cloud.disk_offering table
CALL `cloud`.`IDEMPOTENT_CHANGE_COLUMN`('cloud.disk_offering', 'cache_mode', 'cache_mode', 'varchar(18) DEFAULT "none" COMMENT "The disk cache mode to use for disks created with this offering"');

CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.host', 'last_mgmt_server_id', 'bigint unsigned DEFAULT NULL COMMENT "last management server this host is connected to" AFTER `mgmt_server_id`');
2 changes: 1 addition & 1 deletion plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,7 @@ public String toString() {
}

public enum DiskCacheMode {
NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");
NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough"), HYPERVISOR_DEFAULT("default");
String _diskCacheMode;

DiskCacheMode(String cacheMode) {
Expand Down
8 changes: 5 additions & 3 deletions ...ain/java/org/apache/cloudstack/storage/datastore/adapter/ProviderAdapterDiskOffering.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
this.type = ProvisioningType.getProvisioningType(hiddenDiskOffering.getProvisioningType().toString());
}
if (hiddenDiskOffering.getCacheMode() != null) {
this.diskCacheMode = DiskCacheMode.getDiskCasehMode(hiddenDiskOffering.getCacheMode().toString());
this.diskCacheMode = DiskCacheMode.getDiskCacheMode(hiddenDiskOffering.getCacheMode().toString());

Check warning on line 37 in plugins/storage/volume/adaptive/src/main/java/org/apache/cloudstack/storage/datastore/adapter/ProviderAdapterDiskOffering.java

View check run for this annotation

Codecov / codecov/patch

plugins/storage/volume/adaptive/src/main/java/org/apache/cloudstack/storage/datastore/adapter/ProviderAdapterDiskOffering.java#L37 

Added line #L37 was not covered by tests
}
if (hiddenDiskOffering.getState() != null) {
this.state = State.valueOf(hiddenDiskOffering.getState().toString());
Expand Down Expand Up @@ -166,7 +166,7 @@
}

enum DiskCacheMode {
NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");
NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough"), HYPERVISOR_DEFAULT("hypervisor_default");

Check warning on line 169 in plugins/storage/volume/adaptive/src/main/java/org/apache/cloudstack/storage/datastore/adapter/ProviderAdapterDiskOffering.java

View check run for this annotation

Codecov / codecov/patch

plugins/storage/volume/adaptive/src/main/java/org/apache/cloudstack/storage/datastore/adapter/ProviderAdapterDiskOffering.java#L169 

Added line #L169 was not covered by tests

private final String _diskCacheMode;

Expand All @@ -179,13 +179,15 @@
return _diskCacheMode;
}

public static DiskCacheMode getDiskCasehMode(String cacheMode) {
public static DiskCacheMode getDiskCacheMode(String cacheMode) {
if (cacheMode.equals(NONE._diskCacheMode)) {
return NONE;
} else if (cacheMode.equals(WRITEBACK._diskCacheMode)) {
return WRITEBACK;
} else if (cacheMode.equals(WRITETHROUGH._diskCacheMode)) {
return WRITETHROUGH;
} else if (cacheMode.equals(HYPERVISOR_DEFAULT._diskCacheMode)) {
return HYPERVISOR_DEFAULT;

Check warning on line 190 in plugins/storage/volume/adaptive/src/main/java/org/apache/cloudstack/storage/datastore/adapter/ProviderAdapterDiskOffering.java

View check run for this annotation

Codecov / codecov/patch

plugins/storage/volume/adaptive/src/main/java/org/apache/cloudstack/storage/datastore/adapter/ProviderAdapterDiskOffering.java#L190 

Added line #L190 was not covered by tests
} else {
throw new NotImplementedException("Invalid cache mode specified: " + cacheMode);
}
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8072,7 +8072,7 @@ protected void validateCacheMode(String cacheMode){
!Enums.getIfPresent(DiskOffering.DiskCacheMode.class,
cacheMode.toUpperCase()).isPresent()) {
throw new InvalidParameterValueException(String.format("Invalid cache mode (%s). Please specify one of the following " +
"valid cache mode parameters: none, writeback or writethrough", cacheMode));
"valid cache mode parameters: none, writeback, writethrough or hypervisor_default.", cacheMode));
}
}

Expand Down
3 changes: 2 additions & 1 deletion ui/public/locales/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1110,6 +1110,7 @@
"label.hourly": "Hourly",
"label.hypervisor": "Hypervisor",
"label.hypervisor.capabilities": "Hypervisor capabilities",
"label.hypervisor.default": "Hypervisor default",
"label.hypervisor.type": "Hypervisor type",
"label.hypervisors": "Hypervisors",
"label.hypervisorsnapshotreserve": "Hypervisor Snapshot reserve",
Expand Down Expand Up @@ -2613,7 +2614,7 @@
"label.windows": "Windows",
"label.with.snapshotid": "with Snapshot ID",
"label.write": "Write",
"label.writeback": "Write-back disk caching",
"label.writeback": "Write-back",
"label.writecachetype": "Write-cache Type",
"label.writeio": "Write (IO)",
"label.writethrough": "Write-through",
Expand Down
3 changes: 2 additions & 1 deletion ui/public/locales/pt_BR.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,7 @@
"label.hourly": "A cada hora",
"label.hypervisor": "Virtualizador",
"label.hypervisor.capabilities": "Recursos do virtualizador",
"label.hypervisor.default": "Padr\u00e3o do virtualizador",
"label.hypervisor.type": "Tipo do virtualizador",
"label.hypervisors": "Virtualizadores",
"label.hypervisorsnapshotreserve": "Reserva de snapshot do virtualizador",
Expand Down Expand Up @@ -1811,7 +1812,7 @@
"label.windows": "Windows",
"label.with.snapshotid": "com o ID da snapshot",
"label.write": "Escreva",
"label.writeback": "Cache de disco write-back",
"label.writeback": "Write-back",
"label.writecachetype": "Tipo do cache de escrita",
"label.writeio": "Escrita (IO)",
"label.writethrough": "Write-through",
Expand Down
3 changes: 3 additions & 0 deletions ui/src/views/offering/AddComputeOffering.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,9 @@
<a-radio-button value="writethrough">
{{ $t('label.writethrough') }}
</a-radio-button>
<a-radio-button value="hypervisor_default">
{{ $t('label.hypervisor.default') }}
</a-radio-button>
</a-radio-group>
</a-form-item>
<a-form-item :label="$t('label.qostype')" name="qostype" ref="qostype">
Expand Down
5 changes: 4 additions & 1 deletion ui/src/views/offering/AddDiskOffering.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,9 @@
<a-radio-button value="writethrough">
{{ $t('label.writethrough') }}
</a-radio-button>
<a-radio-button value="hypervisor_default">
{{ $t('label.hypervisor.default') }}
</a-radio-button>
</a-radio-group>
</a-form-item>
<a-form-item v-if="isAdmin() || isDomainAdminAllowedToInformTags" name="tags" ref="tags">
Expand Down Expand Up @@ -601,7 +604,7 @@ export default {
width: 80vw;

@media (min-width: 800px) {
width: 430px;
width: 480px;
}
}
</style>
Loading