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
DaanHoogland 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 1 commit
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
Next Next commit
Add disk offering Hypervisor default cache mode
  • Loading branch information
@hsato03
Henrique Sato authored and hsato03 committed Jan 27, 2025
commit e6d168029395fd942fba6ad2257134e3df36f850
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 @@ enum State {
State getState();

enum DiskCacheMode {
NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");
HYPERVISOR_DEFAULT("hypervisor_default"), NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");
Comment thread
hsato03 marked this conversation as resolved.
Outdated

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 @@ public VolumeObjectTO(VolumeInfo volume) {
iopsWriteRate = volume.getIopsWriteRate();
iopsWriteRateMax = volume.getIopsWriteRateMax();
iopsWriteRateMaxLength = volume.getIopsWriteRateMaxLength();
cacheMode = volume.getCacheMode();
hypervisorType = volume.getHypervisorType();
setCacheMode(volume.getCacheMode());
setDeviceId(volume.getDeviceId());
this.migrationOptions = volume.getMigrationOptions();
this.directDownload = volume.isDirectDownload();
Expand Down Expand Up @@ -337,6 +338,10 @@ public void setDeviceId(Long deviceId) {
}

public void setCacheMode(DiskCacheMode cacheMode) {
if (DiskCacheMode.HYPERVISOR_DEFAULT.equals(cacheMode) && !Hypervisor.HypervisorType.KVM.equals(hypervisorType)) {
this.cacheMode = DiskOffering.DiskCacheMode.NONE;
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member 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;
}
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 @@ -31,3 +31,6 @@ SELECT uuid(), role_id, 'quotaCreditsList', permission, sort_order
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"');
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 @@ -706,7 +706,7 @@ public String toString() {
}

public enum DiskCacheMode {
NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");
HYPERVISOR_DEFAULT("default"), NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");
Comment thread
hsato03 marked this conversation as resolved.
Outdated
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 @@ public ProviderAdapterDiskOffering(DiskOffering hiddenDiskOffering) {
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());
}
if (hiddenDiskOffering.getState() != null) {
this.state = State.valueOf(hiddenDiskOffering.getState().toString());
Expand Down Expand Up @@ -166,7 +166,7 @@ enum State {
}

enum DiskCacheMode {
NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");
HYPERVISOR_DEFAULT("hypervisor_default"), NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");
Comment thread
JoaoJandre marked this conversation as resolved.
Outdated

private final String _diskCacheMode;

Expand All @@ -179,13 +179,15 @@ public String toString() {
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;
} 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 @@ -8061,7 +8061,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 @@ -1101,6 +1101,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 @@ -2596,7 +2597,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 @@ -1812,7 +1813,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>