Alona Kaplan has posted comments on this change. Change subject: core: refactored NetworkAttachmentCompleter + tests. ......................................................................
Patch Set 44: (20 comments) https://gerrit.ovirt.org/#/c/36142/44/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java: Line 361: Line 362: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); Line 363: //TODO MM: complain about unset network id. Line 364: if (!(violations.addViolation(validator.networkExists(), networkId) Line 365: && violations.addViolation(validateCoherentNicIdentification(attachment), attachment.getId().toString()) In case of new network attachment, attachment.getId() is null and you will get NPE here. Line 366: && violations.addViolation(modifiedAttachmentExists(attachment.getId()), networkId) Line 367: && violations.addViolation(validator.notExternalNetwork(), networkId) Line 368: && violations.addViolation(validator.networkAttachedToCluster(), networkId) Line 369: && violations.addViolation(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 382: private ValidationResult validateCoherentNicIdentification(NetworkAttachment attachment) { Line 383: return validateCoherentNicIdentification(attachment.getId(), Line 384: attachment.getNicId(), Line 385: attachment.getNicName(), Line 386: VdcBllMessages.NETWORK_ATTACHMENT_REFERENCES_NICS_INCOHERENTLY); The error message contains the attachment_id. In case of new attachment, there is no id, so please add separate error for new attachments or adjust the existing one for new attachments as well. Line 387: } Line 388: Line 389: private ValidationResult validateCoherentNicIdentification(Bond bond) { Line 390: Guid nicId = bond.getId(); Line 388: Line 389: private ValidationResult validateCoherentNicIdentification(Bond bond) { Line 390: Guid nicId = bond.getId(); Line 391: String nicName = bond.getName(); Line 392: VdcBllMessages message = VdcBllMessages.BOND_REFERENCES_NICS_INCOHERENTLY; same Line 393: return validateCoherentNicIdentification(bond.getId(), nicId, nicName, message); Line 394: Line 395: } Line 396: Line 416: String.format("$nicName %s", nicName) Line 417: }; Line 418: } Line 419: Line 420: private boolean nicNameAndNicIdIsIncoherent(Guid nicId, String nicName) { s/nicNameAndNicIdIsIncoherent/IsnicNameAndNicIdIncoherent Line 421: VdsNetworkInterface interfaceById = existingIfacesById.get(nicId); Line 422: VdsNetworkInterface interfaceByName = existingIfaces.get(nicName); Line 423: return !Objects.equals(interfaceById, interfaceByName); Line 424: } https://gerrit.ovirt.org/#/c/36142/44/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/NicNameNicIdCompleter.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/NicNameNicIdCompleter.java: Line 14: * {@link NetworkAttachment} entity or from {@link Bond} entity. If both were set, their coherence is verified. Line 15: */ Line 16: public class NicNameNicIdCompleter { Line 17: Line 18: static final String INCONSISTENT_NIC_IDENTIFICATION_ERROR_MESSAGE = "Both nicId and nicName was specified, but they do not denote same nic."; 1. s/was/were 2. The message is not use by this file, please move it to the relevant file. Line 19: Line 20: private BusinessEntityMap<VdsNetworkInterface> existingNicsMap; Line 21: Line 22: public NicNameNicIdCompleter(List<VdsNetworkInterface> existingNics) { Line 25: Line 26: public void completeNetworkAttachments(List<NetworkAttachment> networkAttachments) { Line 27: NicNameAndNicIdAccessors.FromNetworkAttachment accessors = new NicNameAndNicIdAccessors.FromNetworkAttachment(); Line 28: for (NetworkAttachment attachment : networkAttachments) { Line 29: complete(accessors.setNetworkAttachment(attachment)); Please call- completeNetworkAttachment(..) Line 30: } Line 31: } Line 32: Line 33: public void completeBonds(List<Bond> bonds) { Line 32: Line 33: public void completeBonds(List<Bond> bonds) { Line 34: NicNameAndNicIdAccessors.FromBond accessors = new NicNameAndNicIdAccessors.FromBond(); Line 35: for (Bond bond : bonds) { Line 36: complete(accessors.setBond(bond)); Please call- completeBond(..) Line 37: } Line 38: } Line 39: Line 40: /** https://gerrit.ovirt.org/#/c/36142/44/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/NicNameNicIdCompleterTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/NicNameNicIdCompleterTest.java: Line 51: assertThat("name should be bound to nicName", accessors.getName(), is(networkAttachment.getNicName())); Line 52: Line 53: accessors.setId(Guid.newGuid()); Line 54: accessors.setName("another name"); Line 55: assertThat("id should be bound to nicId", networkAttachment.getNicId(), is(accessors.getId())); Please extract to a method. Can be used here and instead lines- 50-51. (I don't really understand the reason for switching the order, is there?) Line 56: assertThat("name should be bound to nicName", networkAttachment.getNicName(), is(accessors.getName())); Line 57: } Line 58: Line 59: @Test Line 66: assertThat(accessors.getName(), is(bond.getName())); Line 67: Line 68: accessors.setId(Guid.newGuid()); Line 69: accessors.setName("another name"); Line 70: assertThat(bond.getId(), is(accessors.getId())); same Line 71: assertThat(bond.getName(), is(accessors.getName())); Line 72: } Line 73: Line 74: @Test Line 71: assertThat(bond.getName(), is(accessors.getName())); Line 72: } Line 73: Line 74: @Test Line 75: public void testCompleteNetworkAttachmentWhenUnsetIdAndName() throws Exception { Please change the test name- remove 'NetworkAttachment' from it. Line 76: NicNameAndNicIdAccessors withoutNameOrIdSet = mock(NicNameAndNicIdAccessors.class); Line 77: completer.complete(withoutNameOrIdSet); Line 78: verify(withoutNameOrIdSet, never()).setName(anyString()); Line 79: verify(withoutNameOrIdSet, never()).setId(any(Guid.class)); Line 79: verify(withoutNameOrIdSet, never()).setId(any(Guid.class)); Line 80: } Line 81: Line 82: @Test Line 83: public void testCompleteNetworkAttachmentWhenBothIdAndNameDoesNotReferenceExistingNic() throws Exception { same Line 84: NicNameAndNicIdAccessors accessors = mock(NicNameAndNicIdAccessors.class); Line 85: Line 86: Guid guidOfNotExistingNic = Guid.newGuid(); Line 87: when(accessors.getId()).thenReturn(guidOfNotExistingNic); Line 92: verify(accessors, never()).setId(any(Guid.class)); Line 93: } Line 94: Line 95: @Test Line 96: public void testCompleteNetworkAttachmentWhenNicIdReferencesExistingNic() throws Exception { same Line 97: TestAccessors withIdSet = new TestAccessors(); Line 98: withIdSet.setId(nic.getId()); Line 99: Line 100: completer.complete(withIdSet); Line 102: assertThat(withIdSet.getId(), is(nic.getId())); Line 103: } Line 104: Line 105: @Test Line 106: public void testCompleteNetworkAttachmentWhenNicNameReferencesExistingNic() throws Exception { same Line 107: TestAccessors withNameSet = new TestAccessors(); Line 108: withNameSet.setName(nic.getName()); Line 109: Line 110: completer.complete(withNameSet); Line 112: assertThat(withNameSet.getId(), is(nic.getId())); Line 113: } Line 114: Line 115: @Test Line 116: public void testCompleteNetworkAttachmentWhenNicNameAndNicIdAreIncoherent() throws Exception { same Line 117: TestAccessors accessors = new TestAccessors(); Line 118: accessors.setId(Guid.newGuid()); Line 119: accessors.setName(nic.getName()); Line 120: Line 117: TestAccessors accessors = new TestAccessors(); Line 118: accessors.setId(Guid.newGuid()); Line 119: accessors.setName(nic.getName()); Line 120: Line 121: expectedException.expect(IllegalArgumentException.class); Wasn't the exception removed? Line 122: expectedException.expectMessage(INCONSISTENT_NIC_IDENTIFICATION_ERROR_MESSAGE); Line 123: completer.complete(accessors); Line 124: } Line 125: https://gerrit.ovirt.org/#/c/36142/44/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 538: NETWORK_ATTACHMENT_NOT_SPECIFIED=Cannot ${action} ${type}. Network attachment must be specified for this request. Line 539: NETWORK_INTERFACE_ATTACHED_TO_NETWORK_CANNOT_BE_SLAVE=Network Interface cannot become slave, there's network attached to it. Line 540: BOND_USED_BY_NETWORK_ATTACHMENTS=Cannot remove bond, because it's used by Network Attachments. Line 541: NETWORK_ATTACHMENT_NOT_EXISTS=Cannot ${action} ${type}. Given Network Attachment does not exist Line 542: NETWORK_ATTACHMENT_REFERENCES_NICS_INCOHERENTLY=Cannot ${action} ${type}. NetworkAttachment (id=${id}}) does not reference nic coherently. Nic name and id references different nic: nicId=${nicId} nicName=${nicName}}. Please remove the '=' sign from the message. It is illegal character and should be escaped. But as you can see non of our existing messages use this format in the message. So please just replace the '=' with space. Line 543: BOND_REFERENCES_NICS_INCOHERENTLY=Cannot ${action} ${type}. Bond (id=${id}}) does not reference nic coherently. Nic name and id references different nic: nicId=${nicId} nicName=${nicName}}. Line 544: NETWORK_ALREADY_ATTACHED_TO_HOST=Cannot ${action} ${type}. Given network is already attached to given host. Line 545: CANNOT_CHANGE_ATTACHED_NETWORK=Cannot ${action} ${type}. Network of given Network Attachment cannot be updated. Line 546: ACTION_TYPE_FAILED_DISPLAY_NETWORK_HAS_NO_BOOT_PROTOCOL=Display network hasn't boot protocol assigned. Line 539: NETWORK_INTERFACE_ATTACHED_TO_NETWORK_CANNOT_BE_SLAVE=Network Interface cannot become slave, there's network attached to it. Line 540: BOND_USED_BY_NETWORK_ATTACHMENTS=Cannot remove bond, because it's used by Network Attachments. Line 541: NETWORK_ATTACHMENT_NOT_EXISTS=Cannot ${action} ${type}. Given Network Attachment does not exist Line 542: NETWORK_ATTACHMENT_REFERENCES_NICS_INCOHERENTLY=Cannot ${action} ${type}. NetworkAttachment (id=${id}}) does not reference nic coherently. Nic name and id references different nic: nicId=${nicId} nicName=${nicName}}. Line 543: BOND_REFERENCES_NICS_INCOHERENTLY=Cannot ${action} ${type}. Bond (id=${id}}) does not reference nic coherently. Nic name and id references different nic: nicId=${nicId} nicName=${nicName}}. same Line 544: NETWORK_ALREADY_ATTACHED_TO_HOST=Cannot ${action} ${type}. Given network is already attached to given host. Line 545: CANNOT_CHANGE_ATTACHED_NETWORK=Cannot ${action} ${type}. Network of given Network Attachment cannot be updated. Line 546: ACTION_TYPE_FAILED_DISPLAY_NETWORK_HAS_NO_BOOT_PROTOCOL=Display network hasn't boot protocol assigned. Line 547: NETWORK_INVALID_BOND_NAME=Bond name must be formatted as <bondYYY>. https://gerrit.ovirt.org/#/c/36142/44/frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties File frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties: Please add the new messages also to- AppErrors.java Line 1: DIRECTORY_GROUP_CANNOT_REMOVE_DIRECTORY_GROUP_ATTACHED_TO_VM_POOL=Cannot remove Directory Group. Detach Group from VM-Pool first. Line 2: IO_CD_IMAGE_FILE_ALREADY_EXIST=Image file already exists. Line 3: IO_CD_IMAGE_FILE_NOT_EXIST=Image file does not exist. Line 4: IO_INVALID_CD_IMAGE_EXTENSION=Invalid CD image extension. Line 546: NETWORK_ATTACHMENT_NOT_SPECIFIED=Cannot ${action} ${type}. Network attachment must be specified for this request. Line 547: NETWORK_INTERFACE_ATTACHED_TO_NETWORK_CANNOT_BE_SLAVE=Network Interface cannot become slave, there's network attached to it. Line 548: BOND_USED_BY_NETWORK_ATTACHMENTS=Cannot remove bond, because it's used by Network Attachments. Line 549: NETWORK_ATTACHMENT_NOT_EXISTS=Cannot ${action} ${type}. Given Network Attachment does not exist Line 550: NETWORK_ATTACHMENT_REFERENCES_NICS_INCOHERENTLY=Cannot ${action} ${type}. NetworkAttachment (id=${id}}) does not reference nic coherently. Nic name and id references different nic: nicId=${nicId} nicName=${nicName}}. same Line 551: BOND_REFERENCES_NICS_INCOHERENTLY=Cannot ${action} ${type}. Bond (id=${id}}) does not reference nic coherently. Nic name and id references different nic: nicId=${nicId} nicName=${nicName}}. Line 552: NETWORK_ALREADY_ATTACHED_TO_HOST=Cannot ${action} ${type}. Given network is already attached to given host. Line 553: CANNOT_CHANGE_ATTACHED_NETWORK=Cannot ${action} ${type}. Network of given Network Attachment cannot be updated. Line 554: ACTION_TYPE_FAILED_DISPLAY_NETWORK_HAS_NO_BOOT_PROTOCOL=Display network hasn't boot protocol assigned. Line 547: NETWORK_INTERFACE_ATTACHED_TO_NETWORK_CANNOT_BE_SLAVE=Network Interface cannot become slave, there's network attached to it. Line 548: BOND_USED_BY_NETWORK_ATTACHMENTS=Cannot remove bond, because it's used by Network Attachments. Line 549: NETWORK_ATTACHMENT_NOT_EXISTS=Cannot ${action} ${type}. Given Network Attachment does not exist Line 550: NETWORK_ATTACHMENT_REFERENCES_NICS_INCOHERENTLY=Cannot ${action} ${type}. NetworkAttachment (id=${id}}) does not reference nic coherently. Nic name and id references different nic: nicId=${nicId} nicName=${nicName}}. Line 551: BOND_REFERENCES_NICS_INCOHERENTLY=Cannot ${action} ${type}. Bond (id=${id}}) does not reference nic coherently. Nic name and id references different nic: nicId=${nicId} nicName=${nicName}}. same Line 552: NETWORK_ALREADY_ATTACHED_TO_HOST=Cannot ${action} ${type}. Given network is already attached to given host. Line 553: CANNOT_CHANGE_ATTACHED_NETWORK=Cannot ${action} ${type}. Network of given Network Attachment cannot be updated. Line 554: ACTION_TYPE_FAILED_DISPLAY_NETWORK_HAS_NO_BOOT_PROTOCOL=Display network hasn't boot protocol assigned. Line 555: NETWORK_INVALID_BOND_NAME=Bond name must be formatted as <bondYYY>. -- To view, visit https://gerrit.ovirt.org/36142 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I933333afdcb11bfcf9f761572229f7a7a7814381 Gerrit-PatchSet: 44 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
