Alona Kaplan has posted comments on this change. Change subject: engine: Refactor AttachNetworksToClusterCommand ......................................................................
Patch Set 22: (5 comments) http://gerrit.ovirt.org/#/c/33684/22/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworksToClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworksToClusterCommand.java: Line 29: Line 30: // PropagateLabeledNetworksToClusterHosts should be run asynchronously Line 31: runInternalActionAsynchronously( Line 32: VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 33: new ClusterNetworksParameters(getParameters().getVdsGroupId(), successfullyProcessedParameters)); Since multiple action on attach is now blocked. This command should support attaching networks from different cluster as well. In case of different clusters you pass null as the vdsGroupId. As I wrote in the ui code, it is bug prone. You should create a new parameter class for this command, which doesn't extend VdsGroupParametersBase. As I see you have two options- 1. Fix this and PropagateLabeledNetworksToClusterHosts to support multiple clusters. 2. 2.1 Change the parameter of this commands to List<AttachNetworkToVdsGroupParameter>. 2.2 Leaving PropagateLabeledNetworksToClusterHosts sd it and calling it separately for each cluster (using runMultipleAction). I prefer approach 2. Line 34: } Line 35: Line 36: /** Line 37: * Updates the engine DB with the new network attachments. Line 37: * Updates the engine DB with the new network attachments. Line 38: * Line 39: * @return <code>true</code> on success otherwise <code>false</code>. Line 40: */ Line 41: private List<AttachNetworkToVdsGroupParameter> attachNetworksToCluster() { should be- attachNetworksToClusters() Line 42: Line 43: return runMultipleInternalActionsSequentially( Line 44: VdcActionType.AttachNetworkToClusterInternal, Line 45: getParameters().getClusterNetworksParameters()); http://gerrit.ovirt.org/#/c/33684/22/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/PropagateLabeledNetworksToClusterHostsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/PropagateLabeledNetworksToClusterHostsCommand.java: Line 30: VdsGroupCommandBase You can't extends vds group command base since this action is not per vdsGroup. Line 39: Line 40: @Override Line 41: protected void executeCommand() { Line 42: Line 43: if (NetworkHelper.setupNetworkSupported(getVdsGroup().getcompatibility_version())) { In case of attaching networks form different clusters you will get NPE here. Line 44: setupNetworksOnHosts(); Line 45: } Line 46: Line 47: setSucceeded(true); Line 52: final Map<Guid, Map<String, VdsNetworkInterface>> labelsToNicsByHost = new HashMap<>(); Line 53: Line 54: for (Network network : getNetworks()) { Line 55: List<VdsNetworkInterface> nics = getInterfaceDao().getAllInterfacesByLabelForCluster( Line 56: getVdsGroupId(), In case of attaching networks form different clusters you will get empty list here which will cause not don't the setup networks at all. Line 57: network.getLabel()); Line 58: Line 59: for (VdsNetworkInterface nic : nics) { Line 60: if (!networksByHost.containsKey(nic.getVdsId())) { -- To view, visit http://gerrit.ovirt.org/33684 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a0b86d8bb018a6172891cb357a4402cfef135d1 Gerrit-PatchSet: 22 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
