[virt-tools-list] [RFC Patch v2] Support job cancellation for migration, save functions

[Wen Congyang]

At 2010-12-02 05:21, Cole Robinson Write:
> On 11/30/2010 10:35 PM, Wen Congyang wrote:
>> # HG changeset patch
>> # User Wen Congyang <wency at cn.fujitsu.com>
>> # Date 1291173826 -28800
>> # Node ID da5771e13f5a395c3fff8bcbdf2769d308034744
>> # Parent  dedbf8d0d5a34399a2baa8422b0f154a1c243bb0
>> Support job cancellation for migration, save functions
>>
>> I do not use the API libvirt.jobinfo() to check whether the job is cancelled,
>> because the jobinfo is cleared when job ends...
>>
> 
> Thanks for the patch. A couple suggestions. Again I added a virtinst
> support check for jobInfo(), we can roughly use this to guess if
> libvirt/the hypervisor supports cancel jobs as well:
> 
> http://hg.fedorahosted.org/hg/python-virtinst/rev/871813b2dda1
> 
> So I would recommend actually hiding the cancel button if the HV doesn't
> support jobInfo, otherwise we will eventually get bug reports about the
> cancel button not being clickable for other actions.

Hmm, it is a good idea to hide the cancel button when we do not implement
the cancel action or the HV doesn't support the implement.

The API jobInfo can not provide any info when job ends. It can used only when
the job is running. I think we would hide the cancel button only when
the HV doesn't support abortJob.

> 
> You should be able to use the test driver (virt-manager --connect
> test:///default) to simulate a connection that doesn't support jobInfo,
> to make sure that everything is working correctly.

OK. I will test it before sending v3 patch.

> 
> Also, any plans to implement save/migrate progress reporting? If not, no
> worries.

No plans now.

<snip>
>> +        self.window.signal_autoconnect({
>> +            "on_async_job_delete_event" : self.cancel,
>> +            "on_async_job_cancel_clicked" : self.cancel,
>> +        })
> 
> Hmm, not sure if we want to overwrite the window delete event.
> Previously that wouldn't cancel a migration, so we shouldn't change this
> behavior silently. Maybe launch a confirm dialog or something?

Hmm, It is better to launch a confirm dialog than to overwrite the delete event.

<snip>
>> +        job_info = {"is_cancelled" : False}
> 
> Hmm, rather than use this dictionary hack, I'd just track it in the
> Async class with a job_canceled variable

Sounds good.

>> -    def _save_callback(self, vm, file_to_save, asyncjob):
>> +    def _save_cancel(self, vm, job_info):
>> +        vm.abort_job()
>> +        job_info["is_cancelled"] = True
>> +        return
>> +
> 
> What if this throws an error, will the user ever see it? Maybe if we
> could find a way to update the progress dialog with a little warning
> that cancellation failed.
I will add some codes to deal with the the error in v3 patch.
> 
> Thanks,
> Cole
> 

Thank you for so many comments. I will modify this patch.