Mike Kolesnik has posted comments on this change. Change subject: engine: Support attaching two labeled networks to a cluster ......................................................................
Patch Set 1: Code-Review-1 (6 comments) There are several errors in the code. Besides that, this solution doesn't cover a case where a user chooses to detach multiple network, or when he is trying to attach some networks and detach others that are managed by labels, in the same operation. For this we can either block such an action (attaching & detaching simultaneously). Alternatively, you can introduce a sort of "SetupClusterNetworks" command that can run the correct setup networks command according to the will of the user. http://gerrit.ovirt.org/#/c/23407/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunner.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunner.java: Line 186: public void setIsRunOnlyIfAllCanDoPass(boolean isRunOnlyIfAllCanDoPass) { Line 187: this.isRunOnlyIfAllCanDoPass = isRunOnlyIfAllCanDoPass; Line 188: } Line 189: Line 190: protected VdcActionType getActionType() { Not sure why you need this Line 191: return actionType; Line 192: } http://gerrit.ovirt.org/#/c/23407/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/NetworkClusterAttachmentActionRunner.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/NetworkClusterAttachmentActionRunner.java: Line 47: } Line 48: Line 49: // multiple networks can be either attached or detached from a single cluster Line 50: if (!params.isEmpty()) { Line 51: Backend.getInstance().runInternalAction(getActionType(), Pretty sure you meant to send your new command type here.. Line 52: new ClusterNetworksParameters(params.get(0).getVdsGroupId(), params)); Line 53: } Line 54: } http://gerrit.ovirt.org/#/c/23407/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/AddNetworksByLabelParametersBuilder.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/AddNetworksByLabelParametersBuilder.java: Line 54: throw new VdcBLLException(VdcBllErrors.LABELED_NETWORK_INTERFACE_NOT_FOUND); Line 55: } Line 56: Line 57: // configure the network on the nic Line 58: parameters.getInterfaces().addAll(configureNetworks(nicToConfigure, Collections.singleton(network))); Not sure how this will configure the network on a NIC that already has some other networks (either added there by a label, or just by the user). Line 59: } Line 60: Line 61: return parameters; Line 62: } http://gerrit.ovirt.org/#/c/23407/1/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 22: import org.ovirt.engine.core.compat.Guid; Line 23: import org.ovirt.engine.core.utils.transaction.TransactionMethod; Line 24: import org.ovirt.engine.core.utils.transaction.TransactionSupport; Line 25: Line 26: @NonTransactiveCommandAttribute Do you want to mark as @InternalCommand? Line 27: public class AttachNetworksToClusterCommand<T extends ClusterNetworksParameters> extends CommandBase<T> { Line 28: Line 29: public AttachNetworksToClusterCommand(T parameters) { Line 30: super(parameters); Line 30: super(parameters); Line 31: } Line 32: Line 33: @Override Line 34: protected void setActionMessageParameters() { Why do you need this? Line 35: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__NETWORK); Line 36: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__ATTACH); Line 37: } Line 38: Line 89: Map<Guid, Map<String, VdsNetworkInterface>> labelsToNicsByHost) { Line 90: ArrayList<VdcActionParametersBase> parameters = new ArrayList<>(); Line 91: for (Guid hostId : networksByHost.keySet()) { Line 92: AddNetworksByLabelParametersBuilder builder = new AddNetworksByLabelParametersBuilder(); Line 93: builder.buildParameters(hostId, networksByHost.get(hostId), labelsToNicsByHost.get(hostId)); Shouldn't you save the returned parameters in the list? Line 94: } Line 95: Line 96: getBackend().runInternalMultipleActions(VdcActionType.PersistentSetupNetworks, parameters); Line 97: } -- To view, visit http://gerrit.ovirt.org/23407 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I87194f5c6a382ec49c86cbd26a24ce2aaccb08c9 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
