Arik Hadas has uploaded a new change for review.
Change subject: core: extensive refactoring in RunVmValidtor
......................................................................
core: extensive refactoring in RunVmValidtor
As a first stage in fixing regressions that were found in
RunVmValidator, this patch refactor this class to be more concise and
readable:
- use RunVmValidator#validate within #canRunVm method to make it shorter
and more organized.
- changed #validateStorageDomains, #validateImagesForRunVm and
#validateVdsStatus methods to return ValidationResult instead of
boolean as the other validation methods
- reduced the number of nested level in #validateBootSequence method by
removing unnecessary 'else' blocks
Change-Id: Ib18c7582a0b1b5686c35259be3aed5d399871a15
Signed-off-by: Arik Hadas <aha...@redhat.com>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java
1 file changed, 56 insertions(+), 101 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/96/17896/1
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java
index 35a01d8..59efbd1 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java
@@ -68,28 +68,25 @@
Guid storagePoolId = vm.getStoragePoolId();
// Block from running a VM with no HDD when its first boot device is
// HD and no other boot devices are configured
- if (boot_sequence == BootSequence.C && vmDisks.size() == 0) {
+ if (boot_sequence == BootSequence.C && vmDisks.isEmpty()) {
return new
ValidationResult(VdcBllMessages.VM_CANNOT_RUN_FROM_DISK_WITHOUT_DISK);
- } else {
- // If CD appears as first and there is no ISO in storage
- // pool/ISO inactive -
- // you cannot run this VM
-
- if (boot_sequence == BootSequence.CD
- &&
getIsoDomainListSyncronizer().findActiveISODomain(storagePoolId) == null) {
- return new
ValidationResult(VdcBllMessages.VM_CANNOT_RUN_FROM_CD_WITHOUT_ACTIVE_STORAGE_DOMAIN_ISO);
- } else {
- // if there is network in the boot sequence, check that the
- // vm has network,
- // otherwise the vm cannot be run in vdsm
- if (boot_sequence == BootSequence.N
- && getVmNicDao().getAllForVm(vm.getId()).size() == 0) {
- return new
ValidationResult(VdcBllMessages.VM_CANNOT_RUN_FROM_NETWORK_WITHOUT_NETWORK);
- }
- }
}
- return ValidationResult.VALID;
+ // If CD appears as first and there is no ISO in storage
+ // pool/ISO inactive - you cannot run this VM
+ if (boot_sequence == BootSequence.CD
+ &&
getIsoDomainListSyncronizer().findActiveISODomain(storagePoolId) == null) {
+ return new
ValidationResult(VdcBllMessages.VM_CANNOT_RUN_FROM_CD_WITHOUT_ACTIVE_STORAGE_DOMAIN_ISO);
+ }
+
+ // if there is network in the boot sequence, check that the
+ // vm has network, otherwise the vm cannot be run in vdsm
+ if (boot_sequence == BootSequence.N
+ && getVmNicDao().getAllForVm(vm.getId()).isEmpty()) {
+ return new
ValidationResult(VdcBllMessages.VM_CANNOT_RUN_FROM_NETWORK_WITHOUT_NETWORK);
+ }
+
+ return ValidationResult.VALID;
}
/**
@@ -105,40 +102,38 @@
* The VM's image disks
* @return <code>true</code> if the VM can be run, <code>false</code> if
not
*/
- public boolean validateStorageDomains(VM vm,
- List<String> message,
- boolean isInternalExecution,
+ public ValidationResult validateStorageDomains(VM vm, boolean
isInternalExecution,
List<DiskImage> vmImages) {
if (!vm.isAutoStartup() || !isInternalExecution) {
Set<Guid> storageDomainIds =
ImagesHandler.getAllStorageIdsForImageIds(vmImages);
MultipleStorageDomainsValidator storageDomainValidator =
new MultipleStorageDomainsValidator(vm.getStoragePoolId(),
storageDomainIds);
- if (!validate(storageDomainValidator.allDomainsExistAndActive(),
message)) {
- return false;
+
+ ValidationResult result =
storageDomainValidator.allDomainsExistAndActive();
+ if (!result.isValid()) {
+ return result;
}
- if (!vm.isAutoStartup()
- &&
!validate(storageDomainValidator.allDomainsWithinThresholds(), message)) {
- return false;
+ result = !vm.isAutoStartup() ?
storageDomainValidator.allDomainsWithinThresholds()
+ : ValidationResult.VALID;
+ if (!result.isValid()) {
+ return result;
}
}
- return true;
+ return ValidationResult.VALID;
}
/**
* Check isValid only if VM is not HA VM
*/
- public boolean validateImagesForRunVm(List<String> message,
List<DiskImage> vmDisks) {
- DiskImagesValidator diskImagesValidator = new
DiskImagesValidator(vmDisks);
- return validate(diskImagesValidator.diskImagesNotLocked(), message);
+ public ValidationResult validateImagesForRunVm(List<DiskImage> vmDisks) {
+ return new DiskImagesValidator(vmDisks).diskImagesNotLocked();
}
@SuppressWarnings("unchecked")
- public ValidationResult validateIsoPath(boolean isAutoStartup,
- Guid storagePoolId,
- String diskPath,
- String floppyPath) {
+ public ValidationResult validateIsoPath(boolean isAutoStartup, Guid
storagePoolId,
+ String diskPath, String floppyPath) {
if (isAutoStartup) {
return ValidationResult.VALID;
}
@@ -203,24 +198,23 @@
}
protected boolean isVmDuringInitiating(VM vm) {
- return ((Boolean) getBackend()
+ return (Boolean) getBackend()
.getResourceManager()
.RunVdsCommand(VDSCommandType.IsVmDuringInitiating,
new
IsVmDuringInitiatingVDSCommandParameters(vm.getId()))
- .getReturnValue()).booleanValue();
+ .getReturnValue();
}
- public boolean validateVdsStatus(VM vm, List<String> messages) {
+ public ValidationResult validateVdsStatus(VM vm) {
if (vm.getStatus() == VMStatus.Paused && vm.getRunOnVds() != null) {
- VDS vds = getVdsDao().get(
- new Guid(vm.getRunOnVds().toString()));
+ VDS vds = getVdsDao().get(new Guid(vm.getRunOnVds().toString()));
if (vds.getStatus() != VDSStatus.Up) {
- messages.add(VdcBllMessages.VAR__HOST_STATUS__UP.toString());
-
messages.add(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL.toString());
- return false;
+ return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL,
+ VdcBllMessages.VAR__HOST_STATUS__UP.toString());
}
}
- return true;
+
+ return ValidationResult.VALID;
}
public ValidationResult validateStatelessVm(VM vm, List<Disk> plugDisks,
Boolean stateless) {
@@ -357,71 +351,32 @@
List<Guid> vdsBlackList,
Guid destVds,
VDSGroup vdsGroup) {
- if (!validateVmProperties(vm, messages)) {
+
+ if (!validateVmProperties(vm, messages) ||
+ !validate(validateBootSequence(vm, bootSequence, vmDisks),
messages) ||
+ !validate(new VmValidator(vm).vmNotLocked(), messages) ||
+
!validate(getSnapshotValidator().vmNotDuringSnapshot(vm.getId()), messages) ||
+ !validate(validateVmStatusUsingMatrix(vm), messages)) {
return false;
}
- ValidationResult result = validateBootSequence(vm, bootSequence,
vmDisks);
- if (!result.isValid()) {
- messages.add(result.getMessage().toString());
- return false;
- }
- result = new VmValidator(vm).vmNotLocked();
- if (!result.isValid()) {
- messages.add(result.getMessage().toString());
- return false;
- }
- result = getSnapshotValidator().vmNotDuringSnapshot(vm.getId());
- if (!result.isValid()) {
- messages.add(result.getMessage().toString());
- return false;
- }
+
List<DiskImage> images = ImagesHandler.filterImageDisks(vmDisks, true,
false);
- if (!images.isEmpty()) {
- result = new StoragePoolValidator(storagePool).isUp();
- if (!result.isValid()) {
- messages.add(result.getMessage().toString());
- return false;
- }
- if (!validateStorageDomains(vm, messages, isInternalExecution,
images)) {
- return false;
- }
- if (!validateImagesForRunVm(messages, images)) {
- return false;
- }
- result = validateIsoPath(vm.isAutoStartup(),
vm.getStoragePoolId(), diskPath, floppyPath);
- if (!result.isValid()) {
- messages.add(result.getMessage().toString());
- return false;
- }
- result = vmDuringInitialization(vm);
- if (!result.isValid()) {
- messages.add(result.getMessage().toString());
- return false;
- }
- if (!validateVdsStatus(vm, messages)) {
- return false;
- }
- result = validateStatelessVm(vm, vmDisks, runAsStateless);
- if (!result.isValid()) {
- messages.add(result.getMessage().toString());
- return false;
- }
- }
- if (!SchedulingManager.getInstance().canSchedule(vdsGroup,
- vm,
- vdsBlackList,
- null,
- destVds,
- messages)) {
+ if (!images.isEmpty() && (
+ !validate(new StoragePoolValidator(storagePool).isUp(),
messages) ||
+ !validate(validateStorageDomains(vm, isInternalExecution,
images), messages) ||
+ !validate(validateImagesForRunVm(images), messages) ||
+ !validate(validateIsoPath(vm.isAutoStartup(),
vm.getStoragePoolId(), diskPath, floppyPath), messages) ||
+ !validate(vmDuringInitialization(vm), messages) ||
+ !validate(validateVdsStatus(vm), messages) ||
+ !validate(validateStatelessVm(vm, vmDisks, runAsStateless),
messages))) {
return false;
}
- result = validateVmStatusUsingMatrix(vm);
- if (!result.isValid()) {
- messages.add(result.getMessage().toString());
+
+ if (!SchedulingManager.getInstance().canSchedule(
+ vdsGroup, vm, vdsBlackList, null, destVds, messages)) {
return false;
}
return true;
}
-
}
--
To view, visit http://gerrit.ovirt.org/17896
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib18c7582a0b1b5686c35259be3aed5d399871a15
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Arik Hadas <aha...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
