From b54d19b3b9a66adbb8f5681005160ca549f35e33 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Tue, 16 Jun 2020 13:30:31 +0530 Subject: [PATCH] server: fix TransactionLegacy DB connection leaks due to DB switching by B&R thread (#4121) BackupSync task would switch between databases to update backup usage metrics in the cloud_usage.usage_backup table. The current framework and the usage in ManagedContext causes database connection (LegacyTransaction) leaks. When the thread runs faster, the issue is easily reproducible and checking via heap dump analysis or using JMX MBeans. This fixes by moving the task of backup data updation for usage data to the usage server by publishing usage events instead of switching between databases in a local thread while in a ManagedContextRunnable. Signed-off-by: Rohit Yadav --- .../main/java/com/cloud/event/EventTypes.java | 1 + .../com/cloud/usage/dao/UsageBackupDao.java | 7 +- .../cloud/usage/dao/UsageBackupDaoImpl.java | 71 +++++++++---------- .../com/cloud/utils/db/TransactionLegacy.java | 14 ++-- .../backup/DummyBackupProvider.java | 7 +- .../backup/VeeamBackupProvider.java | 5 +- .../cloudstack/backup/BackupManagerImpl.java | 40 ++++------- .../com/cloud/usage/UsageManagerImpl.java | 9 ++- 8 files changed, 75 insertions(+), 79 deletions(-) diff --git a/api/src/main/java/com/cloud/event/EventTypes.java b/api/src/main/java/com/cloud/event/EventTypes.java index 30b6ac0b0a17..ec8089078c9a 100644 --- a/api/src/main/java/com/cloud/event/EventTypes.java +++ b/api/src/main/java/com/cloud/event/EventTypes.java @@ -492,6 +492,7 @@ public class EventTypes { public static final String EVENT_VM_BACKUP_RESTORE_VOLUME_TO_VM = "BACKUP.RESTORE.VOLUME.TO.VM"; public static final String EVENT_VM_BACKUP_SCHEDULE_CONFIGURE = "BACKUP.SCHEDULE.CONFIGURE"; public static final String EVENT_VM_BACKUP_SCHEDULE_DELETE = "BACKUP.SCHEDULE.DELETE"; + public static final String EVENT_VM_BACKUP_USAGE_METRIC = "BACKUP.USAGE.METRIC"; // external network device events public static final String EVENT_EXTERNAL_NVP_CONTROLLER_ADD = "PHYSICAL.NVPCONTROLLER.ADD"; diff --git a/engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDao.java b/engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDao.java index fb93c01a1226..8a72182ec677 100644 --- a/engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDao.java +++ b/engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDao.java @@ -20,14 +20,11 @@ import java.util.Date; import java.util.List; -import org.apache.cloudstack.backup.Backup; - import com.cloud.usage.UsageBackupVO; import com.cloud.utils.db.GenericDao; -import com.cloud.vm.VirtualMachine; public interface UsageBackupDao extends GenericDao { - void updateMetrics(VirtualMachine vm, Backup.Metric metric); - void removeUsage(Long accountId, Long zoneId, Long backupId); + void updateMetrics(Long vmId, Long size, Long virtualSize); + void removeUsage(Long accountId, Long vmId, Date eventDate); List getUsageRecords(Long accountId, Date startDate, Date endDate); } diff --git a/engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDaoImpl.java b/engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDaoImpl.java index 35b86bc082fe..712f81807c72 100644 --- a/engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDaoImpl.java @@ -19,69 +19,68 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; +import java.sql.SQLException; import java.util.ArrayList; import java.util.Date; import java.util.List; import java.util.TimeZone; -import org.apache.cloudstack.backup.Backup; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; +import com.cloud.exception.CloudException; import com.cloud.usage.UsageBackupVO; import com.cloud.utils.DateUtil; import com.cloud.utils.db.GenericDaoBase; -import com.cloud.utils.db.QueryBuilder; import com.cloud.utils.db.SearchCriteria; -import com.cloud.utils.db.Transaction; -import com.cloud.utils.db.TransactionCallback; import com.cloud.utils.db.TransactionLegacy; -import com.cloud.utils.db.TransactionStatus; -import com.cloud.vm.VirtualMachine; @Component public class UsageBackupDaoImpl extends GenericDaoBase implements UsageBackupDao { public static final Logger LOGGER = Logger.getLogger(UsageBackupDaoImpl.class); - protected static final String GET_USAGE_RECORDS_BY_ACCOUNT = "SELECT id, zone_id, account_id, domain_id, vm_id, backup_offering_id, size, protected_size, created, removed FROM cloud_usage.usage_backup WHERE " + + protected static final String UPDATE_DELETED = "UPDATE usage_backup SET removed = ? WHERE account_id = ? AND vm_id = ? and removed IS NULL"; + protected static final String GET_USAGE_RECORDS_BY_ACCOUNT = "SELECT id, zone_id, account_id, domain_id, vm_id, backup_offering_id, size, protected_size, created, removed FROM usage_backup WHERE " + " account_id = ? AND ((removed IS NULL AND created <= ?) OR (created BETWEEN ? AND ?) OR (removed BETWEEN ? AND ?) " + " OR ((created <= ?) AND (removed >= ?)))"; @Override - public void updateMetrics(final VirtualMachine vm, Backup.Metric metric) { - boolean result = Transaction.execute(TransactionLegacy.USAGE_DB, new TransactionCallback() { - @Override - public Boolean doInTransaction(final TransactionStatus status) { - final QueryBuilder qb = QueryBuilder.create(UsageBackupVO.class); - qb.and(qb.entity().getVmId(), SearchCriteria.Op.EQ, vm.getId()); - final UsageBackupVO entry = findOneBy(qb.create()); - if (entry == null) { - return false; - } - entry.setSize(metric.getBackupSize()); - entry.setProtectedSize(metric.getDataSize()); - return update(entry.getId(), entry); + public void updateMetrics(final Long vmId, final Long size, final Long virtualSize) { + try (TransactionLegacy txn = TransactionLegacy.open(TransactionLegacy.USAGE_DB)) { + SearchCriteria sc = this.createSearchCriteria(); + sc.addAnd("vmId", SearchCriteria.Op.EQ, vmId); + UsageBackupVO vo = findOneBy(sc); + if (vo != null) { + vo.setSize(size); + vo.setProtectedSize(virtualSize); + update(vo.getId(), vo); } - }); - if (!result) { - LOGGER.trace("Failed to update backup metrics for VM ID: " + vm.getId()); + } catch (final Exception e) { + LOGGER.error("Error updating backup metrics: " + e.getMessage(), e); } } @Override - public void removeUsage(Long accountId, Long zoneId, Long vmId) { - boolean result = Transaction.execute(TransactionLegacy.USAGE_DB, new TransactionCallback() { - @Override - public Boolean doInTransaction(final TransactionStatus status) { - final QueryBuilder qb = QueryBuilder.create(UsageBackupVO.class); - qb.and(qb.entity().getAccountId(), SearchCriteria.Op.EQ, accountId); - qb.and(qb.entity().getZoneId(), SearchCriteria.Op.EQ, zoneId); - qb.and(qb.entity().getVmId(), SearchCriteria.Op.EQ, vmId); - final UsageBackupVO entry = findOneBy(qb.create()); - return remove(qb.create()) > 0; + public void removeUsage(Long accountId, Long vmId, Date eventDate) { + TransactionLegacy txn = TransactionLegacy.open(TransactionLegacy.USAGE_DB); + try { + txn.start(); + try (PreparedStatement pstmt = txn.prepareStatement(UPDATE_DELETED);) { + if (pstmt != null) { + pstmt.setString(1, DateUtil.getDateDisplayString(TimeZone.getTimeZone("GMT"), eventDate)); + pstmt.setLong(2, accountId); + pstmt.setLong(3, vmId); + pstmt.executeUpdate(); + } + } catch (SQLException e) { + LOGGER.error("Error removing UsageBackupVO: " + e.getMessage(), e); + throw new CloudException("Remove backup usage exception: " + e.getMessage(), e); } - }); - if (!result) { - LOGGER.warn("Failed to remove usage entry for backup of VM ID: " + vmId); + txn.commit(); + } catch (Exception e) { + txn.rollback(); + LOGGER.error("Exception caught while removing UsageBackupVO: " + e.getMessage(), e); + } finally { + txn.close(); } } diff --git a/framework/db/src/main/java/com/cloud/utils/db/TransactionLegacy.java b/framework/db/src/main/java/com/cloud/utils/db/TransactionLegacy.java index 6777077c3d17..2dde30275a52 100644 --- a/framework/db/src/main/java/com/cloud/utils/db/TransactionLegacy.java +++ b/framework/db/src/main/java/com/cloud/utils/db/TransactionLegacy.java @@ -150,30 +150,28 @@ public static TransactionLegacy open(final String name) { public static TransactionLegacy open(final String name, final short databaseId, final boolean forceDbChange) { TransactionLegacy txn = tls.get(); - boolean isNew = false; if (txn == null) { if (s_logger.isTraceEnabled()) { s_logger.trace("Creating the transaction: " + name); } txn = new TransactionLegacy(name, false, databaseId); tls.set(txn); - isNew = true; + s_mbean.addTransaction(txn); } else if (forceDbChange) { final short currentDbId = txn.getDatabaseId(); if (currentDbId != databaseId) { // we need to end the current transaction and switch databases - txn.close(txn.getName()); + if (txn.close(txn.getName()) && txn.getCurrentConnection() == null) { + s_mbean.removeTransaction(txn); + } txn = new TransactionLegacy(name, false, databaseId); tls.set(txn); - isNew = true; + s_mbean.addTransaction(txn); } } txn.checkConnection(); txn.takeOver(name, false); - if (isNew) { - s_mbean.addTransaction(txn); - } return txn; } @@ -762,8 +760,8 @@ protected void closeConnection() { } _conn.close(); _conn = null; + s_mbean.removeTransaction(this); } - } catch (final SQLException e) { s_logger.warn("Unable to close connection", e); } diff --git a/plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java b/plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java index 449dfbf34373..3139da8fcdd2 100644 --- a/plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java +++ b/plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java @@ -87,8 +87,13 @@ public Pair restoreBackedUpVolume(Backup backup, String volumeU public Map getBackupMetrics(Long zoneId, List vms) { final Map metrics = new HashMap<>(); final Backup.Metric metric = new Backup.Metric(1000L, 100L); + if (vms == null || vms.isEmpty()) { + return metrics; + } for (VirtualMachine vm : vms) { - metrics.put(vm, metric); + if (vm != null) { + metrics.put(vm, metric); + } } return metrics; } diff --git a/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java b/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java index ee1279243305..0b532a52f955 100644 --- a/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java +++ b/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java @@ -216,9 +216,12 @@ public Pair restoreBackedUpVolume(Backup backup, String volumeU @Override public Map getBackupMetrics(final Long zoneId, final List vms) { final Map metrics = new HashMap<>(); + if (vms == null || vms.isEmpty()) { + return metrics; + } final Map backendMetrics = getClient(zoneId).getBackupMetrics(); for (final VirtualMachine vm : vms) { - if (!backendMetrics.containsKey(vm.getUuid())) { + if (vm == null || !backendMetrics.containsKey(vm.getUuid())) { continue; } metrics.put(vm, backendMetrics.get(vm.getUuid())); diff --git a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java index c91805dd4dc0..f3b0a3c4abcb 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java @@ -84,7 +84,6 @@ import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.DiskOfferingDao; import com.cloud.storage.dao.VolumeDao; -import com.cloud.usage.dao.UsageBackupDao; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.AccountService; @@ -126,8 +125,6 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { @Inject private AccountManager accountManager; @Inject - private UsageBackupDao usageBackupDao; - @Inject private VolumeDao volumeDao; @Inject private DataCenterDao dataCenterDao; @@ -1001,7 +998,6 @@ public BackupSyncTask(final BackupManager backupManager) { @Override protected void runInContext() { - final int SYNC_INTERVAL = BackupSyncPollingInterval.value().intValue(); try { if (LOG.isTraceEnabled()) { LOG.trace("Backup sync background task is running..."); @@ -1022,31 +1018,23 @@ protected void runInContext() { continue; } - // Sync backup usage metrics final Map metrics = backupProvider.getBackupMetrics(dataCenter.getId(), new ArrayList<>(vms)); - final GlobalLock syncBackupMetricsLock = GlobalLock.getInternLock("BackupSyncTask_metrics_zone_" + dataCenter.getId()); - if (syncBackupMetricsLock.lock(SYNC_INTERVAL)) { - try { - for (final VirtualMachine vm : metrics.keySet()) { - final Backup.Metric metric = metrics.get(vm); - if (metric != null) { - usageBackupDao.updateMetrics(vm, metric); - } + try { + for (final VirtualMachine vm : metrics.keySet()) { + final Backup.Metric metric = metrics.get(vm); + if (metric != null) { + // Sync out-of-band backups + backupProvider.syncBackups(vm, metric); + // Emit a usage event, update usage metric for the VM by the usage server + UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_BACKUP_USAGE_METRIC, vm.getAccountId(), + vm.getDataCenterId(), vm.getId(), "Backup-" + vm.getHostName() + "-" + vm.getUuid(), + vm.getBackupOfferingId(), null, metric.getBackupSize(), metric.getDataSize(), + Backup.class.getSimpleName(), vm.getUuid()); } - } finally { - syncBackupMetricsLock.unlock(); } - } - - // Sync out-of-band backups - for (final VirtualMachine vm : vms) { - final GlobalLock syncBackupsLock = GlobalLock.getInternLock("BackupSyncTask_backup_vm_" + vm.getId()); - if (syncBackupsLock.lock(SYNC_INTERVAL)) { - try { - backupProvider.syncBackups(vm, metrics.get(vm)); - } finally { - syncBackupsLock.unlock(); - } + } catch (final Throwable e) { + if (LOG.isTraceEnabled()) { + LOG.trace("Failed to sync backup usage metrics and out-of-band backups"); } } } diff --git a/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java b/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java index 314a9fcff3da..98b94e4c86e7 100644 --- a/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java +++ b/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java @@ -1081,7 +1081,10 @@ private boolean isVmSnapshotOnPrimaryEvent(String eventType) { } private boolean isBackupEvent(String eventType) { - return eventType != null && (eventType.equals(EventTypes.EVENT_VM_BACKUP_OFFERING_ASSIGN) || eventType.equals(EventTypes.EVENT_VM_BACKUP_OFFERING_REMOVE)); + return eventType != null && ( + eventType.equals(EventTypes.EVENT_VM_BACKUP_OFFERING_ASSIGN) || + eventType.equals(EventTypes.EVENT_VM_BACKUP_OFFERING_REMOVE) || + eventType.equals(EventTypes.EVENT_VM_BACKUP_USAGE_METRIC)); } private void createVMHelperEvent(UsageEventVO event) { @@ -1913,7 +1916,9 @@ private void createBackupEvent(final UsageEventVO event) { final UsageBackupVO backupVO = new UsageBackupVO(zoneId, accountId, domainId, vmId, backupOfferingId, created); usageBackupDao.persist(backupVO); } else if (EventTypes.EVENT_VM_BACKUP_OFFERING_REMOVE.equals(event.getType())) { - usageBackupDao.removeUsage(accountId, zoneId, vmId); + usageBackupDao.removeUsage(accountId, vmId, event.getCreateDate()); + } else if (EventTypes.EVENT_VM_BACKUP_USAGE_METRIC.equals(event.getType())) { + usageBackupDao.updateMetrics(vmId, event.getSize(), event.getVirtualSize()); } }