You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/03/03 15:40:43 UTC
[GitHub] [cloudstack] radu-todirica opened a new pull request #3925: Add
cache mode param properly
radu-todirica opened a new pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925
## Description
<!--- Describe your changes in detail -->
Right now for the createDiskOffering indicates that the cacheMode parameter comes back in the response but there is no equivalent parameter in the request.
The following changes have been added:
1. "cacheMode" field has been added to the createServiceOffering endpoint
2. "cacheMode" field has been added to the createDiskOffering endpoint
3. "cacheMode" field will be returned in the listServiceOfferings and listDiskOfferings endpoints.
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## Screenshots (if appropriate):
## How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
Wrote a python test for testing the creation of service offering with each one of the valid cache mode types : none, writeback and writethrough.
Wrote a python test for testing the creation of service offering with invalid cache mode type.
[test_service_offerings_results.txt](https://github.com/myENA/cloudstack/files/4252123/test_service_offerings_results.txt)
[test_service_offerings_runinfo.txt](https://github.com/myENA/cloudstack/files/4252124/test_service_offerings_runinfo.txt)
Wrote a python test for testing the creation of disk offering with each one of the valid cache mode types : none, writeback and writethrough.
Wrote a python test for testing the creation of disk offering with invalid cache mode type.
[test_disk_offerings_runinfo.txt](https://github.com/myENA/cloudstack/files/4252121/test_disk_offerings_runinfo.txt)
[test_disk_offerings_results.txt](https://github.com/myENA/cloudstack/files/4252122/test_disk_offerings_results.txt)
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387803550
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java
##########
@@ -144,6 +144,9 @@
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")
Review comment:
@radu-todirica why no since attribute here? no biggy, just wondering if you forgot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387864023
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java
##########
@@ -178,6 +178,13 @@
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.13")
Review comment:
@DaanHoogland this "since" should be 4.14, right? Is it going to be merged into branch 4.13 instead of master?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on issue #3925: Add cache
mode param properly
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on issue #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#issuecomment-595736973
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #3925: Add cache mode
param properly
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#issuecomment-595751520
@andrijapanicsb do you have head space for this, please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #3925: Add cache mode
param properly
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#issuecomment-595751763
@GabrielBrascher did you test?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] RodrigoDLopez edited a comment on issue #3925: Add
cache mode param properly
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez edited a comment on issue #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#issuecomment-596489974
Hi @radu-todirica , nice feature.
I tested it manually using cloudmonkey.
Tests consist of:
1. create disk offering
2. list serviceOffering
```
{
"cacheMode": "writeback",
"displaytext": "test cache write-back",
"id": "f511131c-9f9d-4e3d-ab64-51795654cd2d",
},
{
"cacheMode": "writethrough",
"displaytext": "test cache write trough",
"id": "0b981c95-74b4-4fe9-b996-6c9fcdbf077d",
}
```
3. create VMs
4. list VM instances and checked if the correct seviceOffering id is present
Instance with writeback cache mode:
```
{
"serviceofferingid": "f511131c-9f9d-4e3d-ab64-51795654cd2d",
"serviceofferingname": "test cache write-back",
}
```
Instance with writetrough cache mode:
```
{
"serviceofferingid": "0b981c95-74b4-4fe9-b996-6c9fcdbf077d",
"serviceofferingname": "test cache write-trough",
}
```
All good with cache mode for serviceOffering.
5. verify diskOffering
```
{
"cacheMode": "writethrough",
"displaytext": "test cache write trough",
"id": "5f6ff1c3-e0bb-4c6a-a7e9-5863ab22c8c3",
},
{
"cacheMode": "writeback",
"displaytext": "test cache write back",
"id": "a04abd52-4b0c-4aff-84d2-c1856705db1b",
}
```
6. create some Vms and check the virsh dumpxml
6.1 Dump of a VM using cache mode write-trough:
```
{
<disk type='file' device='disk'>
<driver name='qemu' type='qcow2' cache='writethrough'/>
}
```
More info about diskOffering for this instance with cmk:
```
{
"diskofferingid": "a04abd52-4b0c-4aff-84d2-c1856705db1b",
"diskofferingname": "test cache write back",
}
```
6.2 create a instance with writeback cache mode:
```
{
<disk type='file' device='disk'>
<driver name='qemu' type='qcow2' cache='writeback'/>
}
{
"diskofferingid": "5f6ff1c3-e0bb-4c6a-a7e9-5863ab22c8c3",
"diskofferingname": "teste cache",
}
```
LGMT based on manual tests, nice feature, and nice code too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387778031
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java
##########
@@ -178,6 +178,13 @@
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.13")
Review comment:
`since = "4.14"` if merged in time for 4.14 releasing
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387507238
##########
File path: test/integration/smoke/test_disk_offerings.py
##########
@@ -90,9 +90,81 @@ def test_01_create_disk_offering(self):
)
return
+ @attr(tags=["advanced", "basic", "eip", "sg", "advancedns", "smoke"], required_hardware="false")
+ def test_02_create_disk_offering_with_cache_mode_type(self):
Review comment:
Why this renumbering/renaming? It can break statistic analysis of test breakage or comparison. Why not add them to the end with new numbers?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387845753
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2936,6 +2948,11 @@ public DiskOffering createDiskOffering(final CreateDiskOfferingCmd cmd) {
throw new InvalidParameterValueException("Disksize is not allowed for a customized disk offering");
}
+ // check if valid cache_mode parameter
+ if(isCacheModeInvalid(cmd.getCacheMode())){
+ throw new InvalidParameterValueException("Please specify a valid cache mode parameter");
Review comment:
Can you please add more information on the log? Example: `String.format("Invalid cache mode (%s), specify a valid cache mode parameter", cmd.getCacheMode())`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387864023
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java
##########
@@ -178,6 +178,13 @@
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.13")
Review comment:
@DaanHoogland this "since" should be 4.14, right? Is it going to be merged into branch 4.13 instead master?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387498637
##########
File path: api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java
##########
@@ -192,6 +192,10 @@
@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.4")
Review comment:
"since" on the return @Param as attribute but not on the request @Parameter in the API. Probably C&P as i don't think you are planning this for the 4.4 branch! please adjust and expand (or remove)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387804255
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2318,6 +2319,11 @@ public ServiceOffering createServiceOffering(final CreateServiceOfferingCmd cmd)
}
}
+ // check if valid cache_mode parameter
+ if(isCacheModeInvalid(cmd.getCacheMode())){
+ throw new InvalidParameterValueException("Please specify a valid cache mode parameter");
+ }
+
Review comment:
I think it is better to call the method `validateCachMode(String cacheMode)` and include this code as it is duplicated below in lines 2951 - 2955:
```java
void validateCachMode(String cacheMode){
if( cacheMode != null &&
!Enums.getIfPresent(DiskOffering.DiskCacheMode.class,
cacheMode.toUpperCase()).isPresent() {
throw new InvalidParameterValueException("Please specify a valid cache mode parameter");
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland merged pull request #3925: Add cache mode
param properly
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] RodrigoDLopez commented on issue #3925: Add cache mode
param properly
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on issue #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#issuecomment-596489974
Hi @radu-todirica , nice feature.
I tested it manually using cloudmonkey.
Tests consist of:
1. create disk offering
2. list serviceOffering
{
"cacheMode": "writeback",
"displaytext": "test cache write-back",
"id": "f511131c-9f9d-4e3d-ab64-51795654cd2d",
},
{
"cacheMode": "writethrough",
"displaytext": "test cache write trough",
"id": "0b981c95-74b4-4fe9-b996-6c9fcdbf077d",
}
3. create VMs
4. list VM instances and checked if the correct seviceOffering id is present
Instance with writeback cache mode:
{
"serviceofferingid": "f511131c-9f9d-4e3d-ab64-51795654cd2d",
"serviceofferingname": "test cache write-back",
}
Instance with writetrough cache mode:
{
"serviceofferingid": "0b981c95-74b4-4fe9-b996-6c9fcdbf077d",
"serviceofferingname": "test cache write-trough",
}
All good with cache mode for serviceOffering.
5. verify diskOffering
{
"cacheMode": "writethrough",
"displaytext": "test cache write trough",
"id": "5f6ff1c3-e0bb-4c6a-a7e9-5863ab22c8c3",
},
{
"cacheMode": "writeback",
"displaytext": "test cache write back",
"id": "a04abd52-4b0c-4aff-84d2-c1856705db1b",
}
6. create some Vms and check the virsh dumpxml
6.1 Dump of a VM using cache mode write-trough:
{
<disk type='file' device='disk'>
<driver name='qemu' type='qcow2' cache='writethrough'/>
}
More info about diskOffering for this instance with cmk:
{
"diskofferingid": "a04abd52-4b0c-4aff-84d2-c1856705db1b",
"diskofferingname": "test cache write back",
}
6.2 create a instance with writeback cache mode:
{
<disk type='file' device='disk'>
<driver name='qemu' type='qcow2' cache='writeback'/>
}
{
"diskofferingid": "5f6ff1c3-e0bb-4c6a-a7e9-5863ab22c8c3",
"diskofferingname": "teste cache",
}
LGMT based on manual tests, nice feature, and nice code too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #3925: Add cache mode
param properly
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#issuecomment-595737266
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] wido commented on issue #3925: Add cache mode param
properly
Posted by GitBox <gi...@apache.org>.
wido commented on issue #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#issuecomment-594420255
@GabrielBrascher Can you take a look at this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #3925: Add cache mode
param properly
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#issuecomment-595745992
Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1021
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387778991
##########
File path: api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java
##########
@@ -192,6 +192,10 @@
@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.13.0")
Review comment:
Change to `4.14`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] radu-todirica commented on issue #3925: Add cache mode
param properly
Posted by GitBox <gi...@apache.org>.
radu-todirica commented on issue #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#issuecomment-594624679
Did the changes. @DaanHoogland please take a look.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387804507
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2936,6 +2948,11 @@ public DiskOffering createDiskOffering(final CreateDiskOfferingCmd cmd) {
throw new InvalidParameterValueException("Disksize is not allowed for a customized disk offering");
}
+ // check if valid cache_mode parameter
+ if(isCacheModeInvalid(cmd.getCacheMode())){
+ throw new InvalidParameterValueException("Please specify a valid cache mode parameter");
+ }
+
Review comment:
duplicate of 2322 - 2326
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on issue #3925: Add cache
mode param properly
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on issue #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#issuecomment-595753294
@DaanHoogland not yet, just code review. I am planning to do a few manual tests next week, I can take a look at this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387500896
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2936,6 +2948,11 @@ public DiskOffering createDiskOffering(final CreateDiskOfferingCmd cmd) {
throw new InvalidParameterValueException("Disksize is not allowed for a customized disk offering");
}
+ // check if valid cache_mode parameter
+ if(cmd.getCacheMode() != null && !Enums.getIfPresent(DiskOffering.DiskCacheMode.class, cmd.getCacheMode().toUpperCase()).isPresent()){
+ throw new InvalidParameterValueException("Please specify a valid cache mode parameter");
+ }
+
Review comment:
can you put this in its own method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3925: Add cache mode param properly
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3925: Add cache mode param properly
URL: https://github.com/apache/cloudstack/pull/3925#discussion_r387507603
##########
File path: test/integration/smoke/test_service_offerings.py
##########
@@ -129,6 +129,110 @@ def test_01_create_service_offering(self):
"Check name in createServiceOffering"
)
return
+
+ @attr(
+ tags=[
+ "advanced",
+ "advancedns",
+ "smoke",
+ "basic",
+ "eip",
+ "sg"],
+ required_hardware="false")
+ def test_02_create_service_offering_with_cache_mode_type(self):
Review comment:
again, why not add as test_03 and test_04?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services