Alona Kaplan has posted comments on this change. Change subject: engine,webadmin: Plugging of an unlinked ext VM net interface is blocked ......................................................................
Patch Set 1: Code-Review-1 (11 comments) PLUGGED_UNLINKED_VM_INTERFACE_WITH_EXTERNAL_NETWORK_IS_NOT_SUPPORTED entry should be added also to the user portal AppErrors.properties file. http://gerrit.ovirt.org/#/c/32032/1//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-08-27 13:24:50 +0300 Line 4: Commit: Yevgeny Zaspitsky <[email protected]> Line 5: CommitDate: 2014-08-27 13:59:47 +0300 Line 6: Line 7: engine,webadmin: Plugging of an unlinked ext VM net interface is blocked Plugging of an unlinked vnic with an external net is blocked Line 8: Line 9: Plugged and unlinked VM network interface is not supported for an Line 10: external network, thus the action is blocked. Line 11: http://gerrit.ovirt.org/#/c/32032/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/UpdateVmInterfaceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/UpdateVmInterfaceCommand.java: Line 380: canExternalNetworkVnicBePlugged s/canExternalNetworkVnicBePlugged/canVnicWithExternalNetworkBePlugged Line 381: equals Please use '==' to compare enums. Line 379: Line 380: public ValidationResult canExternalNetworkVnicBePlugged() { Line 381: if (RequiredAction.PLUG.equals(getRequiredAction()) && !nic.isLinked()) { Line 382: final boolean vnicAttachedToExternalNetwork = isVnicAttachedToExternalNetwork(); Line 383: if (validationResult != ValidationResult.VALID) { This if is redundant. This method should validate JUST if vnic with external network can be plugged. You don't need to return the validation result of previous validations. With your code, if the profile is not valid and you try to plug an unlinked vnic- you will return the same validation error twice. Line 384: return validationResult; Line 385: } Line 386: Line 387: if (vnicAttachedToExternalNetwork) { Line 383: if (validationResult != ValidationResult.VALID) { Line 384: return validationResult; Line 385: } Line 386: Line 387: if (vnicAttachedToExternalNetwork) { Since the previous if block should be removed. Please join this if with the upper if- if (RequiredAction.PLUG.equals(getRequiredAction()) && !nic.isLinked() && isVnicAttachedToExternalNetwork()) {..} ps- vnicAttachedToExternalNetwork var can be removed. Line 388: return new ValidationResult(VdcBllMessages.PLUGGED_UNLINKED_EXTERNAL_VM_INTERFACE_IS_NOT_SUPPORTED); Line 389: } Line 390: } Line 391: Line 392: return ValidationResult.VALID; Line 393: } Line 394: Line 395: private boolean isVnicAttachedToExternalNetwork() { Line 396: final Network network = findVnicNetwork(); NetworkHelper.getNetworkByVnicProfileId(..) can be used here, there is no need to introduce new method. Since there is more than one place that needs the vnic network I suggest adding getNetwork() method to AbstractVmInterfaceCommand. You can see 'ActivateDeactivateVmNicCommand.getNetwork()' for example. Line 397: return (network != null && network.isExternal()); Line 398: } Line 399: } http://gerrit.ovirt.org/#/c/32032/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmNicValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmNicValidator.java: Line 29: protected Version version; Line 30: Line 31: protected int osId; Line 32: Line 33: protected ValidationResult validationResult = ValidationResult.VALID; As I wrote in 'UpdateVmInterfaceCommand.canExternalNetworkVnicBePlugged()' this d.m is not needed. Line 34: Line 35: public VmNicValidator(VmNic nic, Version version) { Line 36: this.nic = nic; Line 37: this.version = version; Line 70: * </ul> Line 71: */ Line 72: public ValidationResult profileValid(Guid clusterId) { Line 73: if (nic.getVnicProfileId() != null) { Line 74: final Network network = findVnicNetwork(); As I wrote in UpdateVmInterfaceCommand about adding 'getNetwork()' method to AbstractVmInterfaceCommand. Please add a method 'getVnicProfile()' to AbstractVmInterfaceCommand. Pass its result as a parameter to 'profileValid(..)'. Line 75: if (validationResult != ValidationResult.VALID) { Line 76: return validationResult; Line 77: } Line 78: // Check that the network exists in current cluster Line 71: */ Line 72: public ValidationResult profileValid(Guid clusterId) { Line 73: if (nic.getVnicProfileId() != null) { Line 74: final Network network = findVnicNetwork(); Line 75: if (validationResult != ValidationResult.VALID) { This is not needed since the find should be removed, the previous if should be preserved. if (vnicProfile == null) { return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VNIC_PROFILE_NOT_EXISTS); } Line 76: return validationResult; Line 77: } Line 78: // Check that the network exists in current cluster Line 79: if (network == null || !isNetworkInCluster(network, clusterId)) { Line 121: protected DbFacade getDbFacade() { Line 122: return DbFacade.getInstance(); Line 123: } Line 124: Line 125: protected Network findVnicNetwork() { This method is unneeded. It has the same functionality as 'NetworkHelper.getNetworkByVnicProfileId(..)'. The only difference is the check if the profile exists. There is already a separate validation for it. All the other validations that come after the 'profileValid(..)' can assume the profile is valid. Line 126: if (nic.getVnicProfileId() == null) { Line 127: return null; Line 128: } Line 129: http://gerrit.ovirt.org/#/c/32032/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 775: ACTION_TYPE_FAILED_NETWORK_QOS_IS_NOT_SUPPORTED(ErrorType.NOT_SUPPORTED), Line 776: ACTION_TYPE_FAILED_HOST_NETWORK_QOS_NOT_SUPPORTED(ErrorType.NOT_SUPPORTED), Line 777: ACTION_TYPE_FAILED_HOST_NETWORK_LABELS_NOT_SUPPORTED(ErrorType.NOT_SUPPORTED), Line 778: HOT_VM_INTERFACE_UPDATE_IS_NOT_SUPPORTED(ErrorType.NOT_SUPPORTED), Line 779: PLUGGED_UNLINKED_EXTERNAL_VM_INTERFACE_IS_NOT_SUPPORTED(ErrorType.NOT_SUPPORTED), s/PLUGGED_UNLINKED_EXTERNAL_VM_INTERFACE_IS_NOT_SUPPORTED/PLUGGED_UNLINKED_VM_INTERFACE_WITH_EXTERNAL_NETWORK_IS_NOT_SUPPORTED Note the AppErrors files should also be updated. Line 780: ACTION_TYPE_FAILED_VM_INTERFACE_TYPE_IS_NOT_SUPPORTED_BY_OS(ErrorType.INCOMPATIBLE_VERSION), Line 781: CANNOT_PERFORM_HOT_UPDATE(ErrorType.CONFLICT), Line 782: CANNOT_PERFORM_HOT_UPDATE_WITH_PORT_MIRRORING(ErrorType.CONFLICT), Line 783: PORT_MIRRORING_REQUIRES_NETWORK(ErrorType.BAD_PARAMETERS), -- To view, visit http://gerrit.ovirt.org/32032 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia944ca7e0430adc54a6cae6cb65b8a1df4e88d24 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
