You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/10/26 07:51:47 UTC

[GitHub] [cloudstack] Spaceman1984 opened a new pull request #4425: Setting snapshot state to error on timeout

Spaceman1984 opened a new pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425


   ## Description
   <!--- Describe your changes in detail -->
   When creating a snapshot times-out on the management server, the record in snapshot_store_ref table is deleted.
   
   By changing the snapshot state to "error" on timeout, the record is not deleted.
   
   <!-- For new features, provide link to FS, dev ML discussion etc. -->
   <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
   
   <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
   <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
   <!-- Fixes: # -->
   Fixes: #3559 
   ## Types of changes
   <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [ ] Bug fix (non-breaking change which fixes an issue)
   - [x] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ## Screenshots (if appropriate):
   
   ## How Has This Been Tested?
   <!-- Please describe in detail how you tested your changes. -->
   <!-- Include details of your testing environment, and the tests you ran to -->
   <!-- see how your change affects other areas of the code, etc. -->
   
   * Set the global variable: backup.snapshot.wait to 2 seconds.
   * Create a volume snapshot.
   * Check the snapshots table for a snapshot record and check for a corresponding record in snapshot_store_ref table.
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-729473904


   <b>Trillian test result (tid-3194)</b>
   Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 38791 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4425-t3194-vmware-65u2.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Smoke tests completed. 85 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_03_deploy_and_upgrade_kubernetes_cluster | `Failure` | 777.49 | test_kubernetes_clusters.py
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r519859781



##########
File path: server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
##########
@@ -553,6 +553,25 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo
         }
     }
 
+    public void markFailedSnapshot(long snapshotId) {
+        Account caller = CallContext.current().getCallingAccount();
+
+        // Verify parameters
+        SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId);
+
+        if (snapshotCheck == null) {
+            throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId);
+        }
+
+        _accountMgr.checkAccess(caller, null, true, snapshotCheck);
+
+        snapshotCheck.setState(Snapshot.State.Error);
+        _snapshotDao.update(snapshotId, snapshotCheck);
+        // Setting removed to prevent record from being deleted by garbage collection.
+        _snapshotDao.remove(snapshotId);

Review comment:
       should this be in a transaction to be sure?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-722968844


   @Spaceman1984 please update if this is ready for review?


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-728940437


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 removed a comment on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 removed a comment on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730166716


   @blueorangutan test matrix


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-729466024


   <b>Trillian test result (tid-3192)</b>
   Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 40017 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4425-t3192-xenserver-71.zip
   Intermittent failure detected: /marvin/tests/smoke/test_scale_vm.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
   Smoke tests completed. 84 look OK, 2 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_scale_vm | `Failure` | 10.38 | test_scale_vm.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 538.00 | test_vpc_redundant.py
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730020737


   @blueorangutan package
   
   @sureshanaparti @borisstoyanov any final comments 


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-729018374


   @borisstoyanov this pertains to your #3559. Can you give the final stamp of approval?


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r521158220



##########
File path: server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
##########
@@ -553,6 +553,25 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo
         }
     }
 
+    public void markFailedSnapshot(long snapshotId) {
+        Account caller = CallContext.current().getCallingAccount();
+
+        // Verify parameters
+        SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId);
+
+        if (snapshotCheck == null) {
+            throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId);
+        }
+
+        _accountMgr.checkAccess(caller, null, true, snapshotCheck);
+
+        snapshotCheck.setState(Snapshot.State.Error);

Review comment:
       > Not sure what you mean @sureshanaparti
   
   @Spaceman1984 the caller of _copyAsync_ method would set a callback method to process the result and set the object state accordingly. Can you use that callback method (of snapshot copy operation) to set the state, through the event (check the snapshot state machine) ?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r521139517



