Frank Kobzik has posted comments on this change. Change subject: backend: Control virtio rng device ......................................................................
Patch Set 11: (14 comments) (posting 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? Null doesn't make sense - if you use *RngCommand classes, you manipulate RNG device and you must fill it in. But maybe I should add check for null in the constructor. I'd like to forbid the parameterless constructor, but I think it's needed for GWT. 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? dtto 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 m thanks. 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 Done 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 nee i saw this approach in other parts of the code (AddDiskCommand). I'd prefer generating unique id on db level. I'll explore the possibilities. for now, i'm keeping it this way. 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 i changed the behavior to enforce the rng device in params not to be null. 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? I see. Now it depends what do we wanna do when adding rng fails. We can a) silently go on (watchdog approach) b) rollback the transaction (as you suggest). I think we should go with b), I changed the code, but I suppose I'm doing it wrong. 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 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? not needed anymore. 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? this is indeed pointless. I've moved setting of id to setter of parameters base classes. 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? my mistake - i only checked state of this file in follow up patchset. done. 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/businessentities/VmRngDevice.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmRngDevice.java: Line 23: private Source(String val) { Line 24: this.val = val; Line 25: } Line 26: Line 27: public static Source fromString(String str) { > iiuc, this method is not needed, you can do Source.valueOf(str) and get the yes, but it only works for 1:1 representation of enum names (e.g. "RANDOM", not "random"). What I do here is converting to enum using it's "backing" representation. I think this approach is a little bit more flexible as it allows custom implementation of the converting method (we can decide to ignore case for instance.). But if you want to use valueOf (and capital letters in db), just let me know, I'll do it. Line 28: for (Source val : values()) { Line 29: if (val.toString().equals(str)) { Line 30: return val; Line 31: } 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 I'll use the one on line 618. According to the message this is what i need. 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? I believe the keys are present there. 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? in a follow up (the patch "rng sources reporting") 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
