Re: [libvirt] [PATCH v12] support offline migration
On Mon, Nov 05, 2012 at 11:33:48 +0800, li guang wrote:
在 2012-11-02五的 15:32 +0100，Jiri Denemark写道：
...
> > @@ -1607,6 +1630,15 @@ qemuMigrationPrepareAny(struct
qemud_driver *driver,
> > goto endjob;
> > }
> >
> > + if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
> > + QEMU_MIGRATION_COOKIE_OFFLINE)))
> > + return ret;
> > +
>
> Oops, the return statement will leave a big mess uncleaned. You need to goto
> endjob instead.
You missed this comment when doing v13.
> > + if (mig->flags & QEMU_MIGRATION_COOKIE_OFFLINE)
{
> > + ret = 0;
> > + goto cleanup;
> > + }
> > +
...
> > @@ -2150,6 +2182,9 @@ qemuMigrationRun(struct qemud_driver
*driver,
> > return -1;
> > }
> >
> > + if (flags & VIR_MIGRATE_OFFLINE)
> > + return 0;
> > +
> > if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
> > QEMU_MIGRATION_COOKIE_GRAPHICS)))
> > goto cleanup;
>
> This check seems to be too deep. The whole point of it is to skip transferring
No, otherwise we would do much more work to clean up offline path.
I don't think I understand your point. What would we need to clean up if we
just skipped Perform phase completely? Especially when that's what you're
doing in peer to peer migration.
> > @@ -2665,7 +2700,12 @@ static int
doPeer2PeerMigrate3(struct qemud_driver *driver,
...
> > @@ -2771,7 +2811,7 @@ finish:
> > vm->def->name);
> >
> > cleanup:
> > - if (ddomain) {
> > + if (ddomain || (flags & VIR_MIGRATE_OFFLINE)) {
> > virObjectUnref(ddomain);
> > ret = 0;
> > } else {
>
> This is weird. It means that we don't care if Finish succeeded or not during
> offline migration even though Finish is where the domain configuration gets
> stored on disk.
my purpose is to call virObjectUnref(ddomain),
"ret = 0" is no only for offline migration.
But in case of offline migration the code could call virObjectUnref(NULL) and
set ret = 0 if ddomain is NULL, which actually means that something failed
(non-persistent offline migration is forbidden and thus we are guaranteed that
we will always get non-NULL ddomain for offline migration unless something
failed). In other words, keeping this code as-is should work in the desired
way.
...
> We're almost there, just some unusual code paths (other than
successful
> non-p2p migration) need fixing. Moreover, I think we should explicitly fail v2
> migration with OFFLINE flag to avoid having to touch that path as well. And
really? I think we will not fall into V2 migration path anymore, for we
always check V3 first, and will return true(qemu driver).
In case new libvirt client or libvirtd (during p2p migration) talks to an old
enough libvirtd, we may still end up doing v2 migration. I guess it would fail
somewhere down the road because of unknown flag but I think it's better to
just refuse to start v2 migration if OFFLINE flag is used.
Jirka