Alona Kaplan has posted comments on this change. Change subject: engine: refactor manage networks flow ......................................................................
Patch Set 31: (6 comments) Publishing the comments so far. The review is not finished yet... http://gerrit.ovirt.org/#/c/33684/31//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-11-11 19:11:34 +0200 Line 4: Commit: Yevgeny Zaspitsky <[email protected]> Line 5: CommitDate: 2014-12-28 19:18:24 +0200 Line 6: Line 7: engine: refactor manage networks flow This patch contains also webadmin code. Please update the comment prefix to "engine,webadmin:" Line 8: Line 9: Refactor manage networks flow. Line 10: Introduce ManageNetworkClustersCommand for that purpose. Line 11: That replaces the following classes: Line 10: Introduce ManageNetworkClustersCommand for that purpose. Line 11: That replaces the following classes: Line 12: * DetachNetworkFromClusterMultipleActionRunner Line 13: * AttachNetworksToClusterCommand Line 14: * DetachNetworksFromClusterCommand What about UpdateNetworkOnClusterCommand? Doesn't it replace it? Line 15: Line 16: Refactor (At|De)tacahNetworkToVdsGroupCommand: implement them as Line 17: composition of AttachNetworkToClusterIntenalCommand or Line 18: DetachNetworkFromClusterInternalCommand and http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java: Line 1043: } Line 1044: return true; Line 1045: } Line 1046: Line 1047: final public boolean checkSinglePermission(PermissionSubject permSubject, Collection<String> messages) { IIUC, you need all the permission related code in this patch because you need a combined permissions check for ManageNetworkClustersCommand, since the Attach, Detach and Update are called internally and internal command skip the permissions check. Why not adding to CommandBase an option to run an internal command without skipping the permissions check? You can add a new parameter- CommandBase.forcePremissionCheck which is false by default. overload CommandBase.runInternalAction- CommandBase.runInternalAction(VdcActionType actionType, VdcActionParametersBase parameters, boolean forcePremissionCheck) Modify- CommandBase.isUserAuthorizedToRunAction to skip permissions check just in case of isInternalExecution() && !forcePremissionCheck This way, you can use your new runInternalAction with permission check and omit all the permission related code in this patch. Line 1048: final Guid objectId = permSubject.getObjectId(); Line 1049: final VdcObjectType objectType = permSubject.getObjectType(); Line 1050: final ActionGroup objectActionGroup = permSubject.getActionGroup(); Line 1051: http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunnersFactory.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunnersFactory.java: Line 71: break; Line 72: } Line 73: Line 74: case AttachNetworkToVdsGroup: Line 75: case DetachNetworkToVdsGroup: What about updateNetworkOnCluster? Line 76: throw new UnsupportedOperationException("Multiple network attachments/detachments should be run through ManageNetworkClustersCommand!"); Line 77: Line 78: default: Line 79: runner = new MultipleActionsRunner(actionType, parameters, commandContext, isInternal); Line 72: } Line 73: Line 74: case AttachNetworkToVdsGroup: Line 75: case DetachNetworkToVdsGroup: Line 76: throw new UnsupportedOperationException("Multiple network attachments/detachments should be run through ManageNetworkClustersCommand!"); same- what about update? Line 77: Line 78: default: Line 79: runner = new MultipleActionsRunner(actionType, parameters, commandContext, isInternal); Line 80: break; http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/ManageLabeledNetworksParametersBuilderImpl.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/ManageLabeledNetworksParametersBuilderImpl.java: Line 58: final Set<VdsNetworkInterface> interfacesRemainedAfterRemove = Line 59: new HashSet<>(removeSetupNetworksParameters.getInterfaces()); Line 60: for (VdsNetworkInterface nic : addSetupNetworksParameters.getInterfaces()) { Line 61: if (interfacesRemainedAfterRemove.contains(nic) || !originalInterfaces.contains(nic)) { Line 62: combinedInterfaces.add(nic); It seems there is a bug here. If a non-vlan labeled network was removed, it won't be reflected in the combined result. 1. If the networkName of the nic equals to the network name of the same nic in the original list=> you should take the network name from the AfterRemove list. 2. Otherwise, you should take the network name from, the add list (it means you can add the nic as is, you don't need to modify the network name). Line 63: } Line 64: } Line 65: return resultParam; Line 66: } -- 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: 31 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: Yevgeny Zaspitsky <[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
