From e9cbe0ed6a1de239865d1f1ce9f42897f821ea4c Mon Sep 17 00:00:00 2001 From: Pierre Charton Date: Mon, 21 Oct 2024 17:33:21 +0200 Subject: [PATCH] Revert "Refactoring error handling" This reverts commit d37a35b48977641c23e371ed3e5d9f09d8ae6e08. --- .../backup/BackrollBackupProvider.java | 49 ++++---- .../backup/backroll/BackrollClient.java | 109 ++++++++++-------- 2 files changed, 86 insertions(+), 72 deletions(-) diff --git a/plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java b/plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java index cc425dc038e..080f31a4531 100644 --- a/plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java +++ b/plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.net.URISyntaxException; import java.security.KeyManagementException; import java.security.NoSuchAlgorithmException; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -38,6 +39,7 @@ import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.Configurable; import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.lang3.StringUtils; import org.apache.http.client.ClientProtocolException; import org.joda.time.DateTime; @@ -203,10 +205,7 @@ public class BackrollBackupProvider extends AdapterBase implements BackupProvide backup.setAccountId(vm.getAccountId()); backup.setDomainId(vm.getDomainId()); backup.setZoneId(vm.getDataCenterId()); - if (backupDao.persist(backup) == null) {// TODO is null a failure ? - throw new Exception("Failed to persist backup."); - } - ; + assert backupDao.persist(backup) != null; } catch (Exception e) { throw new CloudRuntimeException(String.format("Failed to take a backup of VM %s.", vm.getName()), e); } @@ -360,31 +359,33 @@ public class BackrollBackupProvider extends AdapterBase implements BackupProvide logger.info("backroll delete backup id: {}", backup.getExternalId()); if (backup.getStatus().equals(Backup.Status.BackingUp)) { throw new CloudRuntimeException("You can't delete a backup while it still backing up."); + } else { + logger.debug("backroll - try delete backup"); + VMInstanceVO vm = vmInstanceDao.findByIdIncludingRemoved(backup.getVmId()); + + if (backup.getStatus().equals(Backup.Status.Removed) || backup.getStatus().equals(Backup.Status.Failed)) { + return deleteBackupInDb(backup); + } + + try { + getClient(backup.getZoneId()).deleteBackup(vm.getUuid(), getBackupName(backup)); + } catch (Exception e) { + throw new CloudRuntimeException(String.format("Failed to delete backup %s", backup.getName())); + } + + logger.debug("Backup deletion for backup {} complete on backroll side.", backup.getUuid()); + return deleteBackupInDb(backup); } - - logger.debug("backroll - try delete backup"); - VMInstanceVO vm = vmInstanceDao.findByIdIncludingRemoved(backup.getVmId()); - - if (backup.getStatus().equals(Backup.Status.Removed) || backup.getStatus().equals(Backup.Status.Failed)) { - deleteBackupInDb(backup); - return true; - } - - try { - getClient(backup.getZoneId()).deleteBackup(vm.getUuid(), getBackupName(backup)); - } catch (Exception e) { - throw new CloudRuntimeException(String.format("Failed to delete backup %s", backup.getName())); - } - - logger.debug("Backup deletion for backup {} complete on backroll side.", backup.getUuid()); - deleteBackupInDb(backup); - return true; } - private void deleteBackupInDb(Backup backup) { + private boolean deleteBackupInDb(Backup backup) { BackupVO backupToUpdate = ((BackupVO) backup); backupToUpdate.setStatus(Backup.Status.Removed); - backupDao.persist(backupToUpdate); // TODO is null a failure ? + if (backupDao.persist(backupToUpdate) != null) { + logger.debug("Backroll backup {} deleted in database.", backup.getUuid()); + return true; + } + return false; } protected BackrollClient getClient(final Long zoneId) throws ClientProtocolException, IOException { diff --git a/plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/backroll/BackrollClient.java b/plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/backroll/BackrollClient.java index e57d9c5e73d..cfdcf566646 100644 --- a/plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/backroll/BackrollClient.java +++ b/plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/backroll/BackrollClient.java @@ -80,6 +80,7 @@ import org.apache.logging.log4j.Logger; import org.joda.time.DateTime; +import org.json.JSONException; import org.json.JSONObject; import com.cloud.utils.exception.CloudRuntimeException; @@ -139,22 +140,25 @@ public class BackrollClient { } } - public String startBackupJob(final String jobId) - throws ClientProtocolException, IOException, ParseException, NotOkBodyException { + public String startBackupJob(final String jobId) { logger.info("Trying to start backup for Backroll job: {}", jobId); - ensureLoggedIn(); - - return this.parse(okBody( - post(String.format("/tasks/singlebackup/%s", jobId), null))).location - .replace("/api/v1/status/", ""); + try { + loginIfAuthenticationFailed(); + return this.parse(okBody( + post(String.format("/tasks/singlebackup/%s", jobId), null))).location + .replace("/api/v1/status/", ""); + } catch (final Exception e) { + logger.error("Failed to start Backroll backup job due to: {}", e.getMessage()); + } + return null; } public String getBackupOfferingUrl() throws ClientProtocolException, IOException, ParseException, NotOkBodyException { logger.info("Trying to list backroll backup policies"); - ensureLoggedIn(); + loginIfAuthenticationFailed(); return this.parse(okBody(get("/backup_policies"))).location.replace("/api/v1", ""); @@ -164,7 +168,7 @@ public class BackrollClient { throws ParseException, NotOkBodyException, ClientProtocolException, IOException, InterruptedException { logger.info("Trying to list backroll backup policies"); - ensureLoggedIn(); + loginIfAuthenticationFailed(); BackupPoliciesResponse backupPoliciesResponse = waitGet(idTask); @@ -178,7 +182,7 @@ public class BackrollClient { public void restoreVMFromBackup(final String vmId, final String backupName) throws Exception { logger.info("Start restore backup with backroll with backup {} for vm {}", backupName, vmId); - ensureLoggedIn(); + loginIfAuthenticationFailed(); JSONObject jsonBody = new JSONObject(); jsonBody.put("virtual_machine_id", vmId); @@ -203,7 +207,7 @@ public class BackrollClient { throws ParseException, IOException, NotOkBodyException { logger.info("Trying to get backup status for Backroll task: {}", taskId); - ensureLoggedIn(); + loginIfAuthenticationFailed(); BackrollTaskStatus status = new BackrollTaskStatus(); @@ -221,27 +225,32 @@ public class BackrollClient { return status; } - public void deleteBackup(final String vmId, final String backupName) - throws Exception { + public boolean deleteBackup(final String vmId, final String backupName) + throws ClientProtocolException, IOException { logger.info("Trying to delete backup {} for vm {} using Backroll", vmId, backupName); - ensureLoggedIn(); + loginIfAuthenticationFailed(); - String urlToRequest = this.parse(okBody(delete( - String.format("/virtualmachines/%s/backups/%s", vmId, backupName)))).location - .replace("/api/v1", ""); + try { + String urlToRequest = this.parse(okBody(delete( + String.format("/virtualmachines/%s/backups/%s", vmId, backupName)))).location + .replace("/api/v1", ""); - BackrollBackupsFromVMResponse backrollBackupsFromVMResponse = waitGet(urlToRequest); - logger.debug(backrollBackupsFromVMResponse.state); - if (!backrollBackupsFromVMResponse.state.equals(TaskState.SUCCESS)) { - throw new Exception("Backroll task failed."); + BackrollBackupsFromVMResponse backrollBackupsFromVMResponse = waitGet(urlToRequest); + logger.debug(backrollBackupsFromVMResponse.state); + return backrollBackupsFromVMResponse.state.equals(TaskState.SUCCESS); + } catch (final NotOkBodyException e) { + return false; + } catch (final Exception e) { + logger.error("Failed to delete backup using Backroll due to: {}", e.getMessage()); } + return false; } public Metric getVirtualMachineMetrics(final String vmId) throws ClientProtocolException, IOException { logger.info("Trying to retrieve virtual machine metric from Backroll for vm {}", vmId); - ensureLoggedIn(); + loginIfAuthenticationFailed(); Metric metric = new Metric(0L, 0L); @@ -276,7 +285,7 @@ public class BackrollClient { JsonProcessingException, ParseException, IOException, NotOkBodyException, InterruptedException { logger.info("Trying to get backup metrics for VM: {}, and backup: {}", vmId, backupId); - ensureLoggedIn(); + loginIfAuthenticationFailed(); String urlToRequest = this.parse(okBody(get( String.format("/virtualmachines/%s/backups/%s", vmId, backupId)))).location.replace("/api/v1", ""); @@ -323,20 +332,26 @@ public class BackrollClient { } private HttpResponse post(final String path, final JSONObject json) throws IOException { + String xml = null; + StringEntity params = null; + if (json != null) { + logger.debug("JSON {}", json.toString()); + params = new StringEntity(json.toString(), ContentType.APPLICATION_JSON); + } + String url = apiURI.toString() + path; final HttpPost request = new HttpPost(url); + if (params != null) { + request.setEntity(params); + } + request.setHeader(HttpHeaders.AUTHORIZATION, "Bearer " + backrollToken); request.setHeader("Content-type", "application/json"); - if (json != null) { - logger.debug("JSON {}", json.toString()); - request.setEntity(new StringEntity(json.toString(), ContentType.APPLICATION_JSON)); - } - final HttpResponse response = httpClient.execute(request); - logger.debug("Response received in POST request with body {} is: {} for URL {}.", json, response.toString(), + logger.debug("Response received in POST request with body {} is: {} for URL {}.", xml, response.toString(), url); return response; @@ -363,14 +378,9 @@ public class BackrollClient { return response; } - private boolean isResponseOk(final HttpResponse response) { - switch (response.getStatusLine().getStatusCode()) { - case HttpStatus.SC_OK: - case HttpStatus.SC_ACCEPTED: - return true; - default: - return false; - } + private boolean isResponseAuthorized(final HttpResponse response) { + return response.getStatusLine().getStatusCode() == HttpStatus.SC_OK + || response.getStatusLine().getStatusCode() == HttpStatus.SC_ACCEPTED; } public class NotOkBodyException extends Exception { @@ -388,15 +398,18 @@ public class BackrollClient { private String okBody(final HttpResponse response) throws ParseException, IOException, NotOkBodyException { - if (isResponseOk(response)) { - HttpEntity bodyEntity = response.getEntity(); - try { - return EntityUtils.toString(bodyEntity); - } finally { - EntityUtils.consumeQuietly(bodyEntity); - } + switch (response.getStatusLine().getStatusCode()) { + case HttpStatus.SC_OK: + case HttpStatus.SC_ACCEPTED: + HttpEntity bodyEntity = response.getEntity(); + try { + return EntityUtils.toString(bodyEntity); + } finally { + EntityUtils.consumeQuietly(bodyEntity); + } + default: + throw new NotOkBodyException(response); } - throw new NotOkBodyException(response); } private T parse(final String json) @@ -428,15 +441,15 @@ public class BackrollClient { boolean result = false; try { final HttpResponse response = post("/auth", null); - result = isResponseOk(response); - EntityUtils.consumeQuietly(response.getEntity()); // TODO Move to a finally clause ? + result = isResponseAuthorized(response); + EntityUtils.consumeQuietly(response.getEntity()); } catch (IOException e) { logger.error("Failed to authenticate to Backroll due to: {}", e.getMessage()); } return result; } - private void ensureLoggedIn() throws ClientProtocolException, IOException { + private void loginIfAuthenticationFailed() throws ClientProtocolException, IOException { if (!isAuthenticated()) { login(appname, password); }