Arik Hadas has uploaded a new change for review. Change subject: core: fix NPE in decreasePendingVms ......................................................................
core: fix NPE in decreasePendingVms There was a race between the VdsUpdateRunTimeInfo thread that invokes RunVmCommandBase#decreasePendingVms method through OnPowerringUp callback and the thread that invokes MigrateVmCommand#rerun (the thread is created in VdsEventListener#rerun). The 'rerun thread' was doing 'setVm(null)' in order to refresh the VM on the next getVm invocation and the VdsUpdateRunTimeInfo thread was trying to the get the VM using the 'getVm' method VM in the same time - as a result of this race, the VdsUpdateRunTimeInfo thread might end up with null VM which results in NPE. The solution is to change the VdsUpdateRunTimeInfo thread to fetch the VM directly from the DB instead of using the 'getVm' method, And since we don't need the dynamic info of the VM we can query only the static part of the VM. Change-Id: Icaca2a3849a9fefbc33b055c6f4d2dd32959ad28 Bug-Url: https://bugzilla.redhat.com/1000824 Signed-off-by: Arik Hadas <aha...@redhat.com> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java 1 file changed, 24 insertions(+), 8 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/85/19885/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java index 115b2e6..ea74927 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java @@ -27,7 +27,9 @@ import org.ovirt.engine.core.common.businessentities.LunDisk; import org.ovirt.engine.core.common.businessentities.StorageServerConnections; import org.ovirt.engine.core.common.businessentities.VDS; +import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VMStatus; +import org.ovirt.engine.core.common.businessentities.VmStatic; import org.ovirt.engine.core.common.config.Config; import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.errors.VdcBllMessages; @@ -40,6 +42,7 @@ import org.ovirt.engine.core.common.vdscommands.VDSCommandType; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dal.dbbroker.DbFacade; +import org.ovirt.engine.core.dao.VdsDynamicDAO; import org.ovirt.engine.core.utils.log.Log; import org.ovirt.engine.core.utils.log.LogFactory; import org.ovirt.engine.core.utils.threadpool.ThreadPoolUtil; @@ -277,17 +280,22 @@ } protected void decreasePendingVms(Guid vdsId) { - DbFacade.getInstance() - .getVdsDynamicDao() - .updatePartialVdsDynamicCalc(vdsId, 0, -getVm().getNumOfCpus(), -getVm().getMinAllocatedMem(), 0, 0); + VM vm = getVm(); + decreasePendingVms(vdsId, vm.getNumOfCpus(), vm.getMinAllocatedMem(), vm.getName()); + } + + protected void decreasePendingVms(Guid vdsId, int numOfCpus, int minAllocatedMem, String vmName) { + getVdsDynamicDao().updatePartialVdsDynamicCalc(vdsId, 0, -numOfCpus, -minAllocatedMem, 0, 0); getBlockingQueue(vdsId).offer(Boolean.TRUE); if (log.isDebugEnabled()) { - log.debugFormat("DecreasePendingVms::Decreasing vds {0} pending vcpu count, in {1}. Vm: {2}", - vdsId, getVm().getNumOfCpus(), getVm().getName()); - log.debugFormat("DecreasePendingVms::Decreasing vds {0} pending vmem size, in {1}. Vm: {2}", - vdsId, getVm().getMinAllocatedMem(), getVm().getName()); + log.debugFormat("Decreasing vds {0} pending vcpu count by {1} and vmem size by {2} (Vm: {3})", + vdsId, numOfCpus, minAllocatedMem, vmName); } + } + + protected VdsDynamicDAO getVdsDynamicDao() { + return DbFacade.getInstance().getVdsDynamicDao(); } /** @@ -329,9 +337,17 @@ return ResourceManager.getInstance().GetVdsManager(vdsId).getVdsMonitor(); } + /** + * Since this callback is called by the VdsUpdateRunTimeInfo thread, we don't want it + * to fetch the VM using {@link #getVm()}, as the thread that invokes {@link #rerun()}, + * which runs in parallel, is doing setVm(null) to refresh the VM, and because of this + * race we might end up with null VM. so we fetch the static part of the VM from the DB. + */ @Override public void onPowerringUp() { - decreasePendingVms(getCurrentVdsId()); + VmStatic vmStatic = getVmStaticDAO().get(getVmId()); + decreasePendingVms(getCurrentVdsId(), vmStatic.getNumOfCpus(), + vmStatic.getMinAllocatedMem(), vmStatic.getName()); } @Override -- To view, visit http://gerrit.ovirt.org/19885 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Icaca2a3849a9fefbc33b055c6f4d2dd32959ad28 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.3 Gerrit-Owner: Arik Hadas <aha...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
