Alona Kaplan has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 26: (7 comments) General comments- 1. Some of the classes in this patch were refactored (including bug fixes) in the following patches. If it is possible and not too complicate please move the fixes to this patch. If it is hard, please move the patches with the fixes to be the next patches in the flow (currently there are >10 patches between this one and the refactoring). 2. Please move the patches with the tests for HostSetupNetworksValidator, HostNetworkInterfacesPersister, HostNetworkAttachmentsPersister, HostNetworkTopologyPersisterImpl as close as possible to this patch. 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 94: @Override Line 95: protected boolean canDoAction() { Line 96: VDS host = getVds(); Line 97: Line 98: if (host == null) { Consider adding these two validations (host exists and in a supported status) to HostSetupNetworksValidator. You have these two validations in all networAttachment related actions- HostSetupNetworks, Add/Update/RemoveNetworkAttachment. Line 99: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST); Line 100: return false; Line 101: } Line 102: 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 312: Line 313: return false; Line 314: } Line 315: Line 316: private boolean validModifiedNetworkAttachments(Map<Guid, NetworkAttachment> attachmentsById) { General question- what is the correct way to move network from one nic to another? 1. Changing the nicId on an existing attachment? 2. Create a new attachment for the new nic and network and remove the previous one? Are both of the ways are accepted? If not, please block the one isn't accepted. 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(); 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'm not sure, why do you need this list? Line 39: Line 40: reportedNetworksById = new HashMap<>(); Line 41: reportedNicsByNetworkId = new HashMap<>(); Line 42: for (VdsNetworkInterface nic : nics) { Line 59: /** Line 60: * Adds new network attachments for reported networks from the host which weren't associated to a user attachment.<br> Line 61: * The network attachment's attributes will be inherited from the network interface on which it is configured. Line 62: */ Line 63: private void addNewNetworkAttachments() { I'm not sure if it possible. But if you somehow get a network defined on top of bond's slave you will create an a NetworkAttachement for it, I think you should block it. Line 64: for (VdsNetworkInterface nic : nicsByName.values()) { Line 65: String networkName = nic.getNetworkName(); Line 66: if (networkName == null Line 67: || !clusterNetworks.containsKey(networkName) 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: } The user defined attachments passed to this class are taken from HostSetupNetworks' parameters. It means that just attachments that were added or changed will be passed. For all the others you are creating new attachments, but most of them (in most of the cases I think- all of them) contain existing attachment. I saw you fixed this issue in a future patch. If it not too complicated, please move it to this patch. Line 80: } Line 81: Line 82: Line 83: private boolean networkAttachmentExistsForNetwork(String networkName) { https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkInterfacesPersisterImpl.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkInterfacesPersisterImpl.java: Line 53: private void updateModifiedInterfaces() { Line 54: List<VdsNetworkInterface> nicsForUpdate = prepareNicsForUpdate(); Line 55: if (!nicsForUpdate.isEmpty()) { Line 56: interfaceDao.massUpdateInterfacesForVds(getNicsForUpdate()); Line 57: ? Line 58: } Line 59: } Line 60: Line 61: private void createNewInterfaces() { https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkTopologyPersisterImpl.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkTopologyPersisterImpl.java: Line 255: * the host for which the network topology should be persisted and contains the list of the reported nics Line 256: * @param dbNics Line 257: * network interfaces from the database prior to vdsm report Line 258: * @param clusterNetworks Line 259: * the networks which assigned to the host's clusters s/clusters/cluster Line 260: * @param userConfiguredNetworkData Line 261: * The network configuration as provided by the user, for which engine managed data will be preserved. Line 262: */ Line 263: private void persistTopology(VDS host, -- 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
