[libvirt] [PATCH v3 2/2] auto cold migration fallback at timeout
Wen Congyang
wency at cn.fujitsu.com
Mon Dec 20 02:20:53 UTC 2010
At 12/18/2010 07:10 AM, Eric Blake Write:
> On 12/17/2010 01:49 AM, Wen Congyang wrote:
>> implement auto cold migration fallback at timeout
>
> Maybe it's a language barrier issue thing, but I had to read this patch
> several times before I understood what you were getting at. Does this
> wording work any better?
>
> If a guest has not completed live migration before timeout, then
> auto-suspend the guest, where the migration will complete offline.
Yes.
>
>>
>> Signed-off-by: Wen Congyang <wency at cn.fujitsu.com>
>>
>> ---
>> tools/virsh.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 66 insertions(+), 0 deletions(-)
>
> Missing a change to tools/virsh.pod to document the new option (that
> wording should be more complete, in comparison to the one-line blurb for
> the 'virsh help migrate' output).
I will add it.
>
>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index cbde085..b95c8fe 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -3379,9 +3379,32 @@ static const vshCmdOptDef opts_migrate[] = {
>> {"desturi", VSH_OT_DATA, VSH_OFLAG_REQ, N_("connection URI of the destination host")},
>> {"migrateuri", VSH_OT_DATA, 0, N_("migration URI, usually can be omitted")},
>> {"dname", VSH_OT_DATA, 0, N_("rename to new name during migration (if supported)")},
>> + {"timeout", VSH_OT_INT, 0, N_("auto cold migration fallback when live migration timeouts(in seconds)")},
>
> Maybe: "force guest to suspend if live migration exceeds timeout (in
> seconds)"
OK
>
>> {NULL, 0, 0, NULL}
>> };
>>
>> +static void migrateTimeoutHandler(void *data)
>> +{
>
> I'd float this helper function to live above the cmdMigrate definition;
> placing it here interrupts the flow between what options cmdMigrate
> takes and how it uses them.
>
>> + virDomainPtr dom = (virDomainPtr)data;
>
> Cast is not necessary.
>
>> cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>> {
>> @@ -3389,6 +3412,8 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>> const char *desturi;
>> const char *migrateuri;
>> const char *dname;
>> + long long timeout;
>> + virTimerPtr migratetimer = NULL;
>> int flags = 0, found, ret = FALSE;
>>
>> if (!vshConnectionUsability (ctl, ctl->conn))
>> @@ -3426,6 +3451,28 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>> if (vshCommandOptBool (cmd, "copy-storage-inc"))
>> flags |= VIR_MIGRATE_NON_SHARED_INC;
>>
>> + timeout = vshCommandOptLongLong(cmd, "timeout", &found);
>
> Why long long? No one is going to set a timeout bigger than 4 billion
> seconds, so plain int would do just fine here. In particular, since
> virEventAddTimeout can only sleep up to int milliseconds, and you are
> already multiplying the user's timeout value (in seconds) by 1000, you
> already have to worry about overflow issues - if the user requests 5
> million seconds (which overflows 4 billion milliseconds for the
> underlying timer max frequency), you must reject the user's value.
>
>> +
>> + migratetimer = virNewTimer(migrateTimeoutHandler, (void *)dom);
>> + if (!migratetimer)
>> + goto done;
>> +
>> + if (virStartTimer() < 0) {
>
> Hmm - given your usage of starting a timer thread as long as any
> virTimerPtr object exists, maybe you're better off creating some sort of
> global ref-counting state under the hood of timer.c that tracks how many
> virTimer objects are currently set to have a time. The user should only
> have to call a single function to create their timer object, without
> having to make extra calls to feed your underlying implementation.
>
>> @@ -3455,6 +3516,11 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>> }
>>
>> done:
>> + if (migratetimer) {
>> + virStopTimer();
>> + virDelTimer(migratetimer);
>
> Again, if multiple threads are managing timers, then the user shouldn't
> be able to kill the timer resources just because one thread no longer
> needs a timer. The user shouldn't have to call virStopTimer.
>
Thanks for your comment.
More information about the libvir-list
mailing list