Yevgeny Zaspitsky has posted comments on this change. Change subject: engine,webadmin: Plugging of an unlinked vnic with an ext net is blocked ......................................................................
Patch Set 1: (12 comments) 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 <yzasp...@redhat.com> 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 Done 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 Done Line 381: equals > Please use '==' to compare enums. Done 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. I do not see how the error can be reported twice. The flow stops on the first violation. Also the method is public and could be called independently. In general, if an error occurs during an execution that should be reported and not hidden. The isVnicAttachedToExternalNetwork method returns a boolean value, which is not enough to know if the method execution terminated correctly or upon an error. Checking validationResult completes the missing info. 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 pls see previous comment 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, pls see my comment on VmNicValidator.findVnicNetwork 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()' pls see my comments on UpdateVmInterfaceCommand 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 t vnicProfile is in use in VmNicValidator and its derrivative class, thus i do not see a good reason adding related to that logic to AbstractVmInterfaceCommand. If you care of performance, it can be cached as a data member of this class and be lazily initialized. 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 that is done in findVnicNetwork 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.g I just re-factored the existing code and strove keeping its behavior. NetworkHelper.getNetworkByVnicProfileId does not report validation violations. Since this piece of code is part of VmNicValidator reporting error is important. 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_ Done 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), http://gerrit.ovirt.org/#/c/32032/1/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 616: ACTION_TYPE_FAILED_EXTERNAL_NETWORK_CANNOT_BE_REWIRED=Cannot ${action} ${type}. External network cannot be changed while the virtual machine is running. Line 617: ACTION_TYPE_FAILED_EXTERNAL_NETWORK_CANNOT_HAVE_MTU=Cannot ${action} ${type}. External network cannot have MTU set. Line 618: ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_REQUIRED=Cannot ${action} ${type}. The management network '${NetworkName}' must be required, please change the network to be required and try again. Line 619: ACTION_TYPE_FAILED_NETWORK_ADDRESS_CANNOT_BE_CHANGED=Cannot ${action} ${type}. The address of the network '${ACTION_TYPE_FAILED_NETWORK_ADDRESS_CANNOT_BE_CHANGED_LIST}' cannot be modified without reinstalling the host, since this address was used to create the host's certification. Line 620: PLUGGED_UNLINKED_EXTERNAL_VM_INTERFACE_IS_NOT_SUPPORTED=Plugged and unlinked VM network interface with external network is not supported. > please add message prefix: Cannot ${action} ${type}. Done Line 621: CANNOT_PREVIEW_ACTIVE_SNAPSHOT=Cannot preview Active VM snapshot. Line 622: CONFIG_UNKNOWN_KEY=Illegal configuration entry.\n\ Line 623: -Please check configuration entry name. Line 624: TAGS_SPECIFIED_TAG_CANNOT_BE_THE_PARENT_OF_ITSELF=Operation canceled, recursive Tag hierarchy cannot be defined. -- 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 <yzasp...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
