Yevgeny Zaspitsky has posted comments on this change. Change subject: engine: update commands that might change management network ......................................................................
Patch Set 24: (11 comments) http://gerrit.ovirt.org/#/c/33416/24/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java: Line 103: Line 104: private boolean validateAttachment() { Line 105: final AttachNetworkClusterValidator networkClusterValidator = Line 106: new AttachNetworkClusterValidator(getNetworkCluster(), getClusterVersion()); Line 107: return validate(networkClusterValidator.networkBelongsToClusterDataCenter(getVdsGroup(), getNetwork())); > Why did you remove the call to validateAttachment(networkClusterValidator) Done Line 108: } Line 109: Line 110: private boolean logicalNetworkExists() { Line 111: if (getPersistedNetwork() != null) { Line 199: NetworkStatus.OPERATIONAL, Line 200: false, Line 201: networkCluster.isRequired(), Line 202: false, Line 203: false)); > In case of mgmt network, shouldn't you call setNetworkExclusivelyAsManageme Done Line 204: Line 205: if (network.getCluster().isDisplay()) { Line 206: final DisplayNetworkClusterHelper displayNetworkClusterHelper = new DisplayNetworkClusterHelper( Line 207: getNetworkClusterDao(), http://gerrit.ovirt.org/#/c/33416/24/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterCommandBase.java: Line 44: getPersistedNetwork > you can use the 'network' parameter instead of getPersistedNetwork() Done http://gerrit.ovirt.org/#/c/33416/24/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidatorBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidatorBase.java: Line 55: * Make sure the management network change is valid - that is allowed in an empty cluster only Line 56: * Line 57: * @return Error if the management network change is not allowed. Line 58: */ Line 59: public ValidationResult managementNetworkChange() { > As I worte in patch set 17- "This check is relevant just for setting a netw management network could be changed by setting an existing network as the management one or attaching a new network as the management network. Here in the abstract base class we cannot tell what exact operation being executed. So, IMHO, "change" better suites the operation description, that what we try to validate. Line 60: return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_CHANGED). Line 61: when(isManagementNetworkChangeInvalid()); Line 62: } Line 63: Line 74: private boolean isClusterEmpty() { Line 75: return getVdsGao().getAllForVdsGroup(networkCluster.getClusterId()).isEmpty(); Line 76: } Line 77: Line 78: protected VdsDAO getVdsGao() { > s/getVdsGao/getVdsDao Done Line 79: return DbFacade.getInstance().getVdsDao(); Line 80: } Line 81: Line 82: /** http://gerrit.ovirt.org/#/c/33416/24/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkClusterValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkClusterValidator.java: Line 17: this.oldNetworkCluster = oldNetworkCluster; Line 18: } Line 19: Line 20: protected boolean isManagementNetworkChanged() { Line 21: return oldNetworkCluster.isManagement() != networkCluster.isManagement(); > You should check just if the network became mgmt. Done Line 22: } Line 23: Line 24: public ValidationResult managementNetworkUnset() { Line 25: return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_UNSET). http://gerrit.ovirt.org/#/c/33416/24/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkOnClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkOnClusterCommand.java: Line 75: getNetworkClusterDAO().update(getNetworkCluster()); Line 76: Line 77: if (getNetworkCluster().isManagement() && !getOldNetworkCluster().isManagement()) { Line 78: getNetworkClusterDAO().setNetworkExclusivelyAsManagement(getVdsGroupId(), getPersistedNetwork().getId()); Line 79: setManagementNetwork(getPersistedNetwork()); > Since managementNetwork is used only in this method, it can be a local vari Done Line 80: } Line 81: Line 82: if (getNetworkCluster().isDisplay() != getOldNetworkCluster().isDisplay()) { Line 83: getNetworkClusterDAO().setNetworkExclusivelyAsDisplay(getVdsGroupId(), http://gerrit.ovirt.org/#/c/33416/24/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/AddClusterNetworkClusterValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/AddClusterNetworkClusterValidatorTest.java: Line 4: import org.mockito.runners.MockitoJUnitRunner; Line 5: Line 6: @RunWith(MockitoJUnitRunner.class) Line 7: public class AddClusterNetworkClusterValidatorTest extends Line 8: NetworkClusterValidatorTestBase<AddClusterNetworkClusterValidator> { > It is still unformatted. Please use the regular formatter- <ovirt-src-root> Done Line 9: Line 10: @Override Line 11: protected AddClusterNetworkClusterValidator createValidator() { Line 12: return new AddClusterNetworkClusterValidator(networkCluster, version); http://gerrit.ovirt.org/#/c/33416/24/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidatorTestBase.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidatorTestBase.java: Line 74: when(currentManagementNetwork.getId()).thenReturn(TEST_CURRENT_MANAGMENT_NETWORK_ID); Line 75: Line 76: validator = spy(createValidator()); Line 77: Line 78: Mockito.doReturn(vdsDao).when(validator).getVdsGao(); > Please add static import for doReturn. Done Line 79: } Line 80: Line 81: protected abstract T createValidator(); Line 82: http://gerrit.ovirt.org/#/c/33416/24/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkClusterValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkClusterValidatorTest.java: Line 55: } Line 56: Line 57: @Test Line 58: public void managementNetworkChangeInvalidNonEmptyClusterUnsetManagementNet() { Line 59: testUpdateManagementNetworkChange( > I think this validation should be responsible for checking this test. The v Done Line 60: true, Line 61: false, Line 62: false, Line 63: failsWith(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_CHANGED)); http://gerrit.ovirt.org/#/c/33416/24/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 630: TAGS_SPECIFIED_TAG_CANNOT_BE_THE_PARENT_OF_ITSELF=Operation canceled, recursive Tag hierarchy cannot be defined. Line 631: VM_CANNOT_MOVE_TO_CLUSTER_IN_OTHER_STORAGE_POOL=VM can be moved only to a Cluster in the same Data Center. Line 632: VM_CLUSTER_IS_NOT_VALID=Cannot ${action} ${type}. Cluster ID is not valid. Line 633: NETWORK_CANNOT_REMOVE_MANAGEMENT_NETWORK=The Management Network ('${NetworkName}') is mandatory and cannot be removed. Line 634: ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_UNSET=Cannot ${action} ${type}. Unsetting management network property is not allowed. > Maybe- "Unsetting management network role is not allowed. It is automatical Done Line 635: NETWORK_CANNOT_REMOVE_ISCSI_BOND_NETWORK=The Network ('${NetworkName}') could not be removed since several iSCSI bonds (${IscsiBonds_COUNTER}) are using this network:\n ${IscsiBonds}.\nPlease remove the network first from those iSCSI bon Line 636: NETWORK_OLD_NETWORK_NOT_SPECIFIED=Previous network name is required. Line 637: ACTION_TYPE_FAILED_DETECTED_ACTIVE_VMS=Cannot ${action} ${type}. Active VMs were detected.\n\ Line 638: -Please ensure all VMs associated with this Storage Domain are stopped and in the Down state first. -- To view, visit http://gerrit.ovirt.org/33416 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I196a583ae7d8d7e373a1aca2a48e592232b18a5b Gerrit-PatchSet: 24 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@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
