Alona Kaplan 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) ? I think I meant in my previous comment (It was so long time ago that I'm not sure) you don't need the prefix "super." 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 setNetworkExclusivelyAsManagement(..)? 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() 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 network to be management. So maybe you can call it- managementNetworkSet." You are checking just if the network became mgmt and not if mgmt network became a regular one. 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 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. networkCluster.isManagement() && !oldNetworkCluster.isManagement() In the current code, managementNetworkChange will also catch (in some cases) unsetting of the mgmt network. This is not desired since there is a special validation for the unsetting. 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 variable of the method. I don't see any reason to make it a d.m of the class. 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>/config/engine-code-format.xml 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. 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 validation is just about setting a network to be mgmt. There is another vaslidation for unsetting a network from being mgmt. 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 automatically done when moving the role to another network" 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
