Alona Kaplan has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 26: (15 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 206: : Config.<Integer> getValue(ConfigValues.NetworkConnectivityCheckTimeoutInSeconds); Line 207: } Line 208: Line 209: private boolean defaultRouteRequired(Network network, IpConfiguration ipConfiguration) { Line 210: return managementNetworkUtil.isManagementNetwork(network.getId()) you should use- managementNetworkUtil.isManagementNetwork(network.getId(), getVds().getVdsGroupId()) The method you are using checks if the network if management in any cluster. You should care just about the current host's cluster. Line 211: && ipConfiguration != null Line 212: && (ipConfiguration.getBootProtocol() == NetworkBootProtocol.DHCP Line 213: || ipConfiguration.getBootProtocol() == NetworkBootProtocol.STATIC_IP Line 214: && StringUtils.isNotEmpty(ipConfiguration.getGateway())); Line 363: Line 364: return userConfiguredNics; Line 365: } Line 366: Line 367: private List<Network> getModifiedNetworks() { Please cache it, like you do in all the other getters. 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())); Line 371: } Line 370: getClusterNetworks().get(attachment.getNetworkId() Please extract getClusterNetworks().get(attachment.getNetworkId()) to a method, you have the same logic there. Also the naming is confusing, maybe you call this method getNetoworks and the one getNetwork->getHostNetworks... 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 83: Line 84: if (validateAndInitRemovedBondInterfaces() Line 85: && validateAndInitRemovedNetworkAttachments() Line 86: && validModifiedNetworkAttachments(attachmentsById) Line 87: && validRemovedNetworkAttachments(attachmentsById) You have a method called validateAndInitRemovedNetworkAttachments() and validRemovedNetworkAttachments(attachmentsById). it is confusing, by the name I understand validRemovedNetworkAttachments should be called via validateAndInitRemovedNetworkAttachments (and as I wrote in a following comment, I think it is what should be done). Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() Line 90: && validateNotRemovingUsedNetworkByVms()) { Line 91: Line 85: && validateAndInitRemovedNetworkAttachments() Line 86: && validModifiedNetworkAttachments(attachmentsById) Line 87: && validRemovedNetworkAttachments(attachmentsById) Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() same comment as the previous. Line 90: && validateNotRemovingUsedNetworkByVms()) { Line 91: Line 92: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 93: if (networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 86: && validModifiedNetworkAttachments(attachmentsById) Line 87: && validRemovedNetworkAttachments(attachmentsById) Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() Line 90: && validateNotRemovingUsedNetworkByVms()) { Shouldn't it be part of validRemovedNetworkAttachments? 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(); Shouldn't this block (except the customProperties validation) of validations be part of validModifiedNetworkAttachments Line 93: if (networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 94: && validate(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Line 95: && validateMtu(attachmentsToConfigure) Line 96: && validateCustomProperties()) { Line 110: Set<Guid> removedBondIds = params.getRemovedBonds(); Line 111: List<VdsNetworkInterface> removedBondInterfaces = new ArrayList<>(removedBondIds.size()); Line 112: if (!Entities.entitiesById(removedBondIds, existingIfaces.values(), removedBondInterfaces)) { Line 113: addViolation(VdcBllMessages.NETWORK_BOND_NOT_EXISTS, null); Line 114: return false; Consider moving this validation to validRemovedBonds() and calling validRemovedBonds() from here. Line 115: } Line 116: Line 117: this.removedBondVdsNetworkInterface = removedBondInterfaces; Line 118: removedBondVdsNetworkInterfaceMap = new BusinessEntityMap<>(removedBondInterfaces); Line 113: addViolation(VdcBllMessages.NETWORK_BOND_NOT_EXISTS, null); Line 114: return false; Line 115: } Line 116: Line 117: this.removedBondVdsNetworkInterface = removedBondInterfaces; The this is redundant. Line 118: removedBondVdsNetworkInterfaceMap = new BusinessEntityMap<>(removedBondInterfaces); Line 119: return true; Line 120: } Line 121: Line 122: public boolean validateAndInitRemovedNetworkAttachments() { Line 123: Set<Guid> removedNetworkAttachmentIds = params.getRemovedNetworkAttachments(); Line 124: List<NetworkAttachment> removedNetworkAttachments = new ArrayList<>(removedNetworkAttachmentIds.size()); Line 125: if (!Entities.entitiesById(removedNetworkAttachmentIds, existingAttachments, removedNetworkAttachments)) { Line 126: addViolation(VdcBllMessages.NETWORK_ATTACHMENT_NOT_EXISTS, null); Same comment as for validateAndInitRemovedBondInterfaces() Line 127: return false; Line 128: } Line 129: Line 130: this.removedNetworkAttachments = removedNetworkAttachments; Line 173: Set<String> requiredNicsNames = getRemovedBondsUsedByNetworks(); Line 174: Line 175: boolean passed = true; Line 176: for (VdsNetworkInterface removedBond : removedBondVdsNetworkInterface) { Line 177: if (removedBond.getName() == null) { How is it possible for the bond name to be null? You fetch the bonds from the db by the id (the name column is marked as not null). Line 178: addViolation(VdcBllMessages.HOST_NETWORK_INTERFACE_NOT_EXIST, null); Line 179: passed = false; Line 180: continue; Line 181: } Line 196: Line 197: return passed; Line 198: } Line 199: Line 200: private Set<String> getRemovedBondsUsedByNetworks() { How is this method related to bonds? But again, I see it was fixed in a following patch... Line 201: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 202: Set<String> requiredNicsNames = new HashSet<>(); Line 203: for (NetworkAttachment attachment : attachmentsToConfigure) { Line 204: requiredNicsNames.add(attachment.getNicName()); 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) { The this is redundant. Line 214: attachmentsToConfigure.remove(removedAttachment.getId()); Line 215: } Line 216: Line 217: List<NetworkAttachment> newAttachments = new ArrayList<>(); Line 224: newAttachments.add(attachment); Line 225: continue; Line 226: } Line 227: Line 228: if (removedBondVdsNetworkInterfaceMap.containsKey(attachment.getNicName())) { I really don't understand this bond related code. As I understand the method should return all the attachments that should exist on the hosts (I mean new + old - removed). I see that it was fixed in a following patch, but it is really hard to review this patch. Why didn't you update the methods in this patch? Is it hard to move the changes to here? Line 229: attachmentsToConfigure.put(attachment.getId(), attachment); Line 230: } else { Line 231: attachmentsToConfigure.remove(attachment.getId()); Line 232: } 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. You also should check it actually exists in the db (or a new bond. Never mind, I see it was added in a following patch... Line 328: && validate(validator.networkIpAddressWasSameAsHostnameAndChanged(getExistingIfaces())) Line 329: && validate(validator.networkNotChanged(attachmentsById.get(attachment.getId()))) Line 330: && validate(validator.validateGateway()))) { Line 331: passed = false; -- 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
