Moti Asayag has posted comments on this change.
Change subject: core: Adds custom properties to VmDevice
......................................................................
Patch Set 11: (27 inline comments)
Nice work Martin and thanks for the comprehensive test which settle the entire
thing down.
I've raised concern about the non-deterministic order of the custom properties
with the VmDevice class:
There is a chance a device will have properties: a=1,b=2 after saving it to the
DB and reloading it will be b=2, a=1
Comparing the two objects will show they aren't equal despite they suppose to
be the same.
As i see it, the correct way (IMO) is keeping the properties in map and not as
a string.
I'd like to hear other opinions as well.
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java
Line 49: .addValue("is_managed", entity.getIsManaged())
Line 50: .addValue("is_plugged", entity.getIsPlugged())
Line 51: .addValue("is_readonly", entity.getIsReadOnly())
Line 52: .addValue("alias", entity.getAlias())
Line 53: .addValue("custom_properties",
entity.getCustomProperties());
please note that eventually, this is the only place where translation into
String of the properties should happen. In all other places in the application,
the custom properties should be managed as a Map<String, String> rather having
the parsing logic duplicated in each component (UI, Rest and engine)
Line 54: }
Line 55:
Line 56: @Override
Line 57: protected RowMapper<VmDevice> createEntityRowMapper() {
Line 150: vmDevice.setIsManaged(rs.getBoolean("is_managed"));
Line 151: vmDevice.setIsPlugged(rs.getBoolean("is_plugged"));
Line 152: vmDevice.setIsReadOnly(rs.getBoolean("is_readonly"));
Line 153: vmDevice.setAlias(rs.getString("alias"));
Line 154:
vmDevice.setCustomProperties(rs.getString("custom_properties"));
and here they should be translated into Map<String, String> from string.
But as said above, could be done in the future.
Line 155: return vmDevice;
Line 156: }
Line 157: }
Line 158:
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 174: ACTION_TYPE_FAILED_MIGRATION_TO_SAME_HOST=Cannot ${action} ${type}.
source and destination is the same.
Line 175: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_SYNTAX=Cannot
${action} ${type} if custom properties are in invalid format. Please check the
input.
Line 176: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_KEYS=Cannot
${action} ${type} if some of the specified custom properties are not configured
by the system. The keys are: ${MissingKeys}
Line 177: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_VALUES=Cannot
${action} ${type} if some of the specified custom properties have illegal
values. The keys are: ${WrongValueKeys}
Line 178: ACTION_TYPE_FAILED_CUSTOM_PROPERTIES_NOT_SUPPORTED_IN_VERSION=Cannot
${action} ${type} custom properties are not supported in version:
${NotSupportedInVersion}
please add a dot after ${type} and start custom with an upper case:
ACTION_TYPE_FAILED_CUSTOM_PROPERTIES_NOT_SUPPORTED_IN_VERSION=Cannot
${action} ${type}. Custom properties are not support
ed in version: ${NotSupportedInVersion}.
Line 179: ACTION_TYPE_FAILED_INVALID_DEVICE_TYPE_FOR_CUSTOM_PROPERTIES=Cannot
${action} ${type} custom properties are not supported for device type:
${InvalidDeviceType}
Line 180: ACTION_TYPE_FAILED_VDS_VM_CLUSTER=Cannot ${action} ${type}. There are
no available running Hosts in the Host Cluster.
Line 181: ACTION_TYPE_FAILED_VDS_VM_MEMORY=Cannot ${action} ${type}. There are
no available running Hosts with sufficient memory in VM's Cluster .
Line 182: ACTION_TYPE_FAILED_VDS_VM_CPUS=Cannot ${action} ${type}. There are no
available running Hosts with enough cores in VM's Cluster .
Line 175: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_SYNTAX=Cannot
${action} ${type} if custom properties are in invalid format. Please check the
input.
Line 176: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_KEYS=Cannot
${action} ${type} if some of the specified custom properties are not configured
by the system. The keys are: ${MissingKeys}
Line 177: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_VALUES=Cannot
${action} ${type} if some of the specified custom properties have illegal
values. The keys are: ${WrongValueKeys}
Line 178: ACTION_TYPE_FAILED_CUSTOM_PROPERTIES_NOT_SUPPORTED_IN_VERSION=Cannot
${action} ${type} custom properties are not supported in version:
${NotSupportedInVersion}
Line 179: ACTION_TYPE_FAILED_INVALID_DEVICE_TYPE_FOR_CUSTOM_PROPERTIES=Cannot
${action} ${type} custom properties are not supported for device type:
${InvalidDeviceType}
same as above:
ACTION_TYPE_FAILED_INVALID_DEVICE_TYPE_FOR_CUSTOM_PROPERTIES=Cannot ${action}
${type}. Custom properties are not supporte
d for device type: ${InvalidDeviceType}.
Line 180: ACTION_TYPE_FAILED_VDS_VM_CLUSTER=Cannot ${action} ${type}. There are
no available running Hosts in the Host Cluster.
Line 181: ACTION_TYPE_FAILED_VDS_VM_MEMORY=Cannot ${action} ${type}. There are
no available running Hosts with sufficient memory in VM's Cluster .
Line 182: ACTION_TYPE_FAILED_VDS_VM_CPUS=Cannot ${action} ${type}. There are no
available running Hosts with enough cores in VM's Cluster .
Line 183: ACTION_TYPE_FAILED_VDS_VM_NETWORKS=Cannot ${action} ${type}. There
are no available running Hosts with all the networks used by the VM.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/customprop/CustomPropertiesUtils.java
Line 115: for (String property : propertiesStrs) {
Line 116:
Line 117: Pattern pattern = null;
Line 118: String[] propertyParts =
property.split(KEY_VALUE_DELIMETER, 2);
Line 119: if (propertyParts.length == 1) { // there is no
value(regex) for the property - we assume
please move this comment above the if statement as we don't have a convention
of adding comment at the end of the code line
Line 120: // in that case that any value is allowed except for
the properties delimeter
Line 121: pattern = VALUE_PATTERN;
Line 122: } else {
Line 123: pattern = Pattern.compile(propertyParts[1]);
Line 167: return Collections.emptyList();
Line 168: }
Line 169: // Syntax error is one of the most severe errors, and should
be returned without checking the rest of the errors
Line 170: if (errorsList.size() == 1 && errorsList.get(0).getReason()
== ValidationFailureReason.SYNTAX_ERROR) {
Line 171: return Arrays.asList(syntaxErrorMessage);
why not simplify it by squashing the two if statements into the same if
statement in the following manner:
if (errorsList.size() == 1 &&
isSevereErrorDetected(errorsList.get(0).getReason())) {
return Arrays.asList(syntaxErrorMessage);
}
boolean isSevereErrorDetected(ValidationFailureReason failure) {
return failure == ValidationFailureReason.SYNTAX_ERROR
|| failure == ValidationFailureReason.UNSUPPORTED_VERSION;
}
Line 172: }
Line 173: // Unsupported version is one of the most severe errors, and
should be returned without checking the rest of the
Line 174: // errors
Line 175: if (errorsList.size() == 1 && errorsList.get(0).getReason()
== ValidationFailureReason.UNSUPPORTED_VERSION) {
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/customprop/DevicePropertiesUtils.java
Line 113: // init supported device types
Line 114: supportedDeviceTypes = new ArrayList<>();
Line 115: for (VmDeviceGeneralType t : VmDeviceGeneralType.values()) {
Line 116: if (t != VmDeviceGeneralType.UNKNOWN) {
Line 117: // custom properties cannot be set for UNKNOWN
please move the comment line above the if statement
Line 118: supportedDeviceTypes.add(t);
Line 119: }
Line 120: }
Line 121:
Line 206: deviceProperties.get(version).put(type,
props);
Line 207: }
Line 208: }
Line 209: }
Line 210: } catch (Throwable ex) {
Would you mind replacing Throwable with Exception since it will treat Error
exceptions on the same manner while it should be reflected beyond of the scope
of this.
Line 211: throw new InitializationException(ex);
Line 212: }
Line 213: }
Line 214:
Line 231: if (!FeatureSupported.deviceCustomProperties(version)) {
Line 232: return Collections.emptySet();
Line 233: }
Line 234:
Line 235: Set<VmDeviceGeneralType> types;
a matter of taste - types can be avoided for the sake of return statements from
the if-else block.
Line 236: EnumMap<VmDeviceGeneralType, Map<String, Pattern>> map =
deviceProperties.get(version);
Line 237: if (map.isEmpty()) {
Line 238: // no device type has any properities
Line 239: types = Collections.emptySet();
Line 308: * @return map containing all device custom properties
Line 309: */
Line 310: public Map<String, String> convertProperties(Version version,
VmDeviceGeneralType type, String properties) {
Line 311: Map<String, String> map = new HashMap<String, String>();
Line 312: if (!StringUtils.isEmpty(properties)) {
please use StringUtils.isNotEmpty instead negating the condition.
Line 313: populateProperties(version, type, properties, map);
Line 314: }
Line 315: return map;
Line 316: }
Line 354: return sb.toString();
Line 355: }
Line 356:
Line 357: /**
Line 358: * Parses a string of device properties and stores them to a map
of key,value
I think the end of this statement "of key,value" is implied by noting it is a
map
Line 359: *
Line 360: * @param version
Line 361: * version of the cluster that the VM containing the
device is in
Line 362: * @param type
Line 371: VmDeviceGeneralType type,
Line 372: String properties,
Line 373: Map<String, String> propertiesMap) {
Line 374:
Line 375: List<ValidationError> results = new
ArrayList<CustomPropertiesUtils.ValidationError>();
there are 3 levels of nested if. you can flatten it by early return, and avoid
the redundant else block and the third level if:
if (!FeatureSupported.deviceCustomProperties(version)) {
results.add(unsupportedVersionValidationError);
return results;
}
if (!supportedDeviceTypes.contains(type) || StringUtils.isEmpty(properties)) {
return results;
}
Set<ValidationError> errorsSet = new
HashSet<CustomPropertiesUtils.ValidationError>();
String keyValuePairs[] = SEMICOLON_PATTERN.split(properties);
...
results.addAll(errorsSet);
return results;
}
Line 376: if (FeatureSupported.deviceCustomProperties(version)) {
Line 377: if (supportedDeviceTypes.contains(type)) {
Line 378: if (!StringUtils.isEmpty(properties)) {
Line 379: Set<ValidationError> errorsSet = new
HashSet<CustomPropertiesUtils.ValidationError>();
Line 383: String[] pairParts =
keyValuePairStr.split(KEY_VALUE_DELIMETER, 2);
Line 384: String key = pairParts[0];
Line 385: String value = pairParts[1];
Line 386: if (propertiesMap.containsKey(key)) {
Line 387: errorsSet.add(new
ValidationError(ValidationFailureReason.DUPLICATE_KEY, key));
please use proper system logging than the standard output or delete this line
completely.
Line 388: System.out.println("Duplicate key");
Line 389: continue;
Line 390: }
Line 391: if
(!deviceProperties.get(version).get(type).containsKey(key)) {
Line 389: continue;
Line 390: }
Line 391: if
(!deviceProperties.get(version).get(type).containsKey(key)) {
Line 392: errorsSet.add(new
ValidationError(ValidationFailureReason.KEY_DOES_NOT_EXIST, key));
Line 393: System.out.println("Key not found");
same as above
Line 394: continue;
Line 395: }
Line 396:
Line 397: if
(!deviceProperties.get(version).get(type).get(key).matcher(value).matches()) {
Line 395: }
Line 396:
Line 397: if
(!deviceProperties.get(version).get(type).get(key).matcher(value).matches()) {
Line 398: errorsSet.add(new
ValidationError(ValidationFailureReason.INCORRECT_VALUE, key));
Line 399: System.out.println("Incorrect value");
same as above
Line 400: continue;
Line 401: }
Line 402: propertiesMap.put(key, value);
Line 403: }
Line 442: // 1. Check if the key exists in device properties key set
Line 443: // 2. Check if the value of the key is valid
Line 444: // In case either 1 or 2 fails, add an error to the errors
list
Line 445: Map<String, String> map = new HashMap<String, String>();
Line 446: List<ValidationError> result = populateProperties(version,
type, properties, map);
perhaps populateProperties could be divided into 2 methods: only for verifying
the input and the second to store them ?
Line 447: return result;
Line 448: }
Line 449:
Line 450: /**
Line 443: // 2. Check if the value of the key is valid
Line 444: // In case either 1 or 2 fails, add an error to the errors
list
Line 445: Map<String, String> map = new HashMap<String, String>();
Line 446: List<ValidationError> result = populateProperties(version,
type, properties, map);
Line 447: return result;
the result could be replaced with inline.
Line 448: }
Line 449:
Line 450: /**
Line 451: * Validates device custom properties definition
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java
Line 237: } else {
Line 238: vmDevice.setIsReadOnly(Boolean.FALSE);
Line 239: }
Line 240: if (node.SelectSingleNode(OvfProperties.VMD_CUSTOM_PROP,
_xmlNS) != null
Line 241: &&
!StringUtils.isEmpty(node.SelectSingleNode(OvfProperties.VMD_CUSTOM_PROP,
_xmlNS).InnerText)) {
You can use StringUtils.isNotEmpty instead negating StringUtils.isEmpty
Line 242:
vmDevice.setCustomProperties(String.valueOf(node.SelectSingleNode(OvfProperties.VMD_CUSTOM_PROP,
_xmlNS).InnerText));
Line 243: } else {
Line 244: vmDevice.setCustomProperties(null);
Line 245: }
....................................................
File
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/customprop/DevicePropertiesUtilsTest.java
Line 72: .getSupportedClusterLevels();
Line 73: try {
Line 74: mockedUtils.init();
Line 75: } catch (InitializationException e) {
Line 76: e.printStackTrace();
please use a proper logger instead of standard output or remove this line
completely
Line 77: }
Line 78: return mockedUtils;
Line 79: }
Line 80:
Line 86: DevicePropertiesUtils utils = mockDevicePropertiesUtils();
Line 87: List<ValidationError> errors =
Line 88: utils.validateProperties(Version.v3_2,
VmDeviceGeneralType.DISK, "bootable=true");
Line 89:
Line 90: assertFalse(errors.isEmpty());
this two lines are repeated for each failed test, please extract them into a
method which receives the failure reason and errors list, and use it in the
negative tests where possible i.e. :
private void validateFailure(List<ValidationError> errors,
ValidationFailureReason reason) {
assertFalse(errors.isEmpty());
assertEquals(reason, errors.get(0).getReason());
}
Line 91: assertEquals(ValidationFailureReason.UNSUPPORTED_VERSION,
errors.get(0).getReason());
Line 92: }
Line 93:
Line 94: /**
Line 210:
Line 211: /**
Line 212: * Tries to validate custom device properties specification with
invalid syntax
Line 213: */
Line 214: @Test
suggestion: just for easing the reading, perhaps add the violation to the
method name?
e.g. customDevPropSpecWithInvalidSyntaxInvalidTypeKey
Line 215: public void customDevPropSpecWithInvalidSyntax1() {
Line 216: String customDevPropSpec =
"{typ=disk;prop={bootable=^(true|false)$}}";
Line 217: DevicePropertiesUtils utils =
DevicePropertiesUtils.getInstance();
Line 218:
Line 221:
Line 222: /**
Line 223: * Tries to validate custom device properties specification with
invalid syntax
Line 224: */
Line 225: @Test
e.g. customDevPropSpecWithInvalidSyntaxIncompleteBrackets
Line 226: public void customDevPropSpecWithInvalidSyntax2() {
Line 227: String customDevPropSpec =
"{type=disk;prop={bootable=^(true|false)$}";
Line 228: DevicePropertiesUtils utils =
DevicePropertiesUtils.getInstance();
Line 229:
Line 232:
Line 233: /**
Line 234: * Tries to validate custom device properties specification with
invalid syntax
Line 235: */
Line 236: @Test
e.g. customDevPropSpecWithInvalidSyntaxMissingSemicolons
Line 237: public void customDevPropSpecWithInvalidSyntax3() {
Line 238: String customDevPropSpec =
"{type=disk,prop={bootable=^(true|false)$}}";
Line 239: DevicePropertiesUtils utils =
DevicePropertiesUtils.getInstance();
Line 240:
Line 471: map.put("hotplug", "0");
Line 472:
Line 473: String value = utils.convertProperties(Version.v3_3,
VmDeviceGeneralType.INTERFACE, map);
Line 474:
Line 475: // order of properties in string is unknown due to unknown
map key ordering
ain't this raise concern about the comparison of two VmDevice objects with the
same entities but with custom properties as noted below will fail in compare ?
I think the order of the properties should be deterministic to avoid it.
If we had the custom properties kept as Map<String, String>, we shouldn't be
concern about it.
Line 476: assertTrue("speed=1024;duplex=half".equals(value) ||
"duplex=half;speed=1024".equals(value));
Line 477: }
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java
Line 462:
Line 463: @DefaultStringValue("Cannot ${action} ${type} if some of the
specified custom properties have illegal values. The keys are:
${WrongValueKeys}")
Line 464: String
ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_VALUES();
Line 465:
Line 466: @DefaultStringValue("Cannot ${action} ${type} custom properties
are not supported in version: ${NotSupportedInVersion}")
please see comments about the message in
backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 467: String
ACTION_TYPE_FAILED_CUSTOM_PROPERTIES_NOT_SUPPORTED_IN_VERSION();
Line 468:
Line 469: @DefaultStringValue("Cannot ${action} ${type} custom properties
are not supported for device type: ${InvalidDeviceType}")
Line 470: String
ACTION_TYPE_FAILED_INVALID_DEVICE_TYPE_FOR_CUSTOM_PROPERTIES();
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties
Line 173: ACTION_TYPE_FAILED_MIGRATION_IN_PROGRESS=Cannot ${action} ${type}. VM
migration is in progress
Line 174: ACTION_TYPE_FAILED_MIGRATION_TO_SAME_HOST=Cannot ${action} ${type}.
source and destination is the same.
Line 175: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_SYNTAX=Cannot
${action} ${type} if custom properties are in invalid format. Please check the
input.
Line 176: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_KEYS=Cannot
${action} ${type} if some of the specified custom properties are not configured
by the system. The keys are: ${MissingKeys}
Line 177: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_VALUES=Cannot
${action} ${type} if some of the specified custom properties have illegal
values. The keys are: ${WrongValueKeys}
please see comments about the message in
backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 178: ACTION_TYPE_FAILED_CUSTOM_PROPERTIES_NOT_SUPPORTED_IN_VERSION=Cannot
${action} ${type} custom properties are not supported in version:
${NotSupportedInVersion}
Line 179: ACTION_TYPE_FAILED_INVALID_DEVICE_TYPE_FOR_CUSTOM_PROPERTIES=Cannot
${action} ${type} custom properties are not supported for device type:
${InvalidDeviceType}
Line 180: ACTION_TYPE_FAILED_VDS_VM_CLUSTER=Cannot ${action} ${type}. There are
no available running Hosts in the Host Cluster.
Line 181: ACTION_TYPE_FAILED_VDS_VM_MEMORY=Cannot ${action} ${type}. There are
no available running Hosts with sufficient memory in VM's Cluster .
....................................................
File
frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties
Line 170: ACTION_TYPE_FAILED_MIGRATION_TO_SAME_HOST=Cannot ${action} ${type}.
source and destination is the same.
Line 171: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_SYNTAX=Cannot
${action} ${type} if custom properties are in invalid format. Please check the
input.
Line 172: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_KEYS=Cannot
${action} ${type} if some of the specified custom properties are not configured
by the system. The keys are: ${MissingKeys}
Line 173: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_VALUES=Cannot
${action} ${type} if some of the specified custom properties have illegal
values. The keys are: ${WrongValueKeys}
Line 174: ACTION_TYPE_FAILED_CUSTOM_PROPERTIES_NOT_SUPPORTED_IN_VERSION=Cannot
${action} ${type} custom properties are not supported in version:
${NotSupportedInVersion}
please see previous comment about the content of these messages.
Line 175: ACTION_TYPE_FAILED_INVALID_DEVICE_TYPE_FOR_CUSTOM_PROPERTIES=Cannot
${action} ${type} custom properties are not supported for device type:
${InvalidDeviceType}
Line 176: ACTION_TYPE_FAILED_VDS_VM_CLUSTER=Cannot ${action} ${type}. There are
no available running Hosts in the Host Cluster.
Line 177: ACTION_TYPE_FAILED_VDS_VM_MEMORY=Cannot ${action} ${type}. There are
no available running Hosts with sufficient memory in VM's Cluster .
Line 178: ACTION_TYPE_FAILED_VDS_VM_CPUS=Cannot ${action} ${type}. There are no
available running Hosts with enough cores in VM's Cluster .
--
To view, visit http://gerrit.ovirt.org/14814
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I07daa5cccbf8e3cbd6b4191bbc8ebf0729d9d9a0
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Peřina <mper...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Martin Peřina <mper...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
