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 2021/06/18 20:55:06 UTC

[GitHub] [cloudstack] slavkap opened a new pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

slavkap opened a new pull request #5130:
URL: https://github.com/apache/cloudstack/pull/5130






-- 
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] weizhouapache commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   @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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   @weizhouapache 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] blueorangutan commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   @weizhouapache 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] rhtyd closed pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   


-- 
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] GabrielBrascher commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   Thanks for the PR @slavkap.
   I will test it on a Ceph env.


-- 
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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   @weizhouapache 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] blueorangutan commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 346


-- 
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] weizhouapache commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   @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] weizhouapache commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   @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] weizhouapache closed pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

Posted by GitBox <gi...@apache.org>.
weizhouapache closed pull request #5130:
URL: https://github.com/apache/cloudstack/pull/5130


   


-- 
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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   @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] blueorangutan commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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






-- 
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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   @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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 301


-- 
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] GabrielBrascher commented on a change in pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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



##########
File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/CephSnapshotStrategy.java
##########
@@ -64,10 +64,6 @@ public StrategyPriority canHandle(Snapshot snapshot, SnapshotOperation op) {
             return StrategyPriority.HIGHEST;
         }
 
-        if (SnapshotOperation.DELETE.equals(op)) {
-            return StrategyPriority.HIGHEST;
-        }
-

Review comment:
       @sureshanaparti thanks for the code review. I would like to just add a note to avoid misunderstandings.
   
   Delete operation for Ceph has always been supported by CephSnapshotStrategy due to the fact that the _parent_ class of CephSnapshotStrategy is StorageSystemSnapshotStrategy, which handles the deletion of Ceph/RBD snapshots.
   
   However, StorageSystemSnapshotStrategy does not handle backed snapshots via NFS in this context and when Ceph had features introduced the backup on secondary storage was not taken into account.
   
   With the fix proposed by @slavkap, it was possible to check that **nowadays** DefaultSnapshotStrategy supports deletions of Snapshots on Ceph (with or without backup on NFS).




-- 
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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   enough :+1:  but I'll rerun the smoke tests anyway to make sure these last three failures are spurious.


-- 
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] weizhouapache commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   > enough  but I'll rerun the smoke tests anyway to make sure these last three failures are spurious.
   
   @DaanHoogland 
   we had the failures in many smoke tests, which should not be related to this pr.
   we need to investigate the failures. what do you think @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] rhtyd commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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






-- 
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] slavkap commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   Thank you, @GabrielBrascher, for your time to test and code 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] blueorangutan removed a comment on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   <b>Trillian test result (tid-1027)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 53251 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5130-t1027-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_network.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
   Smoke tests completed. 83 look OK, 4 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_reboot_router | `Failure` | 439.91 | test_network.py
   test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | `Error` | 425.07 | test_internal_lb.py
   test_01_invalid_upgrade_kubernetes_cluster | `Failure` | 3608.29 | test_kubernetes_clusters.py
   test_02_deploy_and_upgrade_kubernetes_cluster | `Failure` | 3607.74 | test_kubernetes_clusters.py
   test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_04_basic_lifecycle_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_05_delete_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
   test_08_deploy_and_upgrade_kubernetes_ha_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_09_delete_kubernetes_ha_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 88.70 | test_kubernetes_clusters.py
   test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 489.31 | test_vpc_redundant.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 419.18 | test_vpc_redundant.py
   test_05_rvpc_multi_tiers | `Failure` | 464.05 | 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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   Failures don't seem related to this PR. Merging. 


-- 
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] GabrielBrascher commented on a change in pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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



##########
File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/CephSnapshotStrategy.java
##########
@@ -64,10 +64,6 @@ public StrategyPriority canHandle(Snapshot snapshot, SnapshotOperation op) {
             return StrategyPriority.HIGHEST;
         }
 
-        if (SnapshotOperation.DELETE.equals(op)) {
-            return StrategyPriority.HIGHEST;
-        }
-

