Omer Frenkel has posted comments on this change. Change subject: backend: Control virtio rng device ......................................................................
Patch Set 11: (14 comments) http://gerrit.ovirt.org/#/c/18176/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractRngDeviceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractRngDeviceCommand.java: Line 29: Line 30: @Override Line 31: public List<PermissionSubject> getPermissionCheckSubjects() { Line 32: List<PermissionSubject> permissionList = new ArrayList<PermissionSubject>(); Line 33: permissionList.add(new PermissionSubject(getParameters().getRngDevice().getVmId(), what if getParameters().getRngDevice()==null? Line 34: getParameters().isVm() ? VdcObjectType.VM : VdcObjectType.VmTemplate, Line 35: getActionType().getActionGroup())); Line 36: return permissionList; Line 37: } Line 37: } Line 38: Line 39: @Override Line 40: protected boolean canDoAction() { Line 41: if (getParameters().getRngDevice().getVmId() == null || !entityExists()) { what if getParameters().getRngDevice()==null? Line 42: return failCanDoAction(getParameters().isVm() ? VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND Line 43: : VdcBllMessages.ACTION_TYPE_FAILED_TEMPLATE_DOES_NOT_EXIST); Line 44: } Line 45: Line 71: protected boolean entityExists() { Line 72: Guid entityId = getParameters().getRngDevice().getVmId(); Line 73: Line 74: if (getParameters().isVm()) { Line 75: return getVmDAO().get(entityId) != null; maybe cache the vmBase object and use where ever you can? for example the method above (getClusterForEntity) is doing the same code as in entityExists, including getting the vm/template from db again just to get the cluster. once you know vm/template exist, you can do setVdsGroupId(vmBase.getVdsGroupId()) and then the cluster is cached inside the command in getVdsGroup() Line 76: } else { Line 77: return getVmTemplateDAO().get(entityId) != null; Line 78: } Line 79: } Line 78: } Line 79: } Line 80: Line 81: protected List<VmDevice> getRngDevices() { Line 82: return getVmDeviceDao().getVmDeviceByVmIdAndType(getParameters().getRngDevice().getVmId(), it could be nice to cache the result since you call it twice in remove Line 83: VmDeviceGeneralType.RNG); Line 84: } Line 85: Line 86: protected VmDeviceDAO getVmDeviceDao() { http://gerrit.ovirt.org/#/c/18176/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddRngDeviceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddRngDeviceCommand.java: Line 13: Line 14: @Override Line 15: protected void executeCommand() { Line 16: VmRngDevice rngDevice = getParameters().getRngDevice(); Line 17: if (rngDevice.getDeviceId() == null) { if we allow the user to set the id (or is it set only internally??), we need to make sure its valid and unique Line 18: rngDevice.setDeviceId(Guid.newGuid()); Line 19: } Line 20: getDbFacade().getVmDeviceDao().save(rngDevice); Line 21: setActionReturnValue(rngDevice.getDeviceId()); Line 25: protected boolean canDoAction() { Line 26: if (!super.canDoAction()) { Line 27: return false; Line 28: } Line 29: if (getParameters().getRngDevice() == null) { this should be in super as well.. even first Line 30: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_RNG_NOT_FOUND); Line 31: } Line 32: if (!getRngDevices().isEmpty()) { Line 33: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_RNG_ALREADY_EXISTS); http://gerrit.ovirt.org/#/c/18176/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 737: rngDev.setVmId(getParameters().getVmId()); Line 738: RngDeviceParameters params = new RngDeviceParameters(rngDev, true); Line 739: VdcReturnValueBase result = getBackend().runInternalAction(VdcActionType.AddRngDevice, params); Line 740: if (!result.getSucceeded()) { Line 741: getReturnValue().setSucceeded(false); will this roll back all vm creation? if so, what about created disks? i think that it might belong in the first part of execution where we do db updates Line 742: } Line 743: } Line 744: } Line 745: http://gerrit.ovirt.org/#/c/18176/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetRngDeviceQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetRngDeviceQuery.java: Line 14: } Line 15: Line 16: @Override Line 17: protected void executeQueryCommand() { Line 18: final List<VmDevice> vmDevices = getDbFacade().getVmDeviceDao().getVmDeviceByVmIdAndType(getParameters().getId(), since this is a user query i think you need to use filtering and user id Line 19: VmDeviceGeneralType.RNG); Line 20: Line 21: if (vmDevices != null && !vmDevices.isEmpty()) { Line 22: VmDevice dev = vmDevices.get(0); Line 25: setReturnValue(Collections.emptyList()); Line 26: } Line 27: } Line 28: Line 29: private Integer extractInteger(String intStr) { what is this method and what are you doing with it? Line 30: if (intStr == null) { Line 31: return null; Line 32: } Line 33: return Integer.parseInt(intStr); http://gerrit.ovirt.org/#/c/18176/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java: Line 114: @SuppressWarnings("unchecked") Line 115: List<VmRngDevice> rngDevs = query.getReturnValue(); Line 116: Line 117: if (getParameters().getRngDevice() != null) { Line 118: getParameters().getRngDevice().setVmId(getParameters().getVmId()); if you get the device by vmId, why you need to set it here? Line 119: } Line 120: Line 121: VdcReturnValueBase rngCommandResult = null; Line 122: if (rngDevs.isEmpty()) { http://gerrit.ovirt.org/#/c/18176/11/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateRngDeviceTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateRngDeviceTest.java: Line 15: //import org.ovirt.engine.core.dao.VdsGroupDAO; Line 16: //import org.ovirt.engine.core.dao.VmDAO; Line 17: //import org.ovirt.engine.core.dao.VmDeviceDAO; Line 18: // Line 19: //public class UpdateRngDeviceTest { so it working? maybe add a comment? :) Line 20: // Line 21: // @Test Line 22: // public void testCanDoAction() { Line 23: // UpdateRngDeviceCommand command = mockCommand(Version.v3_3); http://gerrit.ovirt.org/#/c/18176/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 615: ACTION_TYPE_FAILED_BOOKMARK_NAME_ALREADY_EXISTS(ErrorType.CONFLICT), Line 616: ACTION_TYPE_FAILED_BOOKMARK_INVALID_ID(ErrorType.BAD_PARAMETERS), Line 617: VDS_FENCE_OPERATION_FAILED(ErrorType.CONFLICT), Line 618: ACTION_NOT_SUPPORTED_FOR_CLUSTER_POOL_LEVEL(ErrorType.INCOMPATIBLE_VERSION), Line 619: ACTION_NOT_SUPPORTED_FOR_CLUSTER_LEVEL(ErrorType.INCOMPATIBLE_VERSION), is this in use? i didnt see you add translation to it Line 620: CAN_DO_ACTION_GENERAL_FAILURE(ErrorType.INTERNAL_ERROR), Line 621: CAN_DO_ACTION_DATABASE_CONNECTION_FAILURE(ErrorType.INTERNAL_ERROR), Line 622: /** @deprecated as it is too general error message */ Line 623: @Deprecated http://gerrit.ovirt.org/#/c/18176/11/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 1152: ISCSI_BOND_WITH_SAME_NAME_EXIST_IN_DATA_CENTER=Cannot ${action} ${type}. iSCSI bond with the same name already exists in the Data Center. Line 1153: Line 1154: ACTION_TYPE_FAILED_IDE_INTERFACE_DOES_NOT_SUPPORT_READ_ONLY_ATTR=Cannot ${action} ${type}. An IDE disk can't be read-only. Line 1155: Line 1156: # RNG device what about appErrors.java and appErrors.props in webadmin/userportal? Line 1157: ACTION_TYPE_FAILED_RNG_NOT_FOUND=Cannot ${action} ${type}. Trying to manipulate with Random Number Generator device but none is found. Line 1158: ACTION_TYPE_FAILED_RNG_ALREADY_EXISTS=Cannot ${action} ${type}. Random Number Generator device already exists. http://gerrit.ovirt.org/#/c/18176/11/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java: Line 387: vds.setIScsiInitiatorName(AssignStringValue(xmlRpcStruct, VdsProperties.iSCSIInitiatorName)); Line 388: Line 389: vds.setSupportedEmulatedMachines(AssignStringValueFromArray(xmlRpcStruct, VdsProperties.emulatedMachines)); Line 390: Line 391: // todo process rngSources too when you plan to do this? Line 392: Line 393: String hooksStr = ""; // default value if hooks is not in the xml rpc struct Line 394: if (xmlRpcStruct.containsKey(VdsProperties.hooks)) { Line 395: hooksStr = xmlRpcStruct.get(VdsProperties.hooks).toString(); -- To view, visit http://gerrit.ovirt.org/18176 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc26a484c84244074426f4b0c790e2cc4ebb8da6 Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Itamar Heim <ih...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@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
