Yair Zaslavsky has posted comments on this change.
Change subject: core: importing external and hosted-engine VMs to the engine
......................................................................
Patch Set 6:
(6 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeDiskCommand.java
Line 32: if (retValue && !canRunActionOnNonManagedVm()) {
Line 33: retValue = false;
Line 34: }
Line 35:
Line 36: if (retValue && !getVm().isRunningOrPaused()) {
Why is this relevant to this patch?
Line 37: setSucceeded(false);
Line 38: retValue = false;
Line 39: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__VM);
Line 40:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
Line 411:
Line 412: @Override
Line 413: protected boolean canDoAction() {
Line 414:
Line 415: if (getVm() == null) {
You're fixing here something outside the scope of the patch.
Not that I mind the fix, I am of course infavor, but wouldn't it be nice to
have all the "preparations" in a separate patch in the patchset?
Line 416:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_EXIST);
Line 417: return false;
Line 418: }
Line 419:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsEventListener.java
Line 312: @Override
Line 313: public void addExternallyManagedVms(List<VmStatic>
externalVmList) {
Line 314: for (VmStatic currVm : externalVmList) {
Line 315: AddVmFromScratchParameters params = new
AddVmFromScratchParameters(currVm, null, null);
Line 316: VdcReturnValueBase returnValue =
Backend.getInstance().runInternalAction(VdcActionType.AddVmFromScratch, params,
ExecutionHandler.createInternalJobContext());
You do understand that you potentially have lots of inserts , non batched?
Would be nice if we can improve that...
Line 317: if (!returnValue.getSucceeded()) {
Line 318: log.debugFormat("Failed adding Externally managed VM
{0}", currVm.getName());
Line 319: }
Line 320: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/ActivateDeactivateVmNicCommand.java
Line 47:
Line 48: @Override
Line 49: protected boolean canDoAction() {
Line 50:
Line 51: if (getVm() == null) {
Same comment as before.
Line 52:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_EXIST);
Line 53: return false;
Line 54: }
Line 55:
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java
Line 1599: // Searching for External VMs that run on the host
Line 1600: for (VmInternalData vmInternalData : _runningVms.values()) {
Line 1601: VM currentVmData =
_vmDict.get(vmInternalData.getVmDynamic().getId());
Line 1602: if (currentVmData == null) {
Line 1603: if
(getDbFacade().getVmStaticDao().get(vmInternalData.getVmDynamic().getId()) ==
null) {
Can't we think of a "one shot" query here, and not do N queries of get?
Please consult with Eli and Liran here (yes, I know this "pattern" exists in
many places in the code).
Line 1604: Guid vmId =
vmInternalData.getVmDynamic().getId();
Line 1605: vmsToQuery.add(vmId.toString());
Line 1606: }
Line 1607: }
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java
Line 57:
Line 58: @DefaultStringValue("Cannot ${action} ${type}. The snapshot is
broken, and no further work can be done on it. Please remove this snapshot from
the VM.")
Line 59: String ACTION_TYPE_FAILED_VM_SNAPSHOT_IS_BROKEN();
Line 60:
Line 61: @DefaultStringValue("Cannot ${action} ${type}. This VM is not
managed by the engine.")
Hmm... would providing some addition info on the VM can help the user? What do
you think?
Line 62: String ACTION_TYPE_FAILED_CANNOT_RUN_ACTION_ON_NON_MANAGED_VM();
Line 63:
Line 64: @DefaultStringValue("Storage Domain cannot be accessed.\nPossible
reasons:\nNo operational Host in Data Center or Data Center state is not Up.")
Line 65: String IMAGE_REPOSITORY_NOT_FOUND();
--
To view, visit http://gerrit.ovirt.org/17269
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b933df39aaf6897a675aef0564d3fb4922f12e7
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
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