Review comment:
       @sureshanaparti thanks for the code review. I would like to just add a note to avoid misunderstandings.
   
   Delete operation for Ceph has always been supported by CephSnapshotStrategy due to the fact that the _parent_ class of CephSnapshotStrategy is StorageSystemSnapshotStrategy, which handles the deletion of Ceph/RBD snapshots.
   
   However, StorageSystemSnapshotStrategy does not handle backed snapshots via NFS in this context and when Ceph had features introduced the backup on secondary storage was not taken into account.
   
   With the fix proposed by @slavkap, it was possible to check that **nowadays** DefaultSnapshotStrategy supports deletions of Snapshots on Ceph (with or without backup on NFS).




-- 
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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   <b>Trillian test result (tid-1027)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 53251 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5130-t1027-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_network.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
   Smoke tests completed. 83 look OK, 4 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_reboot_router | `Failure` | 439.91 | test_network.py
   test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | `Error` | 425.07 | test_internal_lb.py
   test_01_invalid_upgrade_kubernetes_cluster | `Failure` | 3608.29 | test_kubernetes_clusters.py
   test_02_deploy_and_upgrade_kubernetes_cluster | `Failure` | 3607.74 | test_kubernetes_clusters.py
   test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_04_basic_lifecycle_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_05_delete_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
   test_08_deploy_and_upgrade_kubernetes_ha_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_09_delete_kubernetes_ha_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 88.70 | test_kubernetes_clusters.py
   test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 489.31 | test_vpc_redundant.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 419.18 | test_vpc_redundant.py
   test_05_rvpc_multi_tiers | `Failure` | 464.05 | 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] slavkap commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   Thank you all for the reviews! OK @DaanHoogland, all tests have failed with this - Failed to SSH into VM (which I guess it's not related to)


-- 
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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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



##########
File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/CephSnapshotStrategy.java
##########
@@ -64,10 +64,6 @@ public StrategyPriority canHandle(Snapshot snapshot, SnapshotOperation op) {
             return StrategyPriority.HIGHEST;
         }
 
-        if (SnapshotOperation.DELETE.equals(op)) {
-            return StrategyPriority.HIGHEST;
-        }
-

Review comment:
       code LGTM, as delete op is not handled here, it shouldn't consider this strategy.




-- 
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] weizhouapache commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   @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] slavkap commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   Thank you all for the reviews! OK @DaanHoogland, all tests have failed with this - Failed to SSH into VM (which I guess it's not related to)


-- 
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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   <b>Trillian test result (tid-1082)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 48808 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5130-t1082-kvm-centos7.zip
   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_privategw_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_router_dhcphosts.py
   Intermittent failure detected: /marvin/tests/smoke/test_service_offerings.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
   Smoke tests completed. 85 look OK, 2 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_router_dhcphosts | `Failure` | 11.70 | test_router_dhcphosts.py
   ContextSuite context=TestRouterDHCPHosts>:teardown | `Error` | 23.14 | test_router_dhcphosts.py
   test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 482.21 | test_vpc_redundant.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 488.66 | test_vpc_redundant.py
   test_05_rvpc_multi_tiers | `Failure` | 477.87 | 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] weizhouapache commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   > enough  but I'll rerun the smoke tests anyway to make sure these last three failures are spurious.
   
   @DaanHoogland 
   we had the failures in many smoke tests, which should not be related to this pr.
   we need to investigate the failures. what do you think @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] blueorangutan commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   <b>Trillian test result (tid-1027)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 53251 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5130-t1027-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_network.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
   Smoke tests completed. 83 look OK, 4 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_reboot_router | `Failure` | 439.91 | test_network.py
   test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | `Error` | 425.07 | test_internal_lb.py
   test_01_invalid_upgrade_kubernetes_cluster | `Failure` | 3608.29 | test_kubernetes_clusters.py
   test_02_deploy_and_upgrade_kubernetes_cluster | `Failure` | 3607.74 | test_kubernetes_clusters.py
   test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_04_basic_lifecycle_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_05_delete_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
   test_08_deploy_and_upgrade_kubernetes_ha_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_09_delete_kubernetes_ha_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 88.70 | test_kubernetes_clusters.py
   test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 489.31 | test_vpc_redundant.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 419.18 | test_vpc_redundant.py
   test_05_rvpc_multi_tiers | `Failure` | 464.05 | 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] slavkap commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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






