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/HostSetupNetworksValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java: Line 236: candidateAttachments.addAll(newAttachments); Line 237: return candidateAttachments; Line 238: } Line 239: Line 240: private boolean validModifiedBonds() { Should be validNewOrModifiedBonds- It was fixed in a later patch? If you can please move all the fixes to here. It is really hard to review the class this way. Line 241: boolean passed = true; Line 242: for (Bond modifiedBond : params.getBonds()) { Line 243: if (modifiedBond.getName() == null) { Line 244: addViolation(VdcBllMessages.HOST_NETWORK_INTERFACE_NOT_EXIST, null); Line 273: passed = false; Line 274: continue; Line 275: } Line 276: Line 277: String slaveBondName = slave.getBondName(); 1. This validation blocks the possibility of moving slave from one bond to another. Why? 2. You should verify the same slave is not attached to more than one bond. Line 278: if (slave.isBondSlave() && (!slaveBondName.equals(modifiedBond.getName()) Line 279: || !slaveUsedByRemovedBond(slaveBondName))) { Line 280: addViolation(VdcBllMessages.NETWORK_INTERFACE_ALREADY_IN_BOND, slaveName); Line 281: passed = false; Line 292: Line 293: return passed; Line 294: } Line 295: Line 296: private boolean nicUsedByRemovedNetwork(VdsNetworkInterface slave) { 1. maybe nicUseByRemovedNetworkAtachment. It is confusing, since the network may be attached to another nic. The attachment is removed. 2. Why is the paramenter name- slave? The nethod is general and can be use by any nic. Line 297: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 298: if (Objects.equals(slave.getName(), attachment.getNicName())) { Line 299: return true; Line 300: } Line 302: Line 303: return false; Line 304: } Line 305: Line 306: private boolean slaveUsedByRemovedBond(String slaveBondName) { Maybe- shouldRemoveBond(String bondName) Line 307: for (VdsNetworkInterface removedBond : removedBondVdsNetworkInterface) { Line 308: if (slaveBondName.equals(removedBond.getName())) { Line 309: return true; Line 310: } Line 316: private boolean validModifiedNetworkAttachments(Map<Guid, NetworkAttachment> attachmentsById) { 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(); In the next line you check if 'networkAttachmentIsSet' (is not null), so I guess it can be null. Please add this null check when getting the networkId. Line 321: if (!(validate(validator.networkAttachmentIsSet()) Line 322: && validate(validator.networkExists(), networkId) Line 323: && validate(validator.notExternalNetwork(), networkId) Line 324: && validate(validator.networkAttachedToCluster(), networkId) 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. Shouldn't you first check the networkId is not null? 2. In a future patch you also have networkActuallyExists, I don't understand why it is needed. It seems to do the same as this validation. One of them seems to be redundant. 3. In both of the methods you use the following message- "The specified Logical Network doesn't exist." This message is not specific enough for setup networks, you should mention the problematic network id. 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) > The validator.nicExists() only checks if the attachment.nicName is not null Regarding the nicActuallyExistsOrReferencesNewBond method, which is not in this patch but should be: The message- "The host network interface does not exist." Is not specific enough for setup networks. You have to specify the problematic network id. 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()) The message you get in this validation is- Given Network Attachment does not exist. For setup network it is not specific enough. You have to specify the problematic id. Line 344: && validate(validator.notExternalNetwork()) Line 345: && validate(validator.notManagementNetwork()) Line 346: && notRemovingLabeledNetworks(attachment, getExistingIfaces()))) { Line 347: passed = false; 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()) Line 344: && validate(validator.notExternalNetwork()) Is it possible to have an attachment with external network? Line 345: && validate(validator.notManagementNetwork()) Line 346: && notRemovingLabeledNetworks(attachment, getExistingIfaces()))) { Line 347: passed = false; Line 348: } Line 378: private boolean validateMtu(Collection<NetworkAttachment> attachmentsToConfigure) { Line 379: boolean passed = true; Line 380: Map<String, List<Network>> nicsToNetworks = getNetworksOnNics(attachmentsToConfigure); Line 381: Line 382: for (Entry<String, List<Network>> nicToNetwork : nicsToNetworks.entrySet()) { s/nicToNetwork/nicToNetworks Line 383: List<Network> networksOnNic = nicToNetwork.getValue(); Line 384: if (!networksOnNicMatchMtu(networksOnNic)) { Line 385: reportMtuDifferences(networksOnNic); Line 386: passed = false; Line 408: Line 409: return true; Line 410: } Line 411: Line 412: private Map<String, List<Network>> getNetworksOnNics(Collection<NetworkAttachment> attachmentsToConfigure) { Maybe- getNicToNetworks(..) Line 413: Map<String, List<Network>> nicsToNetworks = new HashMap<>(); Line 414: for (NetworkAttachment attachment : attachmentsToConfigure) { Line 415: String nicName = attachment.getNicName(); Line 416: if (!nicsToNetworks.containsKey(nicName)) { Line 448: clusterNetworks.get(attachment.getNetworkId()); Saw this more than once, please extract to a method. Line 489: Line 490: return validationResult.isValid(); Line 491: } Line 492: Line 493: private boolean validate(ValidationResult validationResult) { You can just call- validate(validationResult, null) Line 494: if (!validationResult.isValid()) { Line 495: addViolation(validationResult.getMessage(), null); Line 496: } Line 497: -- 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
