[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3969: Snapshot deletion issues
DaanHoogland commented on a change in pull request #3969: Snapshot deletion
issues
URL: https://github.com/apache/cloudstack/pull/3969#discussion_r406056673
##
File path:
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##
@@ -352,14 +359,21 @@ private boolean deleteSnapshotOnSecondaryStorage(Long
snapshotId, SnapshotInfo s
}
/**
- * Deletes the snapshot on primary storage. It can return false when the
snapshot was not stored on primary storage; however this does not means that it
failed to delete the snapshot.
- * In case of failure, it will throw one of the following exceptions:
CloudRuntimeException, InterruptedException, or ExecutionException.
+ * Deletes the snapshot on primary storage. It returns true when the
snapshot was not found on primary storage;
+ * In case of failure while deleting the snapshot, it will throw one of
the following exceptions: CloudRuntimeException, InterruptedException, or
ExecutionException.
*/
private boolean deleteSnapshotOnPrimary(Long snapshotId, SnapshotInfo
snapshotOnPrimaryInfo) {
SnapshotDataStoreVO snapshotOnPrimary =
snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
-if (isSnapshotOnPrimaryStorage(snapshotId) &&
snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) {
-snapshotOnPrimary.setState(State.Destroyed);
-snapshotStoreDao.update(snapshotOnPrimary.getId(),
snapshotOnPrimary);
+if (isSnapshotOnPrimaryStorage(snapshotId)) {
+s_logger.debug("Snapshot reference is found on primary storage for
snapshot id:" + snapshotId + ", performing snapshot deletion on primary");
Review comment:
can you surround debug statements that include string manupulation in teh
paramater list with `if (LOG.isDebugEnables()) {}` please
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3969: Snapshot deletion issues
DaanHoogland commented on a change in pull request #3969: Snapshot deletion
issues
URL: https://github.com/apache/cloudstack/pull/3969#discussion_r406054109
##
File path:
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##
@@ -287,18 +287,25 @@ private boolean deleteOnPrimaryIfNeeded(Long snapshotId)
{
// Here we handle snapshots which are to be deleted but are not
marked as destroyed yet.
// This may occur for instance when they are created only on
primary because
// snapshot.backup.to.secondary was set to false.
-SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo;
-try {
-obj.processEvent(Snapshot.Event.DestroyRequested);
-deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId,
snapshotOnPrimaryInfo);
-if (!deletedOnPrimary) {
-obj.processEvent(Snapshot.Event.OperationFailed);
-} else {
-obj.processEvent(Snapshot.Event.OperationSucceeded);
+if (snapshotOnPrimaryInfo == null) {
+s_logger.debug("Snapshot id:" + snapshotId + " not found on
primary storage, skipping snapshot deletion on primary and marking it
destroyed");
+snapshotVO.setState(Snapshot.State.Destroyed);
+snapshotDao.update(snapshotId, snapshotVO);
+return true;
Review comment:
no extra return please; this means `deletedOnPrimary = true` and we fall
through?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3969: Snapshot deletion issues
DaanHoogland commented on a change in pull request #3969: Snapshot deletion
issues
URL: https://github.com/apache/cloudstack/pull/3969#discussion_r404344872
##
File path:
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##
@@ -257,21 +257,23 @@ public boolean deleteSnapshot(Long snapshotId) {
return true;
}
-if (!Snapshot.State.BackedUp.equals(snapshotVO.getState()) &&
!Snapshot.State.Error.equals(snapshotVO.getState()) &&
+if (!Snapshot.State.BackedUp.equals(snapshotVO.getState()) &&
!Snapshot.State.Destroying.equals(snapshotVO.getState())) {
throw new InvalidParameterValueException("Can't delete
snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + "
Status");
}
-boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId);
+Boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId);
boolean deletedOnPrimary = deleteOnPrimaryIfNeeded(snapshotId);
if (deletedOnPrimary) {
s_logger.debug(String.format("Successfully deleted snapshot (id:
%d) on primary storage.", snapshotId));
-} else if (deletedOnSecondary) {
-s_logger.debug(String.format("The snapshot was deleted on
secondary storage. Could not find/delete snapshot (id: %d) on primary
storage.", snapshotId));
+} else {
+s_logger.debug(String.format("The snapshot (id: %d) could not be
found/deleted on primary storage.", snapshotId));
}
-
-return deletedOnSecondary || deletedOnPrimary;
+if (null == deletedOnSecondary && deletedOnSecondary) {
Review comment:
yes should be !=, tnx (it's late)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3969: Snapshot deletion issues
DaanHoogland commented on a change in pull request #3969: Snapshot deletion
issues
URL: https://github.com/apache/cloudstack/pull/3969#discussion_r403915720
##
File path:
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##
@@ -285,7 +284,27 @@ public boolean deleteSnapshot(Long snapshotId) {
}
}
-boolean deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId);
+boolean deletedOnPrimary = false;
+snapshotVO = snapshotDao.findById(snapshotId);
+SnapshotInfo snapshotOnPrimaryInfo =
snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
+if (snapshotVO != null && snapshotVO.getState() ==
Snapshot.State.Destroyed) {
+deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId,
snapshotOnPrimaryInfo);
+} else {
+// This is to handle snapshots which are created only on primary
when snapshot.backup.to.secondary is set to false.
+SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo;
+try {
+obj.processEvent(Snapshot.Event.DestroyRequested);
+deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId,
snapshotOnPrimaryInfo);
+if (!deletedOnPrimary) {
+obj.processEvent(Snapshot.Event.OperationFailed);
+} else {
+obj.processEvent(Snapshot.Event.OperationSucceeded);
+}
+} catch (NoTransitionException e) {
+s_logger.debug("Failed to set the state to destroying: ", e);
+return false;
+}
+}
Review comment:
@harikrishna-patnala can you factor this bit out, please?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3969: Snapshot deletion issues
DaanHoogland commented on a change in pull request #3969: Snapshot deletion
issues
URL: https://github.com/apache/cloudstack/pull/3969#discussion_r403918034
##
File path:
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##
@@ -285,7 +284,27 @@ public boolean deleteSnapshot(Long snapshotId) {
}
}
-boolean deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId);
+boolean deletedOnPrimary = false;
+snapshotVO = snapshotDao.findById(snapshotId);
+SnapshotInfo snapshotOnPrimaryInfo =
snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
+if (snapshotVO != null && snapshotVO.getState() ==
Snapshot.State.Destroyed) {
+deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId,
snapshotOnPrimaryInfo);
+} else {
+// This is to handle snapshots which are created only on primary
when snapshot.backup.to.secondary is set to false.
+SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo;
+try {
+obj.processEvent(Snapshot.Event.DestroyRequested);
+deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId,
snapshotOnPrimaryInfo);
+if (!deletedOnPrimary) {
+obj.processEvent(Snapshot.Event.OperationFailed);
+} else {
+obj.processEvent(Snapshot.Event.OperationSucceeded);
+}
+} catch (NoTransitionException e) {
+s_logger.debug("Failed to set the state to destroying: ", e);
+return false;
Review comment:
indent?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3969: Snapshot deletion issues
DaanHoogland commented on a change in pull request #3969: Snapshot deletion
issues
URL: https://github.com/apache/cloudstack/pull/3969#discussion_r403917585
##
File path:
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##
@@ -285,7 +284,27 @@ public boolean deleteSnapshot(Long snapshotId) {
}
}
-boolean deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId);
+boolean deletedOnPrimary = false;
+snapshotVO = snapshotDao.findById(snapshotId);
+SnapshotInfo snapshotOnPrimaryInfo =
snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
+if (snapshotVO != null && snapshotVO.getState() ==
Snapshot.State.Destroyed) {
+deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId,
snapshotOnPrimaryInfo);
+} else {
+// This is to handle snapshots which are created only on primary
when snapshot.backup.to.secondary is set to false.
Review comment:
this comment does not seem to be in alignment with the code. How do we
conclude that we are at this point in code because of the global setting
'snapshot.backup.to.secondary'?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3969: Snapshot deletion issues
DaanHoogland commented on a change in pull request #3969: Snapshot deletion
issues
URL: https://github.com/apache/cloudstack/pull/3969#discussion_r403915764
##
File path:
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##
@@ -285,7 +284,27 @@ public boolean deleteSnapshot(Long snapshotId) {
}
}
-boolean deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId);
+boolean deletedOnPrimary = false;
+snapshotVO = snapshotDao.findById(snapshotId);
+SnapshotInfo snapshotOnPrimaryInfo =
snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
+if (snapshotVO != null && snapshotVO.getState() ==
Snapshot.State.Destroyed) {
+deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId,
snapshotOnPrimaryInfo);
+} else {
+// This is to handle snapshots which are created only on primary
when snapshot.backup.to.secondary is set to false.
+SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo;
+try {
+obj.processEvent(Snapshot.Event.DestroyRequested);
+deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId,
snapshotOnPrimaryInfo);
+if (!deletedOnPrimary) {
+obj.processEvent(Snapshot.Event.OperationFailed);
+} else {
+obj.processEvent(Snapshot.Event.OperationSucceeded);
+}
+} catch (NoTransitionException e) {
+s_logger.debug("Failed to set the state to destroying: ", e);
+return false;
+}
Review comment:
and than factor this bit out again?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