##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
##########
@@ -587,6 +590,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) {
             if (cacheData != null) {
                 cacheMgr.deleteCacheObject(cacheData);
             }
+            _snapshotService.markFailedSnapshot(destData.getId());

Review comment:
       @Spaceman1984 the copy callback should mark the snapshot state accordingly, based on the operation result (success / fail). Not required to set the state explicitly.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-724397012


   <b>Trillian test result (tid-3136)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 39367 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4425-t3136-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Smoke tests completed. 84 look OK, 2 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_07_deploy_kubernetes_ha_cluster | `Failure` | 3609.31 | test_kubernetes_clusters.py
   test_08_deploy_and_upgrade_kubernetes_ha_cluster | `Failure` | 0.10 | test_kubernetes_clusters.py
   test_09_delete_kubernetes_ha_cluster | `Failure` | 0.09 | test_kubernetes_clusters.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 97.92 | test_kubernetes_clusters.py
   test_04_rvpc_privategw_static_routes | `Failure` | 244.19 | test_privategw_acl.py
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r526111484



##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
##########
@@ -587,6 +590,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) {
             if (cacheData != null) {
                 cacheMgr.deleteCacheObject(cacheData);
             }
+            _snapshotService.markRemovedSnapshot(destData.getId());

Review comment:
       I think having the snapshot marked as removed here in the case of any exception is better than just checking for a timeout. I think if the process fails for any reason we would want to keep the db records.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-716391767


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2276


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-717263958


   I'll convert to draft @DaanHoogland 


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730257173


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2408


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-724003686


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730021437


   @rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-728940181


   > code looks good, it would be just one of many fixes i imagine and also; is AncientDataMotionStrategy the only place to do this? aren't there vmware or kvm specific places as well?
   
   I tested this specifically with a vmware environment as the original issue was reported on vmware @DaanHoogland. 


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730221925


   @blueorangutan test matrix


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r526638581



##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
##########
@@ -587,6 +590,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) {
             if (cacheData != null) {
                 cacheMgr.deleteCacheObject(cacheData);
             }
+            _snapshotService.markRemovedSnapshot(destData.getId());

Review comment:
       @Spaceman1984 possible to move this method call to copy snapshot callback method on result failure?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-729133484


   @sureshanaparti please validate you concerns are indeed met.


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-724004011


   @DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r525170236



##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
##########
@@ -587,6 +590,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) {
             if (cacheData != null) {
                 cacheMgr.deleteCacheObject(cacheData);
             }
+            _snapshotService.markFailedSnapshot(destData.getId());

Review comment:
       Addressed




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r525903477



##########
File path: server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
##########
@@ -553,6 +553,22 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo
         }
     }
 
+    public void markRemovedSnapshot(long snapshotId) {
+        Account caller = CallContext.current().getCallingAccount();
+
+        // Verify parameters
+        SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId);
+
+        if (snapshotCheck == null) {
+            throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId);
+        }
+
+        _accountMgr.checkAccess(caller, null, true, snapshotCheck);

Review comment:
       move caller here.
   
   ```suggestion
           Account caller = CallContext.current().getCallingAccount();
           _accountMgr.checkAccess(caller, null, true, snapshotCheck);
   ```




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r521142771



##########
File path: server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
##########
@@ -553,6 +553,25 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo
         }
     }
 
+    public void markFailedSnapshot(long snapshotId) {
+        Account caller = CallContext.current().getCallingAccount();
+
+        // Verify parameters
+        SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId);
+
+        if (snapshotCheck == null) {
+            throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId);
+        }
+
+        _accountMgr.checkAccess(caller, null, true, snapshotCheck);
+
+        snapshotCheck.setState(Snapshot.State.Error);

Review comment:
       Not sure what you mean @sureshanaparti 




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-717738417


   Snapshots are being deleted from the database when using the "Async" flag on creation and it times out. - This is happening in a separate process - I suspect the snapshots are being marked for garbage collection in the async process. I'll need to look into this a bit further.


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r521140625



##########
File path: server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
##########
@@ -553,6 +553,25 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo
         }
     }
 
+    public void markFailedSnapshot(long snapshotId) {
+        Account caller = CallContext.current().getCallingAccount();
+
+        // Verify parameters
+        SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId);
+
+        if (snapshotCheck == null) {
+            throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId);
+        }
+
+        _accountMgr.checkAccess(caller, null, true, snapshotCheck);
+
+        snapshotCheck.setState(Snapshot.State.Error);

