Alona Kaplan has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 26: (13 comments) https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java: Line 160: runVdsCommand(VDSCommandType.GetCapabilities, Line 161: new VdsIdAndVdsVDSCommandParametersBase(getVds())); Line 162: VDS updatedHost = (VDS) returnValue.getReturnValue(); Line 163: persistNetworkChanges(updatedHost); Line 164: } > I can, but wouldn't that be really cumbersome and non-OO? It can be very helpful to have the unlocking message. You can implement functionality for the lock itself to print the lock/unlock messages and then remove the command logging. But till this functionality is introduced please add the command (ulock) logging. Line 165: Line 166: setSucceeded(true); Line 167: } Line 168: } Line 366: Line 367: private List<Network> getModifiedNetworks() { Line 368: List<Network> modifiedNetworks = new ArrayList<>(getParameters().getNetworkAttachments().size()); Line 369: for (NetworkAttachment attachment : getParameters().getNetworkAttachments()) { Line 370: modifiedNetworks.add(getClusterNetworks().get(attachment.getNetworkId())); > I'm not sure if I understand. Sorry, my comment was really unclear:) You've done exactly what I tried to say. Line 371: } Line 372: Line 373: return modifiedNetworks; Line 374: } https://gerrit.ovirt.org/#/c/33333/26/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 86: && validModifiedNetworkAttachments(attachmentsById) Line 87: && validRemovedNetworkAttachments(attachmentsById) Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() Line 90: && validateNotRemovingUsedNetworkByVms()) { > I can do it. Notice that this will make validRemovedNetworkAttachments bigg I think it is more readable and clear. And anyway, you're not testing validRemovedNetworkAttachments, you're testing just its inner validations. Line 91: Line 92: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 93: if (networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 94: && validate(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() Line 90: && validateNotRemovingUsedNetworkByVms()) { Line 91: Line 92: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); > I can do it. Notice that this will make validModifiedNetworkAttachments big Same answer as the previous comment. You're not testing validModifiedNetworkAttachments, it aggregates a lot of tested validation and it is already super hard to test it (I don't think the addig one more validation will make the difference). Line 93: if (networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 94: && validate(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Line 95: && validateMtu(attachmentsToConfigure) Line 96: && validateCustomProperties()) { Line 209: Line 210: private Collection<NetworkAttachment> getAttachmentsToConfigure() { Line 211: Map<Guid, NetworkAttachment> attachmentsToConfigure = new HashMap<>(Entities.businessEntitiesById(existingAttachments)); Line 212: // ignore removed attachments Line 213: for (NetworkAttachment removedAttachment : this.removedNetworkAttachments) { > with what? I meant- s/this.removedNetworkAttachments/removedNetworkAttachments Line 214: attachmentsToConfigure.remove(removedAttachment.getId()); Line 215: } Line 216: Line 217: List<NetworkAttachment> newAttachments = new ArrayList<>(); Line 292: Line 293: return passed; Line 294: } Line 295: Line 296: private boolean nicUsedByRemovedNetwork(VdsNetworkInterface slave) { > both renamed. I opted for "isNetworkAttachmentRemoved" to be consistent wit You are correct, there is a bug here. More than one network attachment can be attached to a nic. You have to check that all the network attachments related to the nic were removed. Line 297: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 298: if (Objects.equals(slave.getName(), attachment.getNicName())) { Line 299: return true; Line 300: } Line 312: Line 313: return false; Line 314: } Line 315: Line 316: private boolean validModifiedNetworkAttachments(Map<Guid, NetworkAttachment> attachmentsById) { > I think moti designed it like this: I discussed it with Moti, and he and I think that removing the attachment and creating a fresh one with the same network should be block. Have only one explicit way for doing it will simplify future validation and may avoid bugs. The ui already persists the ip-conifuration of the network when the network is detached from a nic (and uses it when it is attached to a new one), so it sounds reasonable for a detach networks to save for future use the whole network attachment. Line 317: boolean passed = true; Line 318: for (NetworkAttachment attachment : params.getNetworkAttachments()) { Line 319: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachment, host, managementNetworkUtil); Line 320: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); Line 318: for (NetworkAttachment attachment : params.getNetworkAttachments()) { Line 319: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachment, host, managementNetworkUtil); Line 320: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); Line 321: if (!(validate(validator.networkAttachmentIsSet()) Line 322: && validate(validator.networkExists(), networkId) > 1— I think this is correct; when network id is null, this method ("org.ovir Regarding 3- The error message you get here is part of the host setup networks command- So getting the following error- "The specified Logical Network doesn't exist." is not informative enough. If the network_id is null- you can have the following- "One of the specified attachments doesn't have network id". If it have networkId and but the network doesn't exist you can have- "Logical Network with ID ${id} doesn't exist." You said that- "if it's just a inexisting network .. pair error message .. will be stored", I didn't find the code doing it. Please also notice, adding violating entities to the validation is not enough. The message should contain places holders for the entities data. Line 323: && validate(validator.notExternalNetwork(), networkId) Line 324: && validate(validator.networkAttachedToCluster(), networkId) Line 325: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 326: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) Line 323: && validate(validator.notExternalNetwork(), networkId) Line 324: && validate(validator.networkAttachedToCluster(), networkId) Line 325: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 326: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) Line 327: && validate(validator.nicExists(), networkId) > Done Please refer to my comment on patchset-35. Maybe I miss something, but adding violationEntity will actually affect something, just if the message contains the following {message_name}_LIST. Please validate this patch, I'm pretty sure the displayed error message won't have any reference to the added 'violation entity'. Line 328: && validate(validator.networkIpAddressWasSameAsHostnameAndChanged(getExistingIfaces())) Line 329: && validate(validator.networkNotChanged(attachmentsById.get(attachment.getId()))) Line 330: && validate(validator.validateGateway()))) { Line 331: passed = false; Line 339: boolean passed = true; Line 340: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 341: NetworkAttachment attachmentToValidate = attachmentsById.get(attachment.getId()); Line 342: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachmentToValidate, host, managementNetworkUtil); Line 343: if (!(validate(validator.networkAttachmentIsSet()) > I've added attachment Id. However I think, that after my latest changes, if I agree, the validation should be removed. Regarding the "I've added attachment Id" as I mentioned in previous comments, since the error message wasn't changed I think it won't affect anything. Line 344: && validate(validator.notExternalNetwork()) Line 345: && validate(validator.notManagementNetwork()) Line 346: && notRemovingLabeledNetworks(attachment, getExistingIfaces()))) { Line 347: passed = false; https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Entities.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Entities.java: Line 206: * Line 207: * @param requestedIds id to find. Line 208: * @param existingEntities all existing entities Line 209: * @param resultCollection result collection Line 210: * @param <E> > Done I meant you should remove the @param <E> not the whole java doc. Line 211: * @param <I> Line 212: * Line 213: * @return true if all entities for given ids were found. Line 214: * @throws IllegalArgumentException when result collection is not empty.14.1. https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java: Line 34: this.hostId = hostId; Line 35: this.userNetworkAttachments = userNetworkAttachments; Line 36: this.clusterNetworks = new BusinessEntityMap<>(clusterNetworks); Line 37: nicsByName = Entities.entitiesByName(nics); Line 38: configuredNetworkAttachments = new ArrayList<>(); > I don't know, why it was introduced. And do not want (if possible) to spent never mind Line 39: Line 40: reportedNetworksById = new HashMap<>(); Line 41: reportedNicsByNetworkId = new HashMap<>(); Line 42: for (VdsNetworkInterface nic : nics) { Line 75: Guid nicId = nic.getBaseInterface() == null ? nic.getId() : nicsByName.get(nic.getBaseInterface()).getId(); Line 76: attachment.setNicId(nicId); Line 77: attachment.setProperties(nic.getCustomProperties()); Line 78: networkAttachmentDao.save(attachment); Line 79: } > to be completely frank, I don't know where and how did I 'fix' it, and cann In networkAttachmentExistsForNetwork you've checked if the attachment is part of the configuredNetworkAttachments. I think that I confused 'configuredNetworkAttachments' with userNetworkAttachments. But in the final version you're not using the 'configuredNetworkAttachments' and the code seems to be ok, so please ignore the comment. Line 80: } Line 81: Line 82: Line 83: private boolean networkAttachmentExistsForNetwork(String networkName) { -- To view, visit https://gerrit.ovirt.org/33333 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I60077019f308f371f21fb7b5545ba48adb38bd06 Gerrit-PatchSet: 26 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