-- 
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] Pearl1594 commented on pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   @slavkap there are some unit test failures.
   ```
   [INFO] Running org.apache.cloudstack.storage.snapshot.CephSnapshotStrategyTest
   [ERROR] Tests run: 5, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.208 s <<< FAILURE! - in org.apache.cloudstack.storage.snapshot.CephSnapshotStrategyTest
   [ERROR] canHandleTestReomvedAndSnapshotStoredOnRbd(org.apache.cloudstack.storage.snapshot.CephSnapshotStrategyTest)  Time elapsed: 0.006 s  <<< FAILURE!
   java.lang.AssertionError: expected:<HIGHEST> but was:<CANT_HANDLE>
   	at org.apache.cloudstack.storage.snapshot.CephSnapshotStrategyTest.configureAndVerifyCanHandle(CephSnapshotStrategyTest.java:91)
   	at org.apache.cloudstack.storage.snapshot.CephSnapshotStrategyTest.canHandleTestReomvedAndSnapshotStoredOnRbd(CephSnapshotStrategyTest.java:73)
   
   [ERROR] canHandleTestNotReomvedAndSnapshotStoredOnRbd(org.apache.cloudstack.storage.snapshot.CephSnapshotStrategyTest)  Time elapsed: 0.002 s  <<< FAILURE!
   java.lang.AssertionError: expected:<HIGHEST> but was:<CANT_HANDLE>
   	at org.apache.cloudstack.storage.snapshot.CephSnapshotStrategyTest.configureAndVerifyCanHandle(CephSnapshotStrategyTest.java:91)
   	at org.apache.cloudstack.storage.snapshot.CephSnapshotStrategyTest.canHandleTestNotReomvedAndSnapshotStoredOnRbd(CephSnapshotStrategyTest.java:58)
   
   [INFO] Running org.apache.cloudstack.storage.snapshot.SnapshotDataFactoryImplTest
   [INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.182 s - in org.apache.cloudstack.storage.snapshot.SnapshotDataFactoryImplTest
   [INFO] 
   [INFO] Results:
   [INFO] 
   [ERROR] Failures: 
   [ERROR]   CephSnapshotStrategyTest.canHandleTestNotReomvedAndSnapshotStoredOnRbd:58->configureAndVerifyCanHandle:91 expected:<HIGHEST> but was:<CANT_HANDLE>
   [ERROR]   CephSnapshotStrategyTest.canHandleTestReomvedAndSnapshotStoredOnRbd:73->configureAndVerifyCanHandle:91 expected:<HIGHEST> but was:<CANT_HANDLE>
   [INFO] 
   ```


-- 
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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   <b>Trillian test result (tid-1013)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 42004 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5130-t1013-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Smoke tests completed. 84 look OK, 3 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_02_vpc_privategw_static_routes | `Failure` | 977.05 | test_privategw_acl.py
   test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false | `Failure` | 305.08 | test_routers_network_ops.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 475.10 | 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 merged pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   


-- 
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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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


   enough :+1:  but I'll rerun the smoke tests anyway to make sure these last three failures are spurious.


-- 
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 #5130: Fix of delete of Ceph's snapshots from secondary storage

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



##########
File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/CephSnapshotStrategy.java
##########
@@ -64,10 +64,6 @@ public StrategyPriority canHandle(Snapshot snapshot, SnapshotOperation op) {
             return StrategyPriority.HIGHEST;
         }
 
-        if (SnapshotOperation.DELETE.equals(op)) {
-            return StrategyPriority.HIGHEST;
-        }
-

Review comment:
       code LGTM, as delete op is not handled here, it shouldn't consider this strategy.




-- 
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