Review comment:
       @Spaceman1984 can you handle this in the copy callback, and pass the proper event to set the snapshot state?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-729271669


   <b>Trillian test result (tid-3190)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 29004 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4425-t3190-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Smoke tests completed. 85 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_04_rvpc_network_garbage_collector_nics | `Failure` | 270.32 | test_vpc_redundant.py
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730693491


   <b>Trillian test result (tid-3216)</b>
   Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 38018 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4425-t3216-vmware-65u2.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Smoke tests completed. 85 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_03_deploy_and_upgrade_kubernetes_cluster | `Failure` | 777.31 | test_kubernetes_clusters.py
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730996878


   prety sure the kvm failures are environmental, rerunning that one
   @blueorangutan test centos7 kvm-centos7


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-716376081


   @Spaceman1984 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r526697772



##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
##########
@@ -587,6 +590,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) {
             if (cacheData != null) {
                 cacheMgr.deleteCacheObject(cacheData);
             }
+            _snapshotService.markRemovedSnapshot(destData.getId());

Review comment:
       Thanks @sureshanaparti for pointing me in that direction, I was able to get rid of that method.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r520656350



##########
File path: server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
##########
@@ -553,6 +553,25 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo
         }
     }
 
+    public void markFailedSnapshot(long snapshotId) {
+        Account caller = CallContext.current().getCallingAccount();
+
+        // Verify parameters
+        SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId);
+
+        if (snapshotCheck == null) {
+            throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId);
+        }
+
+        _accountMgr.checkAccess(caller, null, true, snapshotCheck);
+
+        snapshotCheck.setState(Snapshot.State.Error);
+        _snapshotDao.update(snapshotId, snapshotCheck);
+        // Setting removed to prevent record from being deleted by garbage collection.
+        _snapshotDao.remove(snapshotId);

Review comment:
       ping @Spaceman1984 




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-717846958


   @Spaceman1984 can you share the latest update on this, is this ready for review/testing - or you still investigating?


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-728969404


   @blueorangutan test matrix


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730166716


   @blueorangutan test matrix


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-731334776


   <b>Trillian test result (tid-3221)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 35467 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4425-t3221-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_list_ids_parameter.py
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Smoke tests completed. 86 look OK, 0 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r525902440



##########
File path: server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
##########
@@ -553,6 +553,22 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo
         }
     }
 
+    public void markRemovedSnapshot(long snapshotId) {

Review comment:
       _markRemovedSnapshot_ indicates the snapshot is already removed. I think _removeSnapshot_ (or _markSnapshotAsRemoved_) is ok. 




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730260139


   @Spaceman1984 a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730222509


   @DaanHoogland a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730577367


   <b>Trillian test result (tid-3209)</b>
   Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 31247 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4425-t3209-xenserver-71.zip
   Intermittent failure detected: /marvin/tests/smoke/test_scale_vm.py
   Smoke tests completed. 85 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_scale_vm | `Failure` | 9.34 | test_scale_vm.py
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-717862911


   Still investigating @rhtyd 


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r525907352



##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
##########
@@ -587,6 +590,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) {
             if (cacheData != null) {
                 cacheMgr.deleteCacheObject(cacheData);
             }
+            _snapshotService.markRemovedSnapshot(destData.getId());

Review comment:
       Can you check for OperationTimedoutException (if thrown for timeout?), set removed accordingly. Note that, the snapshot here is not in Error state as the copy callback is not yet to be called, which takes the action based on the operation failure.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730259737


   @blueorangutan test matrix


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-720325091


   @Spaceman1984 any update on this?


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-724023734


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2346


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730999866


   @DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r526638581



##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
##########
@@ -587,6 +590,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) {
             if (cacheData != null) {
                 cacheMgr.deleteCacheObject(cacheData);
             }
+            _snapshotService.markRemovedSnapshot(destData.getId());

Review comment:
       @Spaceman1984 possible to move this method call "__snapshotService.markSnapshotAsRemoved(destData.getId())_" to copy snapshot callback method on result failure?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730237083


   @Spaceman1984 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r525169757



##########
File path: server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
##########
@@ -553,6 +553,25 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo
         }
     }
 
