From d549f3bdc8e70400c3c0330ab97afc9df5efc0ea Mon Sep 17 00:00:00 2001 From: Radu Todirica <50488048+radu-todirica@users.noreply.github.com> Date: Mon, 9 Mar 2020 14:17:21 +0200 Subject: [PATCH] Add cache mode param properly (#3925) --- .../apache/cloudstack/api/ApiConstants.java | 1 + .../admin/offering/CreateDiskOfferingCmd.java | 11 ++ .../offering/CreateServiceOfferingCmd.java | 11 ++ .../api/response/ServiceOfferingResponse.java | 7 ++ .../kvm/storage/KVMStorageProcessor.java | 11 +- .../query/dao/ServiceOfferingJoinDaoImpl.java | 1 + .../api/query/vo/ServiceOfferingJoinVO.java | 7 ++ .../ConfigurationManagerImpl.java | 31 +++++- test/integration/smoke/test_disk_offerings.py | 72 ++++++++++++ .../smoke/test_service_offerings.py | 104 ++++++++++++++++++ tools/marvin/marvin/lib/base.py | 10 +- ui/scripts/configuration.js | 56 +++++++++- 12 files changed, 311 insertions(+), 11 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java index 384293f4e08..ed7e39e3f92 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java @@ -781,6 +781,7 @@ public class ApiConstants { public static final String ROUTER_CHECK_TYPE = "checktype"; public static final String LAST_UPDATED = "lastupdated"; public static final String PERFORM_FRESH_CHECKS = "performfreshchecks"; + public static final String CACHE_MODE = "cachemode"; public static final String CONSOLE_END_POINT = "consoleendpoint"; public static final String EXTERNAL_LOAD_BALANCER_IP_ADDRESS = "externalloadbalanceripaddress"; diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java index 3ff8f69568c..f0ca5fb851a 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java @@ -144,6 +144,13 @@ public class CreateDiskOfferingCmd extends BaseCmd { description = "Hypervisor snapshot reserve space as a percent of a volume (for managed storage using Xen or VMware)") private Integer hypervisorSnapshotReserve; + @Parameter(name = ApiConstants.CACHE_MODE, + type = CommandType.STRING, + required = false, + description = "the cache mode to use for this disk offering. none, writeback or writethrough", + since = "4.14") + private String cacheMode; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -262,6 +269,10 @@ public class CreateDiskOfferingCmd extends BaseCmd { return hypervisorSnapshotReserve; } + public String getCacheMode() { + return cacheMode; + } + ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java index c30b43745c5..5015f7c51b8 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java @@ -178,6 +178,13 @@ public class CreateServiceOfferingCmd extends BaseCmd { since = "4.4") private Integer hypervisorSnapshotReserve; + @Parameter(name = ApiConstants.CACHE_MODE, + type = CommandType.STRING, + required = false, + description = "the cache mode to use for this disk offering. none, writeback or writethrough", + since = "4.14") + private String cacheMode; + // Introduce 4 new optional paramaters to work custom compute offerings @Parameter(name = ApiConstants.CUSTOMIZED, type = CommandType.BOOLEAN, @@ -377,6 +384,10 @@ public class CreateServiceOfferingCmd extends BaseCmd { return hypervisorSnapshotReserve; } + public String getCacheMode() { + return cacheMode; + } + /** * If customized parameter is true, then cpuNumber, memory and cpuSpeed must be null * Check if the optional params min/max CPU/Memory have been specified diff --git a/api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java index 41c8645029e..9d5e9ee5d7c 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java @@ -192,6 +192,10 @@ public class ServiceOfferingResponse extends BaseResponse { @Param(description = "is true if the offering is customized", since = "4.3.0") private Boolean isCustomized; + @SerializedName("cacheMode") + @Param(description = "the cache mode to use for this disk offering. none, writeback or writethrough", since = "4.14") + private String cacheMode; + public ServiceOfferingResponse() { } @@ -448,4 +452,7 @@ public class ServiceOfferingResponse extends BaseResponse { } + public void setCacheMode(String cacheMode) { + this.cacheMode = cacheMode; + } } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index 17503ffd49b..dab80c9dc19 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java @@ -1171,7 +1171,7 @@ public class KVMStorageProcessor implements StorageProcessor { final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength, final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength, final Long iopsReadRate, final Long iopsReadRateMax, final Long iopsReadRateMaxLength, - final Long iopsWriteRate, final Long iopsWriteRateMax, final Long iopsWriteRateMaxLength) throws LibvirtException, InternalErrorException { + final Long iopsWriteRate, final Long iopsWriteRateMax, final Long iopsWriteRateMaxLength, final String cacheMode) throws LibvirtException, InternalErrorException { List disks = null; Domain dm = null; DiskDef diskdef = null; @@ -1281,6 +1281,9 @@ public class KVMStorageProcessor implements StorageProcessor { if ((iopsWriteRateMaxLength != null) && (iopsWriteRateMaxLength > 0)) { diskdef.setIopsWriteRateMaxLength(iopsWriteRateMaxLength); } + if(cacheMode != null) { + diskdef.setCacheMode(DiskDef.DiskCacheMode.valueOf(cacheMode.toUpperCase())); + } } final String xml = diskdef.toString(); @@ -1305,12 +1308,13 @@ public class KVMStorageProcessor implements StorageProcessor { storagePoolMgr.connectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), vol.getPath(), disk.getDetails()); final KVMPhysicalDisk phyDisk = storagePoolMgr.getPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), vol.getPath()); + final String volCacheMode = vol.getCacheMode() == null ? null : vol.getCacheMode().toString(); attachOrDetachDisk(conn, true, vmName, phyDisk, disk.getDiskSeq().intValue(), serial, vol.getBytesReadRate(), vol.getBytesReadRateMax(), vol.getBytesReadRateMaxLength(), vol.getBytesWriteRate(), vol.getBytesWriteRateMax(), vol.getBytesWriteRateMaxLength(), vol.getIopsReadRate(), vol.getIopsReadRateMax(), vol.getIopsReadRateMaxLength(), - vol.getIopsWriteRate(), vol.getIopsWriteRateMax(), vol.getIopsWriteRateMaxLength()); + vol.getIopsWriteRate(), vol.getIopsWriteRateMax(), vol.getIopsWriteRateMaxLength(), volCacheMode); return new AttachAnswer(disk); } catch (final LibvirtException e) { @@ -1334,12 +1338,13 @@ public class KVMStorageProcessor implements StorageProcessor { final Connect conn = LibvirtConnection.getConnectionByVmName(vmName); final KVMPhysicalDisk phyDisk = storagePoolMgr.getPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), vol.getPath()); + final String volCacheMode = vol.getCacheMode() == null ? null : vol.getCacheMode().toString(); attachOrDetachDisk(conn, false, vmName, phyDisk, disk.getDiskSeq().intValue(), serial, vol.getBytesReadRate(), vol.getBytesReadRateMax(), vol.getBytesReadRateMaxLength(), vol.getBytesWriteRate(), vol.getBytesWriteRateMax(), vol.getBytesWriteRateMaxLength(), vol.getIopsReadRate(), vol.getIopsReadRateMax(), vol.getIopsReadRateMaxLength(), - vol.getIopsWriteRate(), vol.getIopsWriteRateMax(), vol.getIopsWriteRateMaxLength()); + vol.getIopsWriteRate(), vol.getIopsWriteRateMax(), vol.getIopsWriteRateMaxLength(), volCacheMode); storagePoolMgr.disconnectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), vol.getPath()); diff --git a/server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java b/server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java index 13980441471..add64155375 100644 --- a/server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java +++ b/server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java @@ -102,6 +102,7 @@ public class ServiceOfferingJoinDaoImpl extends GenericDaoBase filteredDomainIds = filterChildSubDomains(domainIds); @@ -2541,6 +2545,9 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati if (iopsWriteRateMaxLength != null && iopsWriteRateMaxLength > 0) { offering.setIopsWriteRateMaxLength(iopsWriteRateMaxLength); } + if(cacheMode != null) { + offering.setCacheMode(DiskOffering.DiskCacheMode.valueOf(cacheMode.toUpperCase())); + } if (hypervisorSnapshotReserve != null && hypervisorSnapshotReserve < 0) { throw new InvalidParameterValueException("If provided, Hypervisor Snapshot Reserve must be greater than or equal to 0."); @@ -2799,7 +2806,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati Long bytesWriteRate, Long bytesWriteRateMax, Long bytesWriteRateMaxLength, Long iopsReadRate, Long iopsReadRateMax, Long iopsReadRateMaxLength, Long iopsWriteRate, Long iopsWriteRateMax, Long iopsWriteRateMaxLength, - final Integer hypervisorSnapshotReserve) { + final Integer hypervisorSnapshotReserve, String cacheMode) { long diskSize = 0;// special case for custom disk offerings if (numGibibytes != null && numGibibytes <= 0) { throw new InvalidParameterValueException("Please specify a disk size of at least 1 Gb."); @@ -2905,6 +2912,9 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati if (iopsWriteRateMaxLength != null && iopsWriteRateMaxLength > 0) { newDiskOffering.setIopsWriteRateMaxLength(iopsWriteRateMaxLength); } + if (cacheMode != null) { + newDiskOffering.setCacheMode(DiskOffering.DiskCacheMode.valueOf(cacheMode.toUpperCase())); + } if (hypervisorSnapshotReserve != null && hypervisorSnapshotReserve < 0) { throw new InvalidParameterValueException("If provided, Hypervisor Snapshot Reserve must be greater than or equal to 0."); @@ -2971,6 +2981,9 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati throw new InvalidParameterValueException("Disksize is not allowed for a customized disk offering"); } + // check if cache_mode parameter is valid + validateCacheMode(cmd.getCacheMode()); + boolean localStorageRequired = false; final String storageType = cmd.getStorageType(); if (storageType != null) { @@ -2997,13 +3010,14 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati final Long iopsWriteRateMax = cmd.getIopsWriteRateMax(); final Long iopsWriteRateMaxLength = cmd.getIopsWriteRateMaxLength(); final Integer hypervisorSnapshotReserve = cmd.getHypervisorSnapshotReserve(); + final String cacheMode = cmd.getCacheMode(); final Long userId = CallContext.current().getCallingUserId(); return createDiskOffering(userId, domainIds, zoneIds, name, description, provisioningType, numGibibytes, tags, isCustomized, localStorageRequired, isDisplayOfferingEnabled, isCustomizedIops, minIops, maxIops, bytesReadRate, bytesReadRateMax, bytesReadRateMaxLength, bytesWriteRate, bytesWriteRateMax, bytesWriteRateMaxLength, iopsReadRate, iopsReadRateMax, iopsReadRateMaxLength, iopsWriteRate, iopsWriteRateMax, iopsWriteRateMaxLength, - hypervisorSnapshotReserve); + hypervisorSnapshotReserve, cacheMode); } @Override @@ -6267,6 +6281,15 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati return filteredDomainIds; } + protected void validateCacheMode(String cacheMode){ + if(cacheMode != null && + !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)); + } + } + public List getSecChecker() { return _secChecker; } diff --git a/test/integration/smoke/test_disk_offerings.py b/test/integration/smoke/test_disk_offerings.py index af7ba6a2ccc..d0d3433e96c 100644 --- a/test/integration/smoke/test_disk_offerings.py +++ b/test/integration/smoke/test_disk_offerings.py @@ -223,6 +223,78 @@ class TestCreateDiskOffering(cloudstackTestCase): ) return + @attr(tags=["advanced", "basic", "eip", "sg", "advancedns", "smoke"], required_hardware="false") + def test_06_create_disk_offering_with_cache_mode_type(self): + """Test to create disk offering with each one of the valid cache mode types : none, writeback and writethrough + + # Validate the following: + # 1. createDiskOfferings should return valid info for new offering + # 2. The Cloud Database contains the valid information + """ + cache_mode_types=["none", "writeback", "writethrough"] + for i in range(3): + disk_offering = DiskOffering.create( + self.apiclient, + self.services["disk_offering"], + cacheMode=cache_mode_types[i] + ) + self.cleanup.append(disk_offering) + + self.debug("Created Disk offering with valid cacheMode param with ID: %s" % disk_offering.id) + + list_disk_response = list_disk_offering( + self.apiclient, + id=disk_offering.id + ) + self.assertEqual( + isinstance(list_disk_response, list), + True, + "Check list response returns a valid list" + ) + self.assertNotEqual( + len(list_disk_response), + 0, + "Check Disk offering is created" + ) + disk_response = list_disk_response[0] + + self.assertEqual( + disk_response.displaytext, + self.services["disk_offering"]["displaytext"], + "Check server id in createServiceOffering" + ) + self.assertEqual( + disk_response.name, + self.services["disk_offering"]["name"], + "Check name in createServiceOffering" + ) + self.assertEqual( + disk_response.cacheMode, + cache_mode_types[i], + "Check cacheMode in createServiceOffering" + ) + + return + + @attr(tags=["advanced", "basic", "eip", "sg", "advancedns", "smoke"], required_hardware="false") + def test_07_create_disk_offering_with_invalid_cache_mode_type(self): + """Test to create disk offering with invalid cacheMode type + + # Validate the following: + # 1. createDiskOfferings should return valid info for new offering + # 2. The Cloud Database contains the valid information + """ + + with self.assertRaises(Exception): + disk_offering = DiskOffering.create( + self.apiclient, + self.services["disk_offering"], + cacheMode="invalid_cache_mode_type" + ) + + + return + class TestDiskOfferings(cloudstackTestCase): def setUp(self): diff --git a/test/integration/smoke/test_service_offerings.py b/test/integration/smoke/test_service_offerings.py index 61d83b90282..0e7f068a277 100644 --- a/test/integration/smoke/test_service_offerings.py +++ b/test/integration/smoke/test_service_offerings.py @@ -129,6 +129,7 @@ class TestCreateServiceOffering(cloudstackTestCase): "Check name in createServiceOffering" ) return + @attr( tags=[ "advanced", @@ -197,6 +198,109 @@ class TestCreateServiceOffering(cloudstackTestCase): return + @attr( + tags=[ + "advanced", + "advancedns", + "smoke", + "basic", + "eip", + "sg"], + required_hardware="false") + def test_03_create_service_offering_with_cache_mode_type(self): + """Test to create service offering with each one of the valid cache mode types : none, writeback and writethrough""" + + # Validate the following: + # 1. createServiceOfferings should return a valid information + # for newly created offering + # 2. The Cloud Database contains the valid information + + cache_mode_types=["none", "writeback", "writethrough"] + for i in range(3): + service_offering = ServiceOffering.create( + self.apiclient, + self.services["service_offerings"]["tiny"], + cacheMode=cache_mode_types[i] + ) + self.cleanup.append(service_offering) + + self.debug( + "Created service offering with ID: %s" % + service_offering.id) + + list_service_response = list_service_offering( + self.apiclient, + id=service_offering.id + ) + self.assertEqual( + isinstance(list_service_response, list), + True, + "Check list response returns a valid list" + ) + + self.assertNotEqual( + len(list_service_response), + 0, + "Check Service offering is created" + ) + + self.assertEqual( + list_service_response[0].cpunumber, + self.services["service_offerings"]["tiny"]["cpunumber"], + "Check server id in createServiceOffering" + ) + self.assertEqual( + list_service_response[0].cpuspeed, + self.services["service_offerings"]["tiny"]["cpuspeed"], + "Check cpuspeed in createServiceOffering" + ) + self.assertEqual( + list_service_response[0].displaytext, + self.services["service_offerings"]["tiny"]["displaytext"], + "Check server displaytext in createServiceOfferings" + ) + self.assertEqual( + list_service_response[0].memory, + self.services["service_offerings"]["tiny"]["memory"], + "Check memory in createServiceOffering" + ) + self.assertEqual( + list_service_response[0].name, + self.services["service_offerings"]["tiny"]["name"], + "Check name in createServiceOffering" + ) + self.assertEqual( + list_service_response[0].cacheMode, + cache_mode_types[i], + "Check cacheMode in createServiceOffering" + ) + return + + @attr( + tags=[ + "advanced", + "advancedns", + "smoke", + "basic", + "eip", + "sg"], + required_hardware="false") + def test_04_create_service_offering_with_invalid_cache_mode_type(self): + """Test to create service offering with invalid cache mode type""" + + # Validate the following: + # 1. createServiceOfferings should return a valid information + # for newly created offering + # 2. The Cloud Database contains the valid information + + with self.assertRaises(Exception): + service_offering = ServiceOffering.create( + self.apiclient, + self.services["service_offerings"]["tiny"], + cacheMode="invalid_cache_mode_type" + ) + return + class TestServiceOfferings(cloudstackTestCase): diff --git a/tools/marvin/marvin/lib/base.py b/tools/marvin/marvin/lib/base.py index fc64f2723f6..df38bb54a2b 100755 --- a/tools/marvin/marvin/lib/base.py +++ b/tools/marvin/marvin/lib/base.py @@ -2167,7 +2167,7 @@ class ServiceOffering: self.__dict__.update(items) @classmethod - def create(cls, apiclient, services, tags=None, domainid=None, **kwargs): + def create(cls, apiclient, services, tags=None, domainid=None, cacheMode=None, **kwargs): """Create Service offering""" cmd = createServiceOffering.createServiceOfferingCmd() cmd.cpunumber = services["cpunumber"] @@ -2220,6 +2220,9 @@ class ServiceOffering: if domainid: cmd.domainid = domainid + if cacheMode: + cmd.cacheMode = cacheMode + if tags: cmd.tags = tags elif "tags" in services: @@ -2253,7 +2256,7 @@ class DiskOffering: self.__dict__.update(items) @classmethod - def create(cls, apiclient, services, tags=None, custom=False, domainid=None, **kwargs): + def create(cls, apiclient, services, tags=None, custom=False, domainid=None, cacheMode=None, **kwargs): """Create Disk offering""" cmd = createDiskOffering.createDiskOfferingCmd() cmd.displaytext = services["displaytext"] @@ -2266,6 +2269,9 @@ class DiskOffering: if domainid: cmd.domainid = domainid + if cacheMode: + cmd.cacheMode = cacheMode + if tags: cmd.tags = tags elif "tags" in services: diff --git a/ui/scripts/configuration.js b/ui/scripts/configuration.js index 641fc549357..3d3783ddd4f 100644 --- a/ui/scripts/configuration.js +++ b/ui/scripts/configuration.js @@ -137,6 +137,28 @@ }); } }, + cacheMode: { + label: 'label.cache.mode', + docID: 'helpDiskOfferingCacheMode', + select: function (args) { + var items = []; + items.push({ + id: 'none', + description: 'No disk cache' + }); + items.push({ + id: 'writeback', + description: 'Write-back disk caching' + }); + items.push({ + id: "writethrough", + description: 'Write-through' + }); + args.response.success({ + data: items + }) + } + }, offeringType: { label: 'label.compute.offering.type', docID: 'helpComputeOfferingType', @@ -731,7 +753,8 @@ displaytext: args.data.description, storageType: args.data.storageType, provisioningType :args.data.provisioningType, - customized: !isFixedOfferingType + customized: !isFixedOfferingType, + cacheMode: args.data.cacheMode }; //custom fields (begin) @@ -1221,6 +1244,9 @@ provisioningtype: { label: 'label.disk.provisioningtype' }, + cacheMode: { + label: 'label.cache.mode' + }, cpunumber: { label: 'label.num.cpu.cores' }, @@ -1481,6 +1507,28 @@ }); } }, + cacheMode: { + label: 'label.cache.mode', + docID: 'helpDiskOfferingCacheMode', + select: function(args) { + var items = []; + items.push({ + id: 'none', + description: 'No disk cache' + }); + items.push({ + id: 'writeback', + description: 'Write-back disk caching' + }); + items.push({ + id: 'writethrough', + description: 'Write-through disk caching' + }); + args.response.success({ + data: items + }) + } + }, cpuNumber: { label: 'label.num.cpu.cores', docID: 'helpSystemOfferingCPUCores', @@ -1694,7 +1742,8 @@ provisioningType: args.data.provisioningType, cpuNumber: args.data.cpuNumber, cpuSpeed: args.data.cpuSpeed, - memory: args.data.memory + memory: args.data.memory, + cacheMode: args.data.cacheMode }; if (args.data.networkRate != null && args.data.networkRate.length > 0) { @@ -1920,6 +1969,9 @@ provisioningtype: { label: 'label.disk.provisioningtype' }, + cacheMode: { + label: 'label.cache.mode' + }, cpunumber: { label: 'label.num.cpu.cores' },