You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by anshul1886 <gi...@git.apache.org> on 2015/09/09 10:56:00 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
GitHub user anshul1886 opened a pull request:
https://github.com/apache/cloudstack/pull/793
CLOUDSTACK-8827: Move the VM snapshots stuck in transitional states to stable states during managment server restart
i.e. If snapshot in creating state move it to Error state and if it is in reverting state move it to Ready state.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 CLOUDSTACK-8827
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/793.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #793
----
commit c19bdcaf0847d23e3a5ee1daf374bf9173d86290
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-09-09T08:51:15Z
CLOUDSTACK-8827: Move the VM snapshots stuck in transitional states to stable state during managment server restart
i.e. If snapshot in creating state move it to Error state and if it is in reverting state move it to Ready state.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/793#issuecomment-139524505
@anshul1886 LGTM, can you add some unit test for the new methods?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/793#issuecomment-216188660
@anshul1886 please rebase against latest master and push -f, update on status of your PR
Can you add unit test for the changes, thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/793#issuecomment-139980724
you are decreasing the code coverage with these changes. especially with interface changes and new methods, I think unittests should be there.
This is not the final state of this code. It will go through multiple iterations with different developers touching it and there should be a check that the code you have written behaves in the expected way even after those changes.
for unittests, you could mock jobMgr to return failed jobs with snapshots(and some other jobs as well) in different states and then assert what you are expecting the code to do actually happens. There can be some cases where the deserialization might fail etc. you could test those cases as well.
For example, the current code will fail with NPE if jobMgr.findFailureAsyncJobs returns null. I dont know if it can, there is not javadoc describing its return value. It will be good to write a unit test for this case and assert that you code doesnt cause system error if that happens.
You could also add marvin test case to cover this scenario. Basically, there should be an automated way to test the code you have written.
Also, please post the test results(manual or automatic) you already executed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/793#discussion_r39367811
--- Diff: server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java ---
@@ -1046,4 +1046,51 @@ private VmWorkJobVO createPlaceHolderWork(long instanceId) {
return workJob;
}
+
+ @Override
+ public void cleanupVmSnapshotJobs(){
+ List<AsyncJobVO> jobs = _jobMgr.findFailureAsyncJobs(VmWorkCreateVMSnapshot.class.getName(), VmWorkRevertToVMSnapshot.class.getName());
+
+ for (AsyncJobVO job : jobs) {
+ try {
+ if (job.getCmd().equalsIgnoreCase(VmWorkCreateVMSnapshot.class.getName())) {
+ VmWorkCreateVMSnapshot work = VmWorkSerializer.deserialize(VmWorkCreateVMSnapshot.class, job.getCmdInfo());
+ cleanupVmSnapshotCreateFailure(work.getVmSnapshotId());
+ } else if(job.getCmd().equalsIgnoreCase(VmWorkRevertToVMSnapshot.class.getName())) {
+ VmWorkRevertToVMSnapshot work = VmWorkSerializer.deserialize(VmWorkRevertToVMSnapshot.class, job.getCmdInfo());
+ cleanupVmSnapshotRevertFailure(work.getVmSnapshotId());
+ }
+ } catch (Exception e) {
--- End diff --
Basically I don't care about specific exception I am getting here and in any case of exception I will continue. That's why I am catching generic exception and logging the specific exception in logs for debugging purposes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/793#issuecomment-143590468
@anshul1886 Any update on this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #793: CLOUDSTACK-8827: Move the VM snapshots stuck in trans...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/793
### ACS CI BVT Run
**Sumarry:**
Build Number 129
Hypervisor xenserver
NetworkType Advanced
Passed=73
Failed=0
Skipped=3
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_vpc_vpn.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #793: CLOUDSTACK-8827: Move the VM snapshots stuck i...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 closed the pull request at:
https://github.com/apache/cloudstack/pull/793
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #793: CLOUDSTACK-8827: Move the VM snapshots stuck in trans...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/793
@anshul1886 I am curious if there is a way to cancel stuck vmsnapshot jobs without restarting management server. E.g. VM deployment go to error start upon any failure in the process.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/793#discussion_r39365314
--- Diff: server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java ---
@@ -1046,4 +1046,51 @@ private VmWorkJobVO createPlaceHolderWork(long instanceId) {
return workJob;
}
+
+ @Override
+ public void cleanupVmSnapshotJobs(){
+ List<AsyncJobVO> jobs = _jobMgr.findFailureAsyncJobs(VmWorkCreateVMSnapshot.class.getName(), VmWorkRevertToVMSnapshot.class.getName());
+
+ for (AsyncJobVO job : jobs) {
+ try {
+ if (job.getCmd().equalsIgnoreCase(VmWorkCreateVMSnapshot.class.getName())) {
+ VmWorkCreateVMSnapshot work = VmWorkSerializer.deserialize(VmWorkCreateVMSnapshot.class, job.getCmdInfo());
+ cleanupVmSnapshotCreateFailure(work.getVmSnapshotId());
+ } else if(job.getCmd().equalsIgnoreCase(VmWorkRevertToVMSnapshot.class.getName())) {
+ VmWorkRevertToVMSnapshot work = VmWorkSerializer.deserialize(VmWorkRevertToVMSnapshot.class, job.getCmdInfo());
+ cleanupVmSnapshotRevertFailure(work.getVmSnapshotId());
+ }
+ } catch (Exception e) {
--- End diff --
what exception do you expect here? Is it the deserialization failure or any thing else? Can you catch specific exceptions?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/793#issuecomment-139989420
@karuturi I don't get what will be validate by adding above kind of tests other than what I have mentioned in my previous comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/793#issuecomment-139975023
@bhaisaab Here we are just accessing and updating the DB and not much logic is involved. So I can think of tests which does data validation of DB which will basically mean we are testing different unit then the unit which we have at hand. So to me unit tests will not add any value here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/793#issuecomment-143647389
Rebased the PR to current master.
Regarding unit tests, method doesn't have input or output as a unit. It has output only in terms of DB updation so for me useful unit tests for these methods can not be written.
http://www.petrikainulainen.net/programming/testing/writing-tests-for-data-access-code-unit-tests-are-waste/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/793#issuecomment-175666104
@anshul1886 ping
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #793: CLOUDSTACK-8827: Move the VM snapshots stuck i...
Posted by anshul1886 <gi...@git.apache.org>.
GitHub user anshul1886 reopened a pull request:
https://github.com/apache/cloudstack/pull/793
CLOUDSTACK-8827: Move the VM snapshots stuck in transitional states to stable states during managment server restart
i.e. If snapshot in creating state move it to Error state and if it is in reverting state move it to Ready state.
Manual tests performed
Take VM snapshot of VM
stop the management server
Verify in DB that VM snapshot is in creating state
Start the management server
Verify that snapshot moves to ERROR state.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 CLOUDSTACK-8827
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/793.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #793
----
commit 656e855792737318b7bb5c84eea7a2a30eb6ad71
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-09-09T08:51:15Z
CLOUDSTACK-8827: Move the VM snapshots stuck in transitional states to stable state during managment server restart
i.e. If snapshot in creating state move it to Error state and if it is in reverting state move it to Ready state.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8827: Move the VM snapshots st...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/793#issuecomment-140003171
@anshul1886 I do not agree to your statement and hence I already tried to explain what you can test in this piece of code. If you still dont get it(or dont want to understand it) I cannot explain.
I really dont understand why you dont see a value in unittests. I dont want to explain the benefits of unit testing here (I neither have time nor do I have belief that you will understand).
Please add a marvin test on what you are achieving by this change. As much as possible, we need an automated way to test new code.
Also, add information what tests you already performed to ensure the code works.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #793: CLOUDSTACK-8827: Move the VM snapshots stuck in trans...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/793
@rhtyd Rebased the branch to latest master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #793: CLOUDSTACK-8827: Move the VM snapshots stuck in trans...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/793
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #793: CLOUDSTACK-8827: Move the VM snapshots stuck in trans...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/793
@koushik-das @sateesh-chodapuneedi @ramkatru @karuturi @rhtyd Regarding unit tests, I can't see any useful tests can be written here which I have already specified the reason for. Also we have already lot of unit tests which are wasting time to fix. Every now and then I am coming across unit tests which are not written in proper way and leads to waste of effort. As far as my understanding of unit test goes they are to reduce effort not increase. One example is in this PR's last jenkins run. That build was failed because one test is trying to download/copy file and that failed. As per my understanding unit tests should not do any thing which are dependent on external environments failure. As those kind of failures are bound to happen. As when those happen we have to go and check why that is failed wasting some time to understand. So ultimately we are wasting lot of effort currently because of tests not properly written.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #793: CLOUDSTACK-8827: Move the VM snapshots stuck in trans...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/793
@serg38 That case is already handled. Issue arises if management server goes down then we didn't get chance to clean things up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #793: CLOUDSTACK-8827: Move the VM snapshots stuck in trans...
Posted by cloudmonger <gi...@git.apache.org>.
Github user cloudmonger commented on the issue:
https://github.com/apache/cloudstack/pull/793
### ACS CI BVT Run
**Sumarry:**
Build Number 436
Hypervisor xenserver
NetworkType Advanced
Passed=103
Failed=2
Skipped=7
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* test_non_contigiousvlan.py
* test_extendPhysicalNetworkVlan Failed
* test_routers_network_ops.py
* test_01_isolate_network_FW_PF_default_routes_egress_true Failed
**Skipped tests:**
test_01_test_vm_volume_snapshot
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_11_ss_nfs_version_on_ssvm
test_nested_virtualization_vmware
test_3d_gpu_support
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_loadbalance.py
test_routers.py
test_reset_vm_on_reboot.py
test_deploy_vms_with_varied_deploymentplanners.py
test_network.py
test_router_dns.py
test_login.py
test_deploy_vm_iso.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #793: CLOUDSTACK-8827: Move the VM snapshots stuck in trans...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/793
@blueorangutan package
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---