+    public void markFailedSnapshot(long snapshotId) {
+        Account caller = CallContext.current().getCallingAccount();
+
+        // Verify parameters
+        SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId);
+
+        if (snapshotCheck == null) {
+            throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId);
+        }
+
+        _accountMgr.checkAccess(caller, null, true, snapshotCheck);
+
+        snapshotCheck.setState(Snapshot.State.Error);

Review comment:
       I have removed setting the error state, this leaves the snapshot in a "backing up" state forever which is fine.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 edited a comment on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 edited a comment on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-720390279






----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-731241171


   @DaanHoogland let's merge this once the test results are back we've enough LGTMs and manual tests done


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r525046235



##########
File path: server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
##########
@@ -553,6 +553,25 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo
         }
     }
 
+    public void markFailedSnapshot(long snapshotId) {
+        Account caller = CallContext.current().getCallingAccount();
+
+        // Verify parameters
+        SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId);
+
+        if (snapshotCheck == null) {
+            throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId);
+        }
+
+        _accountMgr.checkAccess(caller, null, true, snapshotCheck);
+
+        snapshotCheck.setState(Snapshot.State.Error);

Review comment:
       I'll take a look.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#discussion_r520866018



##########
File path: server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
##########
@@ -553,6 +553,25 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo
         }
     }
 
+    public void markFailedSnapshot(long snapshotId) {
+        Account caller = CallContext.current().getCallingAccount();
+
+        // Verify parameters
+        SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId);
+
+        if (snapshotCheck == null) {
+            throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId);
+        }
+
+        _accountMgr.checkAccess(caller, null, true, snapshotCheck);
+
+        snapshotCheck.setState(Snapshot.State.Error);
+        _snapshotDao.update(snapshotId, snapshotCheck);
+        // Setting removed to prevent record from being deleted by garbage collection.
+        _snapshotDao.remove(snapshotId);

Review comment:
       ping @davidjumani @GabrielBrascher @rhtyd the two statements above should be grouped in a transaction. agree?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-716786913


   <b>Trillian test result (tid-3075)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 37527 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4425-t3075-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Smoke tests completed. 84 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_03_deploy_and_upgrade_kubernetes_cluster | `Failure` | 284.61 | test_kubernetes_clusters.py
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-716407585


   @Spaceman1984 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-716406830


   @blueorangutan test


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730043878


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2405


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-717252882


   > Discussed an issue with @Spaceman1984
   > Might be unrelated, Waiting for his response
   
   so this is on hold, @davidjumani ?


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd merged pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425


   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-728970246


   @Spaceman1984 a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730236570


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-724041501


   @Spaceman1984 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-728963339


   @blueorangutan test help


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-728961533


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2394


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-728941247


   @Spaceman1984 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-716375728


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-720390279






