Skip to content
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

Add Hypervisor default as cache mode for disk offerings #10282

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add Hypervisor default as cache mode for disk offerings #10282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 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
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");
HYPERVISOR_DEFAULT("hypervisor_default"), NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Although there shouldn't be any issue with changing the order of the enum. I would put the new value at the end of the order.


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;
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
4 changes: 3 additions & 1 deletion 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 @@ -32,5 +32,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');

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`');
-- 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");
HYPERVISOR_DEFAULT("default"), NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

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");
HYPERVISOR_DEFAULT("hypervisor_default"), NONE("none"), WRITEBACK("writeback"), WRITETHROUGH("writethrough");

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
Comment on lines 168 to +169
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have this enum multiple times? Maybe we could extract it to a common place?


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 @@ -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 @@ -1103,6 +1103,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 @@ -2601,7 +2602,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>
Loading