Chris Morrissey has posted comments on this change.
Change subject: core: Added RegisterDiskCommand and GetUnregisteredDisksQuery
......................................................................
Patch Set 2: (19 inline comments)
Responded to comments
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUnregisteredDiskQuery.java
Line 28: getQueryReturnValue().setSucceeded(true);
Line 29: }
Line 30: }
Line 31:
Line 32: public DiskImage populateDisk(BackendInternal backend, Guid
storagePoolId, Guid storageDomainId, Guid diskId) {
Weird, could have sworn I changed that. Will be private in the next patch.
Line 33:
Line 34: // Now get the list of volumes for each new image.
Line 35: StoragePoolDomainAndGroupIdBaseVDSCommandParameters
getVolumesParameters = new StoragePoolDomainAndGroupIdBaseVDSCommandParameters(
Line 36: storagePoolId, storageDomainId, diskId);
Line 28: getQueryReturnValue().setSucceeded(true);
Line 29: }
Line 30: }
Line 31:
Line 32: public DiskImage populateDisk(BackendInternal backend, Guid
storagePoolId, Guid storageDomainId, Guid diskId) {
That was actually an artifact of an earlier version. You're right in that it's
not needed here. Will be removed in next patch.
Line 33:
Line 34: // Now get the list of volumes for each new image.
Line 35: StoragePoolDomainAndGroupIdBaseVDSCommandParameters
getVolumesParameters = new StoragePoolDomainAndGroupIdBaseVDSCommandParameters(
Line 36: storagePoolId, storageDomainId, diskId);
Line 35: StoragePoolDomainAndGroupIdBaseVDSCommandParameters
getVolumesParameters = new StoragePoolDomainAndGroupIdBaseVDSCommandParameters(
Line 36: storagePoolId, storageDomainId, diskId);
Line 37: VDSReturnValue volumesListReturn =
backend.getResourceManager().RunVdsCommand(VDSCommandType.GetVolumesList,
Line 38: getVolumesParameters);
Line 39: if (volumesListReturn.getSucceeded()) {
I had actually written up this block of code as you have suggested but chose to
keep it this way as I thought it looked easier to follow. Investigating online
showed that this is really a matter of taste. However, I don't want to get in a
battle over this and it appears to be the consensus here to go your route, so
I'll code it that way in the next patch.
Line 40: @SuppressWarnings("unchecked")
Line 41: List<Guid> volumesList = (List<Guid>)
volumesListReturn.getReturnValue();
Line 42:
Line 43: // We can't deal with snapshots, so there should only be a
single volume associated with the
Line 41: List<Guid> volumesList = (List<Guid>)
volumesListReturn.getReturnValue();
Line 42:
Line 43: // We can't deal with snapshots, so there should only be a
single volume associated with the
Line 44: // image. If there are multiple volumes, skip the image
and move on to the next.
Line 45: if (volumesList.size() == 1) {
Will do as stated above.
Line 46: Guid volumeId = volumesList.get(0);
Line 47:
Line 48: // Get the information about the volume from VDSM.
Line 49: GetImageInfoVDSCommandParameters imageInfoParameters =
new GetImageInfoVDSCommandParameters(
Line 61: } else {
Line 62: RuntimeException exc =
imageInfoReturn.getExceptionObject();
Line 63: if (exc != null) {
Line 64: throw exc;
Line 65: }
Makes sense. I'll set the exception string on the return value as well.
Line 66: }
Line 67: }
Line 68: } else {
Line 69: RuntimeException exc =
volumesListReturn.getExceptionObject();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RegisterDiskCommand.java
Line 11: import
org.ovirt.engine.core.common.businessentities.Disk.DiskStorageType;
Line 12: import org.ovirt.engine.core.compat.Guid;
Line 13: import org.ovirt.engine.core.dal.VdcBllMessages;
Line 14:
Line 15: @InternalCommandAttribute
This was copied from another command. Will remove in next patch.
Line 16: @NonTransactiveCommandAttribute(forceCompensation=true)
Line 17: public class RegisterDiskCommand <T extends RegisterDiskParameters>
extends BaseImagesCommand<T> implements QuotaStorageDependent {
Line 18:
Line 19: private static final long serialVersionUID = -1201881996330878181L;
Line 12: import org.ovirt.engine.core.compat.Guid;
Line 13: import org.ovirt.engine.core.dal.VdcBllMessages;
Line 14:
Line 15: @InternalCommandAttribute
Line 16: @NonTransactiveCommandAttribute(forceCompensation=true)
You're right. This was copied from another command. Will remove in next patch.
Line 17: public class RegisterDiskCommand <T extends RegisterDiskParameters>
extends BaseImagesCommand<T> implements QuotaStorageDependent {
Line 18:
Line 19: private static final long serialVersionUID = -1201881996330878181L;
Line 20:
Line 32: @Override
Line 33: protected boolean canDoAction() {
Line 34: // Currently this only supports importing images and does not
work with luns.
Line 35: if (getParameters().getDiskImage().getDiskStorageType() !=
DiskStorageType.IMAGE)
Line 36: {
Yes, I normally do that. Not sure why I had it on a separate line here. Will
move up for next patch.
Line 37: addCanDoActionMessage("$diskId " +
getParameters().getDiskImage().getId());
Line 38: addCanDoActionMessage("$storageType " +
getParameters().getDiskImage().getDiskStorageType());
Line 39:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_UNSUPPORTED_DISK_STORAGE_TYPE);
Line 40: return false;
Line 37: addCanDoActionMessage("$diskId " +
getParameters().getDiskImage().getId());
Line 38: addCanDoActionMessage("$storageType " +
getParameters().getDiskImage().getDiskStorageType());
Line 39:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_UNSUPPORTED_DISK_STORAGE_TYPE);
Line 40: return false;
Line 41: } else {
I've heard arguments both ways for a single line if or else clause. I tend to
go with always wrapping as if some code needs to be added later to the
statement it's less likely to cause a problem.
Line 42: return true;
Line 43: }
Line 44: }
Line 45:
Line 39:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_UNSUPPORTED_DISK_STORAGE_TYPE);
Line 40: return false;
Line 41: } else {
Line 42: return true;
Line 43: }
Thanks for the tip. I've added that for the next patch.
Line 44: }
Line 45:
Line 46: @Override
Line 47: protected void executeCommand() {
Line 45:
Line 46: @Override
Line 47: protected void executeCommand() {
Line 48: final DiskImage newDiskImage = getParameters().getDiskImage();
Line 49: if (newDiskImage != null) {
I've removed the check as the parameters will fail to initialize if disk image
is null anyway.
Line 50: addDiskImageToDb(newDiskImage, getCompensationContext());
Line 51:
getReturnValue().setActionReturnValue(newDiskImage.getId());
Line 52: getReturnValue().setSucceeded(true);
Line 53: }
Line 46: @Override
Line 47: protected void executeCommand() {
Line 48: final DiskImage newDiskImage = getParameters().getDiskImage();
Line 49: if (newDiskImage != null) {
Line 50: addDiskImageToDb(newDiskImage, getCompensationContext());
I've removed the @NonTransactiveCommandAttribute annotation as this should be
in a transaction.
Line 51:
getReturnValue().setActionReturnValue(newDiskImage.getId());
Line 52: getReturnValue().setSucceeded(true);
Line 53: }
Line 54: }
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetUnregisteredDisksQueryTest.java
Line 39: extends
Line 40: AbstractQueryTest<GetUnregisteredDisksQueryParameters,
GetUnregisteredDisksQuery<GetUnregisteredDisksQueryParameters>> {
Line 41:
Line 42: @Rule
Line 43: public static final MockConfigRule mcr = new MockConfigRule();
I think it was an artifact of copying another test. Removed.
Line 44:
Line 45: private Guid importDiskId = Guid.NewGuid();
Line 46: private Guid existingDiskId = Guid.NewGuid();
Line 47: private Guid storageDomainId = Guid.NewGuid();
Line 44:
Line 45: private Guid importDiskId = Guid.NewGuid();
Line 46: private Guid existingDiskId = Guid.NewGuid();
Line 47: private Guid storageDomainId = Guid.NewGuid();
Line 48: private Guid storagePoolId = Guid.NewGuid();
I've moved them to the setup.
Line 49:
Line 50: private List<Guid> importDiskIds = new
ArrayList<Guid>(Arrays.asList(importDiskId, existingDiskId));
Line 51:
Line 52: @Before
Line 46: private Guid existingDiskId = Guid.NewGuid();
Line 47: private Guid storageDomainId = Guid.NewGuid();
Line 48: private Guid storagePoolId = Guid.NewGuid();
Line 49:
Line 50: private List<Guid> importDiskIds = new
ArrayList<Guid>(Arrays.asList(importDiskId, existingDiskId));
The list is returned from a query and the command modifies it, however a list
returned by Arrays.asList() is immutable and throws an exception during the
modification. Wrapping it in a new ArrayList() allows for mutation while
keeping the initialization on a single line. I'll add a comment as it may
confuse future readers of the code.
Line 51:
Line 52: @Before
Line 53: public void setUp() throws Exception {
Line 54: super.setUp();
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
Line 716: ACTION_TYPE_FAILED_EXTERNAL_EVENT_NOT_FOUND,
Line 717: ACTION_TYPE_FAILED_EXTERNAL_EVENT_ILLRGAL_OPERATION,
Line 718:
Line 719: // Register disk
Line 720: ACTION_TYPE_FAILED_UNSUPPORTED_DISK_STORAGE_TYPE;
Done.
Line 721:
Line 722: public int getValue() {
Line 723: return this.ordinal();
Line 724: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDiskResourceTest.java
Line 69: DiskImage entity = new DiskImage();
Line 70: entity.setId(GUIDS[index]);
Line 71: entity.setvolume_format(VolumeFormat.RAW);
Line 72: entity.setDiskInterface(DiskInterface.VirtIO);
Line 73: entity.setImageStatus(ImageStatus.OK);
ok, I'll update with whatever version is there.
Line 74: entity.setvolume_type(VolumeType.Sparse);
Line 75: entity.setBoot(false);
Line 76: entity.setShareable(false);
Line 77: entity.setPropagateErrors(PropagateErrors.On);
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDisksResourceTest.java
Line 41: DiskImage entity = new DiskImage();
Line 42: entity.setId(GUIDS[index]);
Line 43: entity.setvolume_format(VolumeFormat.RAW);
Line 44: entity.setDiskInterface(DiskInterface.VirtIO);
Line 45: entity.setImageStatus(ImageStatus.OK);
ok
Line 46: entity.setvolume_type(VolumeType.Sparse);
Line 47: entity.setBoot(false);
Line 48: entity.setShareable(false);
Line 49: entity.setPropagateErrors(PropagateErrors.On);
....................................................
Commit Message
Line 30: storage domain, I found that it returned a list of storage
Line 31: pools. I am assuming that an NFS based storage domain would
Line 32: only belong to a single storage pool and thus I took the first
Line 33: item in the list. If this assumption is unfounded please note
Line 34: in the review.
I've added a check in the canDoAction methods of the query and command to
verify the domain is a data domain.
Line 35:
Line 36: This change also implements the GET and POST methods for the
Line 37: REST /api/storagedomains/{storagedomain:id}/disks;unregistered
Line 38: resource. These depend on the new RegisterDiskCommand and
--
To view, visit http://gerrit.ovirt.org/11783
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I82de498fd9a8e25ed9e1dc5776f2fdf0c35b46da
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Chris Morrissey <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Chris Morrissey <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
