You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by karuturi <gi...@git.apache.org> on 2016/12/15 11:17:06 UTC
[GitHub] cloudstack pull request #1832: cloudstack-9652 Job framework - Cancelling as...
GitHub user karuturi opened a pull request:
https://github.com/apache/cloudstack/pull/1832
cloudstack-9652 Job framework - Cancelling async jobs
enabled cancellation of long running or subsequent queued up async jobs
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Accelerite/cloudstack CLOUDSTACK-9652
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1832.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 #1832
----
commit 1099b3eb1adbb4dd028a86877986e3ac77eeee71
Author: Rajani Karuturi <ra...@accelerite.com>
Date: 2016-12-13T11:55:44Z
CLOUDSTACK-9652: added new cancel async job api
commit d9f4f6e898f063096d4bace46753b179c0bb68dc
Author: Rajani Karuturi <ra...@accelerite.com>
Date: 2016-07-12T05:45:32Z
CLOUDSTACK-9652 unittests for cancelAsyncJob cmd
commit 5093e592b7e71978f50ae8007c1da985758d6efd
Author: Rajani Karuturi <ra...@accelerite.com>
Date: 2016-12-14T09:35:37Z
CLOUDSTACK-9652 API to find long running jobs
added listlongrunnningjobs api
added listqueuedupasyncjobs api
commit 649f5b9237b85c1c2aba67065b13261ec4930487
Author: Rajani Karuturi <ra...@accelerite.com>
Date: 2016-12-14T10:14:24Z
CLOUDSTACK-9652 Cleanup at Agent Layer
Throwing an exception in agentattache incase the job is cancelled. The
top layers should handle the exception and take necessary action for
cleaning of resources.
commit cebbd36f94914f144f5cd9b016467d18f02c222c
Author: Rajani Karuturi <ra...@accelerite.com>
Date: 2016-12-14T10:34:12Z
CLOUDSTACK-9652 Annotating async APIs as cancellable or not
commit e2df57ca6ba642cf120295cf805d04881e7c14cf
Author: Rajani Karuturi <ra...@accelerite.com>
Date: 2016-12-14T11:43:53Z
CLOUDSTACK-9652: added OperationCancelledException for a cancelled job
Added exception handling at various agent commands for this new
checked exception.
commit ebfc75879deb9b7de775fe9d92f361d432852b91
Author: Rajani Karuturi <ra...@accelerite.com>
Date: 2016-07-22T09:38:11Z
CLOUDSTACK-9652 cancelling a job in queue should not throw exception
check the state of the parent job before submitting the worker thread.
starting work thread only if parent job is not done.
commit adb19cb2788706b66e7a1454327d541b2ba0eeae
Author: Rajani Karuturi <ra...@accelerite.com>
Date: 2016-12-15T09:53:43Z
CLOUDSTACK-9652 cleaning up async jobs on graceful MS shutdown
----
---
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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the issue:
https://github.com/apache/cloudstack/pull/1832
@marcaurele The thread which is running the job gets killed due to OperationCancelledException. But, if any command is already sent and is being executed on hypervisor, that wont be cancelled. check for the changes in AgentAttache where this new exception is thrown.
For example, if a deployvm is cancelled and the command is already sent to hypervisor, the vm will continue to launch on hypervisor. but, on cloudstack side, the threads and the jobs will be cleanedup and cancelled. Eventually, vm sync will sync the states. There can be instances when the job cancellation was successful but, the vm is in running state after sometime.
The cancellation should be used with caution only by admin keeping in mind that some resource cleanup on hypervisor might be required.
This will only unblock the jobs in cloudstack which are waiting for a long time for a certain job to complete.
---
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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the issue:
https://github.com/apache/cloudstack/pull/1832
> @karuturi Ok thanks for the clarifications, and it's the scenario I thought about too. That being said, I'm currently thinking of a new approach for the command sequencer because having implemented the live migration, the non-parallel commands isn't optimal at all when you have long running sequential commands on a hypervisor. And I tend to think that's the reason behind your PR, isn't it?
Yes, thats right.
> The way it's currently done is too simple (if a job cannot be run in parallel on the HV, it will put in
> So don't you think we're better of rewriting the sequencer to let more commands being executed in parallel to avoid this bottleneck on the AgentAttache? It would normally make the cancellation not needed in the way you implemented it since less jobs will be queued.
>
As you already said, with todays design it isn't possible. Rewriting is obviously better. But, thats a bigger job. In the current design, this was the only possible way to allow cloudstack to process queued up jobs.
> If we wish to be able to cancel a job, IMHO it should cancel the job down on the hypervisor too, thus clearing normally the resources involved as if the execution didn't go well.
>
I agree. But, thats a huge task given the number of hypervisors we support and their versions.
---
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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1832
@karuturi please squash your changes and fix the merge conflict. We've merged very large changes that were squash to a single commit, it becomes easier to then cherry-pick them or revert them.
---
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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1832
Nice feature, @karuturi can you add suitable marvin tests, squash your changes.
---
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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by cloudmonger <gi...@git.apache.org>.
Github user cloudmonger commented on the issue:
https://github.com/apache/cloudstack/pull/1832
### ACS CI BVT Run
**Sumarry:**
Build Number 199
Hypervisor xenserver
NetworkType Advanced
Passed=98
Failed=7
Skipped=7
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* test_snapshots.py
* test_01_snapshot_root_disk Failing since 8 runs
* test_network.py
* test_delete_account Failed
* test_deploy_vm_iso.py
* test_deploy_vm_from_iso Failing since 32 runs
* test_vm_life_cycle.py
* test_10_attachAndDetach_iso Failing since 33 runs
* test_routers_network_ops.py
* test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true Failed
* test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false Failed
* test_03_RVR_Network_check_router_state 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_router_dns.py
test_non_contigiousvlan.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_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 #1832: CLOUDSTACK-9652 Job framework - Cancelling as...
Posted by marcaurele <gi...@git.apache.org>.
Github user marcaurele commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1832#discussion_r92676861
--- Diff: engine/orchestration/src/com/cloud/agent/manager/AgentAttache.java ---
@@ -399,10 +414,22 @@ public void send(final Request req, final Listener listener) throws AgentUnavail
try {
for (int i = 0; i < 2; i++) {
Answer[] answers = null;
+ job = _agentMgr._asyncJobDao.findById(jobId);
+ if (job != null && job.getStatus() == JobInfo.Status.CANCELLED) {
+ throw new OperationCancelledException(req.getCommands(), _id, seq, wait, false);
+ }
try {
answers = sl.waitFor(wait);
+ job = _agentMgr._asyncJobDao.findById(jobId);
+ if (job != null && job.getStatus() == JobInfo.Status.CANCELLED) {
+ throw new OperationCancelledException(req.getCommands(), _id, seq, wait, false);
--- End diff --
Why do you want to throw an `OperationCancelledException` if the we have the job answer. It's better to let the normal response come back to the user.
---
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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by marcaurele <gi...@git.apache.org>.
Github user marcaurele commented on the issue:
https://github.com/apache/cloudstack/pull/1832
@karuturi Ok thanks for the clarifications, and it's the scenario I thought about too. That being said, I'm currently thinking of a new approach for the command sequencer because having implemented the live migration, the non-parallel commands isn't optimal at all when you have long running sequential commands on a hypervisor. And I tend to think that's the reason behind your PR, isn't it? The way it's currently done is too simple (if a job cannot be run in parallel on the HV, it will put in a queue any other coming job that needs to run on this same HV). IMO this sequencing should take into account what kind of job is coming and for which type of resources. For example a security group update, a VM start and a migration for different VMs should be able to run in parallel because they are unrelated. With today design, it isn't possible.
So don't you think we're better of rewriting the sequencer to let more commands being executed in parallel to avoid this bottleneck on the AgentAttache? It would normally make the cancellation not needed in the way you implemented it since less jobs will be queued.
If we wish to be able to cancel a job, IMHO it should cancel the job down on the hypervisor too, thus clearing normally the resources involved as if the execution didn't go well.
Otherwise, the way you implemented it. I would not let a job being cancel if it has been sent to the hypervisor to clearly return to the user that it wasn't cancelable anymore (you're too late! -> seq number isn't in `_requests` anymore so it has been sent to the HV). I'm putting more comment in the code.
What do you think?
---
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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the issue:
https://github.com/apache/cloudstack/pull/1832
@rhtyd I dont buy that argument. Just for the ease of revert or cherry-pick we shouldnt commit all the changes together. The revert or cherry-pick quickly becomes irrelevant as more code gets committed on top of it. I would like to commit them as is.
regarding the conflicts, will resolve them as soon as I get time. right now, busy with $dayjob.
---
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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by marcaurele <gi...@git.apache.org>.
Github user marcaurele commented on the issue:
https://github.com/apache/cloudstack/pull/1832
@karuturi It's a good feature but I don't see where the job gets actually cancelled. Reading the changes I can only see that the response of the job will become a cancelled operation, but the actual job does not get kill/cancel on the hypervisor for example. Did I miss something or is it not fully implemented yet?
---
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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the issue:
https://github.com/apache/cloudstack/pull/1832
@marcaurele out for the next three days. will get back
@rhtyd marvin test is in progress. I will take it up in a separate PR.
Regarding squashing, a humongous commit would be difficult. I already grouped them into individual logical units.
---
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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by cloudmonger <gi...@git.apache.org>.
Github user cloudmonger commented on the issue:
https://github.com/apache/cloudstack/pull/1832
### ACS CI BVT Run
**Sumarry:**
Build Number 445
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=1
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
**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_routers_network_ops.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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by cloudmonger <gi...@git.apache.org>.
Github user cloudmonger commented on the issue:
https://github.com/apache/cloudstack/pull/1832
### ACS CI BVT Run
**Sumarry:**
Build Number 431
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=1
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
**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_routers_network_ops.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 #1832: CLOUDSTACK-9652 Job framework - Cancelling async job...
Posted by marcaurele <gi...@git.apache.org>.
Github user marcaurele commented on the issue:
https://github.com/apache/cloudstack/pull/1832
@karuturi Ok
---
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.
---