Alona Kaplan has posted comments on this change. Change subject: engine: Refactor AttachNetworksToClusterCommand ......................................................................
Patch Set 9: (25 comments) http://gerrit.ovirt.org/#/c/33684/9//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-10-01 22:36:40 +0300 Line 4: Commit: Yevgeny Zaspitsky <[email protected]> Line 5: CommitDate: 2014-10-21 11:46:01 +0300 Line 6: Line 7: engine: Refactor AttachNetworksToClusterCommand p.s- what about the detach (multiple and single)? didn't see any patch related to it. Line 8: Line 9: Make network management network attacments be processed first. Line 10: Refactor AttachNetworkToVdsGroupCommand: split the command into Line 11: 2 internal commands: http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToClusterCommand.java: Line 22: import org.ovirt.engine.core.dao.VmDAO; Line 23: import org.ovirt.engine.core.dao.network.NetworkClusterDao; Line 24: Line 25: @InternalCommandAttribute Line 26: public class AttachNetworkToClusterCommand<T extends AttachNetworkToVdsGroupParameter> extends Please rename the class. You have AttachNetworkToVdsGroup and now you call this class AttachNetworkToCluster, it is really confusing since the names are synonymous. Consider calling the class- AttachNetworkToClusterInternalCommand. Line 27: NetworkClusterCommandBase<T> { Line 28: Line 29: AttachNetworkToClusterCommand(T parameters) { Line 30: super(parameters, null); Line 42: setSucceeded(true); Line 43: } Line 44: Line 45: @Override Line 46: protected boolean canDoAction() { Moving the canDoAction to here it problematic. Since the ui executes- runMultipleAction(VdcActionType.AttachNetworkToVdsGroup) the can do actions that are returned and displayed in the ui are the the canDoActions of AttachNetworkToVdsGroup. Since in running mulitiple action the action returns after executing all the canDos, the canDos of AttachNetworkToClusterCommand (which is ran internally from AttachNetworkToVds) will never be displayed. Line 47: return networkNotAttachedToCluster() Line 48: && vdsGroupExists() Line 49: && changesAreClusterCompatible() Line 50: && logicalNetworkExists() http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java: Line 8: import org.ovirt.engine.core.common.action.VdcActionParametersBase; Line 9: import org.ovirt.engine.core.common.action.VdcActionType; Line 10: import org.ovirt.engine.core.common.action.VdcReturnValueBase; Line 11: import org.ovirt.engine.core.compat.TransactionScopeOption; Line 12: Why should this command be transactive? The only transactive part is calling internal command- AttachNetworkToVdsGroup. You can pass parameters.setTransactionScopeOption(TransactionScopeOption.RequiredNew) to the command to make sure it runs in new transaction. Line 13: public class AttachNetworkToVdsGroupCommand<T extends AttachNetworkToVdsGroupParameter> extends VdsGroupCommandBase<T> { Line 14: Line 15: public AttachNetworkToVdsGroupCommand(T parameters, CommandContext cmdContext) { Line 16: super(parameters, cmdContext); Line 28: parameters.setTransactionScopeOption(TransactionScopeOption.Required) Should be changed to- RequiredNew. Please see the comment on the class declaration. Line 25: Line 26: final T parameters = getParameters(); Line 27: Line 28: parameters.setTransactionScopeOption(TransactionScopeOption.Required); Line 29: parameters.setCompensationEnabled(false); Why do you need this? Line 30: Line 31: final VdcReturnValueBase returnValue = Line 32: getBackend().runInternalAction(VdcActionType.AttachNetworkToVdsGroup, parameters); Line 33: Line 28: parameters.setTransactionScopeOption(TransactionScopeOption.Required); Line 29: parameters.setCompensationEnabled(false); Line 30: Line 31: final VdcReturnValueBase returnValue = Line 32: getBackend().runInternalAction(VdcActionType.AttachNetworkToVdsGroup, parameters); The CommandBase has runInternalAction method. It should be called in order to execute internal method. http://www.ovirt.org/Wiki/Engine_Command_changes - point 2. Line 33: Line 34: final boolean attachSucceed = returnValue.getSucceeded(); Line 35: setSucceeded(attachSucceed); Line 36: Line 41: Line 42: private void attachLabeledNetworks() { Line 43: final T parameters = getParameters(); Line 44: Line 45: parameters.setTransactionScopeOption(TransactionScopeOption.Suppress); Can be removed. Please see the comment on the class declaration. Line 46: parameters.setCompensationEnabled(false); Line 47: Line 48: // AttachLabeledNetworks should be run asynchronously Line 49: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 42: private void attachLabeledNetworks() { Line 43: final T parameters = getParameters(); Line 44: Line 45: parameters.setTransactionScopeOption(TransactionScopeOption.Suppress); Line 46: parameters.setCompensationEnabled(false); Again- why do you need this? Line 47: Line 48: // AttachLabeledNetworks should be run asynchronously Line 49: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 50: createParametersArrayList(parameters)); Line 46: parameters.setCompensationEnabled(false); Line 47: Line 48: // AttachLabeledNetworks should be run asynchronously Line 49: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 50: createParametersArrayList(parameters)); Why do you run this command as multiple actions? Line 51: } Line 52: Line 53: private ArrayList<VdcActionParametersBase> createParametersArrayList(final T parameters) { Line 54: final ArrayList<VdcActionParametersBase> resultList = new ArrayList<>(); Line 49: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 50: createParametersArrayList(parameters)); Line 51: } Line 52: Line 53: private ArrayList<VdcActionParametersBase> createParametersArrayList(final T parameters) { Same- as I understand there is no need to run the command as multiple action, it is only a single command. So there is no need to put the parameters in a list. Line 54: final ArrayList<VdcActionParametersBase> resultList = new ArrayList<>(); Line 55: resultList.add(parameters); Line 56: Line 57: return resultList; http://gerrit.ovirt.org/#/c/33684/9/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 36: protected void executeCommand() { Line 37: Line 38: final boolean attachNetworksToClusterResult = attachNetworksToCluster(); Line 39: Line 40: setSucceeded(attachNetworksToClusterResult); Shouldn't you return in case attachNetworksToClusterResult is false? (although I think you should run PropagateLabeledNetworksToClusterHosts just on the parameters that their AttachNetworkToCluster action has passed). Line 41: Line 42: // PropagateLabeledNetworksToClusterHosts should be run asynchronously Line 43: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 44: wrapParametersWithList(getParameters())); Line 39: Line 40: setSucceeded(attachNetworksToClusterResult); Line 41: Line 42: // PropagateLabeledNetworksToClusterHosts should be run asynchronously Line 43: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, If you have just one parameter, why do you run it a multiple action? Line 44: wrapParametersWithList(getParameters())); Line 45: } Line 46: Line 47: private ArrayList<VdcActionParametersBase> wrapParametersWithList(T parameters) { Line 58: private boolean attachNetworksToCluster() { Line 59: final List<AttachNetworkToVdsGroupParameter> clusterNetworksParameters = Line 60: getParameters().getClusterNetworksParameters(); Line 61: Line 62: Collections.sort(clusterNetworksParameters, setManagementNetworkFirstParameterComparator); This sort is not enough. When the ui manages the network in a cluster he is performing 3 multiple actions- attach, detach and update. So sorting just the attaches is not enough. Mabe the new management network is set via the update. I can think of two options to solve the issue- 1. Sorting in the ui side- first updating the management network attachment. On the success of this update- doing all the rest, same as before. 2. Adding a new backend command for manageNetworkCluster- it will be invoked from the ui instead of the three multiple actions, and will do the sorting described in 1 internally. Line 63: Line 64: return runInternalActionsSequentiallyAndTransactionally( Line 65: VdcActionType.AttachNetworkToCluster, Line 66: clusterNetworksParameters); Line 75: * @param parameters Line 76: * parameters list. The size of the {@link List} defines the number of iteration on the action. Line 77: * @return <code>TRUE</code> on success otherwise <code>FALSE</code>. Line 78: */ Line 79: private <P extends VdcActionParametersBase> boolean runInternalActionsSequentiallyAndTransactionally(final VdcActionType actionType, Why do you check that all can do action pass a rollback otherwise? The updates and detaches which are done from the same dialog are ran as multiple action and don't check it. You should keep the same behaviour here, or fix the update, detach behaviour. You can use (add overload to runMultipleActions)- runMultipleActionsImpl(VdcActionType actionType, ArrayList<VdcActionParametersBase> parameters, boolean isInternal= true, boolean isRunOnlyIfAllCanDoPass=false, boolean isWaitForResult=true, CommandContext commandContext) To keep all the actions in one transaction you can keep the TransactionSupport.executeInNewTransaction and pass Transsaction.Required to the parameters. Although I am not sure why do you need to have one transaction for all the attaches. You can make the AttachNetworkToCluster transactional and remove the transaction treatment (when calling to AttachNetworkToCluster) from AttachNetworkToVdsGroup and from here. Line 80: final List<P> parameters) { Line 81: return TransactionSupport.executeInNewTransaction(new TransactionMethod<Boolean>() { Line 82: Line 83: @Override Line 80: parameters Please format. http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterCommandBase.java: Line 16: } Line 17: Line 18: protected NetworkClusterCommandBase(T parameters) { Line 19: super(parameters); Line 20: setVdsGroupId(parameters.getVdsGroupId()); You've added this line to VdsGroupCommandBase, so you don't need it here any more. Line 21: } Line 22: Line 23: protected NetworkCluster getNetworkCluster() { Line 24: return getParameters().getNetworkCluster(); Line 40: && validate(validator.externalNetworkNotDisplay(getNetworkName())) Line 41: && validate(validator.externalNetworkNotRequired(getNetworkName())); Line 42: } Line 43: Line 44: protected Version getClusterVersion() { You should remove the override of this method from UpdateNetworkOnClusterCommand. Line 45: return getVdsGroup().getcompatibility_version(); Line 46: } Line 47: Line 48: protected boolean validateAttachment() { http://gerrit.ovirt.org/#/c/33684/9/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 Please format. Line 77: return networks; Line 78: } Line 79: Line 80: private void configureNetworksOnHosts(Map<Guid, List<Network>> networksByHost, Line 81: Map<Guid, Map<String, VdsNetworkInterface>> labelsToNicsByHost) { please format Line 82: ArrayList<VdcActionParametersBase> parameters = new ArrayList<>(); Line 83: for (Entry<Guid, List<Network>> entry : networksByHost.entrySet()) { Line 84: AddNetworksByLabelParametersBuilder builder = new AddNetworksByLabelParametersBuilder(getContext()); Line 85: final Guid hostId = entry.getKey(); http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/SetManagementNetworkFirstParameterComparator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/SetManagementNetworkFirstParameterComparator.java: Line 1: package org.ovirt.engine.core.bll.network.cluster; Please move to org.ovirt.engine.core.common.businessentities.comparators Line 2: Line 3: import static org.apache.commons.lang.BooleanUtils.toInteger; Line 4: Line 5: import java.util.Comparator; Line 8: import org.ovirt.engine.core.common.businessentities.network.Network; Line 9: import org.ovirt.engine.core.common.businessentities.network.NetworkCluster; Line 10: Line 11: final class SetManagementNetworkFirstParameterComparator<T extends AttachNetworkToVdsGroupParameter> implements Line 12: Comparator<T> { Please format the class. Line 13: @Override Line 14: public int compare(T param1, T param2) { Line 15: final boolean management1 = isManagementNetworkAppointmentCommand(param1); Line 16: final boolean management2 = isManagementNetworkAppointmentCommand(param2); Line 14: public int compare(T param1, T param2) { Line 15: final boolean management1 = isManagementNetworkAppointmentCommand(param1); Line 16: final boolean management2 = isManagementNetworkAppointmentCommand(param2); Line 17: Line 18: return toInteger(management2) - toInteger(management1); 1. Shouldn't you return toInteger(management1) - toInteger(management2)? You want the management network to be first. It means the management network must be greater than other network. You're returning the opposite. 2. But either way, I would return Boolean.compare(management1, management2) Line 19: } Line 20: Line 21: private boolean isManagementNetworkAppointmentCommand(T param) { Line 22: final Network network = param.getNetwork(); Line 17: Line 18: return toInteger(management2) - toInteger(management1); Line 19: } Line 20: Line 21: private boolean isManagementNetworkAppointmentCommand(T param) { Maybe- isManagementNetworkParameter? Line 22: final Network network = param.getNetwork(); Line 23: if (network == null) { Line 24: return false; Line 25: } Line 18: return toInteger(management2) - toInteger(management1); Line 19: } Line 20: Line 21: private boolean isManagementNetworkAppointmentCommand(T param) { Line 22: final Network network = param.getNetwork(); You don't need this check, you can call directy- param.getNetworkCluster() Line 23: if (network == null) { Line 24: return false; Line 25: } Line 26: -- 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: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [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