----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4425: Setting snapshot removed on timeout

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-730779738


   <b>Trillian test result (tid-3217)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 41875 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4425-t3217-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_certauthority_root.py
   Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
   Intermittent failure detected: /marvin/tests/smoke/test_iso.py
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
   Intermittent failure detected: /marvin/tests/smoke/test_list_ids_parameter.py
   Intermittent failure detected: /marvin/tests/smoke/test_loadbalance.py
   Intermittent failure detected: /marvin/tests/smoke/test_metrics_api.py
   Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
   Intermittent failure detected: /marvin/tests/smoke/test_nested_virtualization.py
   Intermittent failure detected: /marvin/tests/smoke/test_network_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_network.py
   Intermittent failure detected: /marvin/tests/smoke/test_password_server.py
   Intermittent failure detected: /marvin/tests/smoke/test_portforwardingrules.py
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_projects.py
   Intermittent failure detected: /marvin/tests/smoke/test_reset_vm_on_reboot.py
   Smoke tests completed. 70 look OK, 16 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_provision_certificate | `Error` | 121.05 | test_certauthority_root.py
   ContextSuite context=TestInternalLb>:setup | `Error` | 0.00 | test_internal_lb.py
   test_01_1_create_iso_with_checksum_sha1_negative | `Error` | 66.51 | test_iso.py
   test_01_create_iso_with_checksum_sha1 | `Error` | 65.42 | test_iso.py
   test_01_create_iso_with_checksum_sha1 | `Error` | 66.50 | test_iso.py
   test_02_1_create_iso_with_checksum_sha256_negative | `Error` | 66.50 | test_iso.py
   test_02_create_iso_with_checksum_sha256 | `Error` | 65.42 | test_iso.py
   test_02_create_iso_with_checksum_sha256 | `Error` | 66.51 | test_iso.py
   test_03_1_create_iso_with_checksum_md5_negative | `Error` | 66.49 | test_iso.py
   test_03_create_iso_with_checksum_md5 | `Error` | 65.41 | test_iso.py
   test_03_create_iso_with_checksum_md5 | `Error` | 66.50 | test_iso.py
   test_04_create_iso_with_no_checksum | `Error` | 65.42 | test_iso.py
   test_04_create_iso_with_no_checksum | `Error` | 66.52 | test_iso.py
   test_01_create_iso | `Failure` | 1513.23 | test_iso.py
   ContextSuite context=TestISO>:setup | `Error` | 3027.46 | test_iso.py
   test_01_deploy_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
   test_02_invalid_upgrade_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
   test_03_deploy_and_upgrade_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
   test_04_deploy_and_scale_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
   test_05_delete_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
   test_06_deploy_invalid_kubernetes_ha_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
   test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
   test_08_deploy_and_upgrade_kubernetes_ha_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
   test_09_delete_kubernetes_ha_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
   test_01_add_delete_kubernetes_supported_version | `Error` | 1802.56 | test_kubernetes_supported_versions.py
   ContextSuite context=TestListIdsParams>:setup | `Error` | 0.00 | test_list_ids_parameter.py
   ContextSuite context=TestLoadBalance>:setup | `Error` | 0.00 | test_loadbalance.py
   test_list_vms_metrics | `Error` | 0.19 | test_metrics_api.py
   test_nic_secondaryip_add_remove | `Error` | 0.07 | test_multipleips_per_nic.py
   ContextSuite context=TestNestedVirtualization>:setup | `Error` | 0.00 | test_nested_virtualization.py
   ContextSuite context=TestNetworkACL>:setup | `Error` | 0.00 | test_network_acl.py
   test_delete_account | `Error` | 0.62 | test_network.py
   test_delete_network_while_vm_on_it | `Error` | 0.08 | test_network.py
   test_deploy_vm_l2network | `Error` | 0.07 | test_network.py
   test_l2network_restart | `Error` | 1.29 | test_network.py
   ContextSuite context=TestPortForwarding>:setup | `Error` | 3.60 | test_network.py
   ContextSuite context=TestPublicIP>:setup | `Error` | 1.43 | test_network.py
   test_reboot_router | `Error` | 0.07 | test_network.py
   test_releaseIP | `Error` | 0.64 | test_network.py
   ContextSuite context=TestRouterRules>:setup | `Error` | 0.71 | test_network.py
   ContextSuite context=TestIsolatedNetworksPasswdServer>:setup | `Error` | 0.00 | test_password_server.py
   ContextSuite context=TestPortForwardingRules>:setup | `Error` | 0.00 | test_portforwardingrules.py
   ContextSuite context=TestPrivateGwACL>:setup | `Error` | 0.00 | test_privategw_acl.py
   test_09_project_suspend | `Error` | 1.11 | test_projects.py
   test_10_project_activation | `Error` | 1.09 | test_projects.py
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4425: Setting snapshot state to error on timeout

Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4425:
URL: https://github.com/apache/cloudstack/pull/4425#issuecomment-724040925


   @blueorangutan test


----------------------------------------------------------------
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:
users@infra.apache.org