Piotr Kliczewski has uploaded a new change for review. Change subject: vdsbroker: reduced scope of synchronized blocks ......................................................................
vdsbroker: reduced scope of synchronized blocks In VdsIdVDSCommandBase there was too broad synchronized block which blocked many threads. The synchnornization on vdsManager was moved to commands which inherit from the class and only to blocks of code which need it. In IrsBrokerCommand there was synchronized block for each execution of storage related commands. I have seen tens of threds being blocked in executeVDSCommand. After executing few times performance tests (10 threads) we see no more threads being blocked by synchonization blocks. Bug-Url: https://bugzilla.redhat.com/1193509 Change-Id: I7d1bfd7b1fc7bfcc6465eae62feda6f1a27ff455 Signed-off-by: pkliczewski <[email protected]> --- M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/AddVdsVDSCommand.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/RemoveVdsVDSCommand.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/SetVdsStatusVDSCommand.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/UpdateVdsVMsClearedVDSCommand.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsIdVDSCommandBase.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java 7 files changed, 151 insertions(+), 150 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/47/37947/1 diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/AddVdsVDSCommand.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/AddVdsVDSCommand.java index 080aef0..c21a3e9 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/AddVdsVDSCommand.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/AddVdsVDSCommand.java @@ -17,9 +17,11 @@ @Override protected void executeVdsIdCommand() { log.info("AddVds - entered , starting logic to add VDS '{}'", getVdsId()); - VDS vds = DbFacade.getInstance().getVdsDao().get(getVdsId()); log.info("AddVds - VDS '{}' was added, will try to add it to the resource manager", getVdsId()); - ResourceManager.getInstance().AddVds(vds, false); + synchronized (_vdsManager.getLockObj()) { + VDS vds = DbFacade.getInstance().getVdsDao().get(getVdsId()); + ResourceManager.getInstance().AddVds(vds, false); + } } } diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/RemoveVdsVDSCommand.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/RemoveVdsVDSCommand.java index c54b9b1..ee3f72a 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/RemoveVdsVDSCommand.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/RemoveVdsVDSCommand.java @@ -13,6 +13,8 @@ @Override protected void executeVdsIdCommand() { - ResourceManager.getInstance().RemoveVds(getVdsId(), parameters.isNewHost()); + synchronized (_vdsManager.getLockObj()) { + ResourceManager.getInstance().RemoveVds(getVdsId(), parameters.isNewHost()); + } } } diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/SetVdsStatusVDSCommand.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/SetVdsStatusVDSCommand.java index 3a06eca..382b7d9 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/SetVdsStatusVDSCommand.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/SetVdsStatusVDSCommand.java @@ -27,45 +27,46 @@ final SetVdsStatusVDSCommandParameters parameters = getParameters(); if (_vdsManager != null) { + synchronized (_vdsManager.getLockObj()) { + final VDS vds = getVds(); + if (vds.getSpmStatus() != VdsSpmStatus.None && parameters.getStatus() != VDSStatus.Up) { + log.info("VDS '{}' is spm and moved from up calling resetIrs.", vds.getName()); + // check if this host was spm and reset if do. + getVDSReturnValue().setSucceeded( + ResourceManager + .getInstance() + .runVdsCommand( + VDSCommandType.ResetIrs, + new ResetIrsVDSCommandParameters(vds.getStoragePoolId(), vds.getId())) + .getSucceeded()); - final VDS vds = getVds(); - if (vds.getSpmStatus() != VdsSpmStatus.None && parameters.getStatus() != VDSStatus.Up) { - log.info("VDS '{}' is spm and moved from up calling resetIrs.", vds.getName()); - // check if this host was spm and reset if do. - getVDSReturnValue().setSucceeded( - ResourceManager - .getInstance() - .runVdsCommand( - VDSCommandType.ResetIrs, - new ResetIrsVDSCommandParameters(vds.getStoragePoolId(), vds.getId())) - .getSucceeded()); + if (!getVDSReturnValue().getSucceeded()) { + if (getParameters().isStopSpmFailureLogged()) { + AuditLogableBase base = new AuditLogableBase(); + base.setVds(vds); + AuditLogDirector.log(base, AuditLogType.VDS_STATUS_CHANGE_FAILED_DUE_TO_STOP_SPM_FAILURE); + } - if (!getVDSReturnValue().getSucceeded()) { - if (getParameters().isStopSpmFailureLogged()) { - AuditLogableBase base = new AuditLogableBase(); - base.setVds(vds); - AuditLogDirector.log(base, AuditLogType.VDS_STATUS_CHANGE_FAILED_DUE_TO_STOP_SPM_FAILURE); + if (parameters.getStatus() == VDSStatus.PreparingForMaintenance) { + // ResetIrs command failed, SPM host status cannot be moved to Preparing For Maintenance + return; + } } - if (parameters.getStatus() == VDSStatus.PreparingForMaintenance) { - // ResetIrs command failed, SPM host status cannot be moved to Preparing For Maintenance - return; - } } + updateVdsFromParameters(parameters, vds); + TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { + + @Override + public Void runInTransaction() { + _vdsManager.setStatus(parameters.getStatus(), vds); + _vdsManager.updateDynamicData(vds.getDynamicData()); + _vdsManager.updateStatisticsData(vds.getStatisticsData()); + return null; + } + }); } - - updateVdsFromParameters(parameters, vds); - TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { - - @Override - public Void runInTransaction() { - _vdsManager.setStatus(parameters.getStatus(), vds); - _vdsManager.updateDynamicData(vds.getDynamicData()); - _vdsManager.updateStatisticsData(vds.getStatisticsData()); - return null; - } - }); } else { getVDSReturnValue().setSucceeded(false); } diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/UpdateVdsVMsClearedVDSCommand.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/UpdateVdsVMsClearedVDSCommand.java index 61f8119..be2d209 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/UpdateVdsVMsClearedVDSCommand.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/UpdateVdsVMsClearedVDSCommand.java @@ -11,11 +11,13 @@ @Override protected void executeVdsIdCommand() { if (_vdsManager != null) { - getVds().setVmCount(0); - getVds().setVmsCoresCount(0); - getVds().setVmActive(0); - getVds().setVmMigrating(0); - _vdsManager.updateDynamicData(getVds().getDynamicData()); + synchronized (_vdsManager.getLockObj()) { + getVds().setVmCount(0); + getVds().setVmsCoresCount(0); + getVds().setVmActive(0); + getVds().setVmMigrating(0); + _vdsManager.updateDynamicData(getVds().getDynamicData()); + } } else { getVDSReturnValue().setSucceeded(false); } diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsIdVDSCommandBase.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsIdVDSCommandBase.java index 7c9720d..6dfd4b8 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsIdVDSCommandBase.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsIdVDSCommandBase.java @@ -43,13 +43,7 @@ @Override protected void executeVDSCommand() { - if (_vdsManager != null) { - synchronized (_vdsManager.getLockObj()) { - executeVdsIdCommand(); - } - } else { - executeVdsIdCommand(); - } + executeVdsIdCommand(); } protected abstract void executeVdsIdCommand(); diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java index 15522c8..5ef3a4c 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java @@ -555,24 +555,26 @@ && mFailedToRunVmAttempts.incrementAndGet() >= Config .<Integer> getValue(ConfigValues.NumberOfFailedRunsOnVds)) { //Only one thread at a time can enter here - ResourceManager.getInstance().runVdsCommand(VDSCommandType.SetVdsStatus, - new SetVdsStatusVDSCommandParameters(vds.getId(), VDSStatus.Error)); + synchronized (getLockObj()) { + ResourceManager.getInstance().runVdsCommand(VDSCommandType.SetVdsStatus, + new SetVdsStatusVDSCommandParameters(vds.getId(), VDSStatus.Error)); - SchedulerUtil sched = SchedulerUtilQuartzImpl.getInstance(); - sched.scheduleAOneTimeJob( - this, - "recoverFromError", - new Class[0], - new Object[0], - Config.<Integer>getValue(ConfigValues.TimeToReduceFailedRunOnVdsInMinutes), - TimeUnit.MINUTES); - AuditLogDirector.log( - new AuditLogableBase(vds.getId()).addCustomValue( - "Time", - Config.<Integer> getValue(ConfigValues.TimeToReduceFailedRunOnVdsInMinutes).toString()), - AuditLogType.VDS_FAILED_TO_RUN_VMS); - log.info("Vds '{}' moved to Error mode after {} attempts. Time: {}", vds.getName(), - mFailedToRunVmAttempts, new Date()); + SchedulerUtil sched = SchedulerUtilQuartzImpl.getInstance(); + sched.scheduleAOneTimeJob( + this, + "recoverFromError", + new Class[0], + new Object[0], + Config.<Integer> getValue(ConfigValues.TimeToReduceFailedRunOnVdsInMinutes), + TimeUnit.MINUTES); + AuditLogDirector.log( + new AuditLogableBase(vds.getId()).addCustomValue( + "Time", + Config.<Integer> getValue(ConfigValues.TimeToReduceFailedRunOnVdsInMinutes).toString()), + AuditLogType.VDS_FAILED_TO_RUN_VMS); + log.info("Vds '{}' moved to Error mode after {} attempts. Time: {}", vds.getName(), + mFailedToRunVmAttempts, new Date()); + } } } diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java index 9a5ae6d..c2b6b05 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java @@ -156,94 +156,92 @@ @Override protected void executeVDSCommand() { boolean isStartReconstruct = false; - synchronized (getCurrentIrsProxyData().syncObj) { - try { - if (getIrsProxy() != null) { - executeIrsBrokerCommand(); - } else { - if (getVDSReturnValue().getVdsError() == null) { - getVDSReturnValue().setExceptionString("Cannot allocate IRS server"); - VDSError tempVar = new VDSError(); - tempVar.setCode(VdcBllErrors.IRS_REPOSITORY_NOT_FOUND); - tempVar.setMessage("Cannot allocate IRS server"); - getVDSReturnValue().setVdsError(tempVar); - } - getVDSReturnValue().setSucceeded(false); + try { + if (getIrsProxy() != null) { + executeIrsBrokerCommand(); + } else { + if (getVDSReturnValue().getVdsError() == null) { + getVDSReturnValue().setExceptionString("Cannot allocate IRS server"); + VDSError tempVar = new VDSError(); + tempVar.setCode(VdcBllErrors.IRS_REPOSITORY_NOT_FOUND); + tempVar.setMessage("Cannot allocate IRS server"); + getVDSReturnValue().setVdsError(tempVar); } - } catch (UndeclaredThrowableException ex) { - getVDSReturnValue().setExceptionString(ex.toString()); - getVDSReturnValue().setExceptionObject(ex); - getVDSReturnValue().setVdsError(new VDSError(VdcBllErrors.VDS_NETWORK_ERROR, ex.getMessage())); - if (ExceptionUtils.getRootCause(ex) != null) { - logException(ExceptionUtils.getRootCause(ex)); - } else { - LoggedUtils.logError(log, LoggedUtils.getObjectId(this), this, ex); - } - failover(); - } catch (XmlRpcRunTimeException ex) { - getVDSReturnValue().setExceptionString(ex.toString()); - getVDSReturnValue().setExceptionObject(ex); - if (ex.isNetworkError()) { - log.error("IrsBroker::Failed::{} - network exception.", getCommandName()); - getVDSReturnValue().setSucceeded(false); - } else { - log.error("IrsBroker::Failed::{}", getCommandName()); - LoggedUtils.logError(log, LoggedUtils.getObjectId(this), this, ex); - throw new IRSProtocolException(ex); - } - } catch (IRSNoMasterDomainException ex) { - getVDSReturnValue().setExceptionString(ex.toString()); - getVDSReturnValue().setExceptionObject(ex); - getVDSReturnValue().setVdsError(ex.getVdsError()); - log.error("IrsBroker::Failed::{}: {}", getCommandName(), ex.getMessage()); - log.debug("Exception", ex); - - if ((ex.getVdsError() == null || ex.getVdsError().getCode() != VdcBllErrors.StoragePoolWrongMaster) - && getCurrentIrsProxyData().getHasVdssForSpmSelection()) { - failover(); - } else { - isStartReconstruct = true; - } - } catch (IRSUnicodeArgumentException ex) { - throw new IRSGenericException("UNICODE characters are not supported.", ex); - } catch (IRSStoragePoolStatusException | IrsOperationFailedNoFailoverException ex) { - throw ex; - } catch (IRSNonOperationalException ex) { - getVDSReturnValue().setExceptionString(ex.toString()); - getVDSReturnValue().setExceptionObject(ex); - getVDSReturnValue().setVdsError(ex.getVdsError()); - logException(ex); - if (ex.getVdsError() != null && VdcBllErrors.SpmStatusError == ex.getVdsError().getCode()) { - getCurrentIrsProxyData().setCurrentVdsId(Guid.Empty); - } - failover(); - } catch (IRSErrorException ex) { - getVDSReturnValue().setExceptionString(ex.toString()); - getVDSReturnValue().setExceptionObject(ex); - getVDSReturnValue().setVdsError(ex.getVdsError()); - logException(ex); - if (log.isDebugEnabled()) { - LoggedUtils.logError(log, LoggedUtils.getObjectId(this), this, ex); - } - failover(); - } catch (RuntimeException ex) { - getVDSReturnValue().setExceptionString(ex.toString()); - getVDSReturnValue().setExceptionObject(ex); - if (ex instanceof VDSExceptionBase) { - getVDSReturnValue().setVdsError(((VDSExceptionBase) ex).getVdsError()); - } - if (ExceptionUtils.getRootCause(ex) != null && - ExceptionUtils.getRootCause(ex) instanceof SocketException) { - logException(ExceptionUtils.getRootCause(ex)); - } else { - LoggedUtils.logError(log, LoggedUtils.getObjectId(this), this, ex); - } - // always failover because of changes in vdsm error, until we - // realize what to do in each case: - failover(); - } finally { - getCurrentIrsProxyData().getTriedVdssList().clear(); + getVDSReturnValue().setSucceeded(false); } + } catch (UndeclaredThrowableException ex) { + getVDSReturnValue().setExceptionString(ex.toString()); + getVDSReturnValue().setExceptionObject(ex); + getVDSReturnValue().setVdsError(new VDSError(VdcBllErrors.VDS_NETWORK_ERROR, ex.getMessage())); + if (ExceptionUtils.getRootCause(ex) != null) { + logException(ExceptionUtils.getRootCause(ex)); + } else { + LoggedUtils.logError(log, LoggedUtils.getObjectId(this), this, ex); + } + failover(); + } catch (XmlRpcRunTimeException ex) { + getVDSReturnValue().setExceptionString(ex.toString()); + getVDSReturnValue().setExceptionObject(ex); + if (ex.isNetworkError()) { + log.error("IrsBroker::Failed::{} - network exception.", getCommandName()); + getVDSReturnValue().setSucceeded(false); + } else { + log.error("IrsBroker::Failed::{}", getCommandName()); + LoggedUtils.logError(log, LoggedUtils.getObjectId(this), this, ex); + throw new IRSProtocolException(ex); + } + } catch (IRSNoMasterDomainException ex) { + getVDSReturnValue().setExceptionString(ex.toString()); + getVDSReturnValue().setExceptionObject(ex); + getVDSReturnValue().setVdsError(ex.getVdsError()); + log.error("IrsBroker::Failed::{}: {}", getCommandName(), ex.getMessage()); + log.debug("Exception", ex); + + if ((ex.getVdsError() == null || ex.getVdsError().getCode() != VdcBllErrors.StoragePoolWrongMaster) + && getCurrentIrsProxyData().getHasVdssForSpmSelection()) { + failover(); + } else { + isStartReconstruct = true; + } + } catch (IRSUnicodeArgumentException ex) { + throw new IRSGenericException("UNICODE characters are not supported.", ex); + } catch (IRSStoragePoolStatusException | IrsOperationFailedNoFailoverException ex) { + throw ex; + } catch (IRSNonOperationalException ex) { + getVDSReturnValue().setExceptionString(ex.toString()); + getVDSReturnValue().setExceptionObject(ex); + getVDSReturnValue().setVdsError(ex.getVdsError()); + logException(ex); + if (ex.getVdsError() != null && VdcBllErrors.SpmStatusError == ex.getVdsError().getCode()) { + getCurrentIrsProxyData().setCurrentVdsId(Guid.Empty); + } + failover(); + } catch (IRSErrorException ex) { + getVDSReturnValue().setExceptionString(ex.toString()); + getVDSReturnValue().setExceptionObject(ex); + getVDSReturnValue().setVdsError(ex.getVdsError()); + logException(ex); + if (log.isDebugEnabled()) { + LoggedUtils.logError(log, LoggedUtils.getObjectId(this), this, ex); + } + failover(); + } catch (RuntimeException ex) { + getVDSReturnValue().setExceptionString(ex.toString()); + getVDSReturnValue().setExceptionObject(ex); + if (ex instanceof VDSExceptionBase) { + getVDSReturnValue().setVdsError(((VDSExceptionBase) ex).getVdsError()); + } + if (ExceptionUtils.getRootCause(ex) != null && + ExceptionUtils.getRootCause(ex) instanceof SocketException) { + logException(ExceptionUtils.getRootCause(ex)); + } else { + LoggedUtils.logError(log, LoggedUtils.getObjectId(this), this, ex); + } + // always failover because of changes in vdsm error, until we + // realize what to do in each case: + failover(); + } finally { + getCurrentIrsProxyData().getTriedVdssList().clear(); } if (isStartReconstruct) { startReconstruct(); -- To view, visit http://gerrit.ovirt.org/37947 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7d1bfd7b1fc7bfcc6465eae62feda6f1a27ff455 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
