Re: [Qemu-devel] [PATCH RFC v3 24/27] COLO NIC: Implement NIC checkpoint and failover

[zhanghailiang]

On 2015/3/6 1:12, Dr. David Alan Gilbert wrote:
> * zhanghailiang (address@hidden) wrote:
> > Signed-off-by: zhanghailiang <address@hidden>
> > Signed-off-by: Gao feng <address@hidden>
> > ---
> >   include/net/colo-nic.h |  3 ++-
> >   migration/colo.c       | 22 ++++++++++++++++++----
> >   net/colo-nic.c         | 19 +++++++++++++++++++
> >   3 files changed, 39 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/colo-nic.h b/include/net/colo-nic.h
> > index 67c9807..ddc21cd 100644
> > --- a/include/net/colo-nic.h
> > +++ b/include/net/colo-nic.h
> > @@ -20,5 +20,6 @@ void colo_add_nic_devices(NetClientState *nc);
> >   void colo_remove_nic_devices(NetClientState *nc);
> >
> >   int colo_proxy_compare(void);
> > -
> > +int colo_proxy_failover(void);
> > +int colo_proxy_checkpoint(void);
> >   #endif
> > diff --git a/migration/colo.c b/migration/colo.c
> > index 579aabf..874971c 100644
> > --- a/migration/colo.c
> > +++ b/migration/colo.c
> > @@ -94,6 +94,11 @@ static void slave_do_failover(void)
> >           ;
> >       }
> >
> > +    if (colo_proxy_failover() != 0) {
> > +        error_report("colo proxy failed to do failover");
> > +    }
> > +    colo_proxy_destroy(COLO_SECONDARY_MODE);

Hi, Dave

> I'm not sure if this is the best thing to do on a secondary failover.
> If I understand correctly, when it's running, we have:
>
>
> -------+
>         |                    br0---eth0
>         |
>   slave +-tun - xt_SECCOLO - br1---eth1
>         |
> -------+
>
> what I think that colo-proxy-destroy  is doing is rewiring that as:
>
>
> -------+
>         |     +--------------br0---eth0
>         |     |
>   slave +-tun +              br1---eth1
>         |
> -------+

Yes, you got it.

> but now we've lost the sequence number adjustment data that
> was held in xt_SECCOLO and so you are likely to break existing TCP
> connections.

In our test, we didn't come across the 'break existing TCP connections' 
situation,
We only adjust the sequence number at the beginning of building connection, 
after
the connection is build, this data in xt_SECCOLO is useless ...

> Also, I don't think colo-proxy-script is passed a flag to let it
> know whether the reason it's doing a slave_uninstall is due to
> a failover or a simple shutdown; and so it assumes it has
> to do the rewire for a failover.
> (Actually the script in the qemu repo is newer than the script in
> the colo-proxy repo, that one doesn't have the rewire at all).

You are right, we should distinguish between shutdown and failover for the 
slave_uninstall,
Actually, using script to do the corresponding work maybe not so appropriate,
we are trying to fix the net-related part.

Thanks,
zhanghailiang
> Dave
>
> > +
> >       colo = NULL;
> >
> >       if (!autostart) {
> > @@ -115,7 +120,7 @@ static void master_do_failover(void)
> >       if (!colo_runstate_is_stopped()) {
> >           vm_stop_force_state(RUN_STATE_COLO);
> >       }
> > -
> > +    colo_proxy_destroy(COLO_PRIMARY_MODE);
> >       if (s->state != MIG_STATE_ERROR) {
> >           migrate_set_state(s, MIG_STATE_COLO, MIG_STATE_COMPLETED);
> >       }
> > @@ -245,6 +250,11 @@ static int do_colo_transaction(MigrationState *s, QEMUFile 
> > *control)
> >
> >       qemu_fflush(trans);
> >
> > +    ret = colo_proxy_checkpoint();
> > +    if (ret < 0) {
> > +        goto out;
> > +    }
> > +
> >       ret = colo_ctl_put(s->file, COLO_CHECKPOINT_SEND);
> >       if (ret < 0) {
> >           goto out;
> > @@ -387,8 +397,6 @@ out:
> >       qemu_bh_schedule(s->cleanup_bh);
> >       qemu_mutex_unlock_iothread();
> >
> > -    colo_proxy_destroy(COLO_PRIMARY_MODE);
> > -
> >       return NULL;
> >   }
> >
> > @@ -508,6 +516,12 @@ void *colo_process_incoming_checkpoints(void *opaque)
> >               goto out;
> >           }
> >
> > +        ret = colo_proxy_checkpoint();
> > +        if (ret < 0) {
> > +                goto out;
> > +        }
> > +        DPRINTF("proxy begin to do checkpoint\n");
> > +
> >           ret = colo_ctl_get(f, COLO_CHECKPOINT_SEND);
> >           if (ret < 0) {
> >               goto out;
> > @@ -584,6 +598,7 @@ out:
> >           * just kill slave
> >           */
> >           error_report("SVM is going to exit!");
> > +        colo_proxy_destroy(COLO_SECONDARY_MODE);
> >           exit(1);
> >       } else {
> >           /* if we went here, means master may dead, we are doing failover */
> > @@ -610,6 +625,5 @@ out:
> >
> >       loadvm_exit_colo();
> >
> > -    colo_proxy_destroy(COLO_SECONDARY_MODE);
> >       return NULL;
> >   }
> > diff --git a/net/colo-nic.c b/net/colo-nic.c
> > index 563d661..02a454d 100644
> > --- a/net/colo-nic.c
> > +++ b/net/colo-nic.c
> > @@ -379,6 +379,25 @@ void colo_proxy_destroy(int side)
> >       cp_info.index = -1;
> >       colo_nic_side = -1;
> >   }
> > +
> > +int colo_proxy_failover(void)
> > +{
> > +    if (colo_proxy_send(NULL, 0, COLO_FAILOVER) < 0) {
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +int colo_proxy_checkpoint(void)
> > +{
> > +    if (colo_proxy_send(NULL, 0, COLO_CHECKPOINT) < 0) {
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   /*
> >   do checkpoint: return 1
> >   error: return -1
> > --
> > 1.7.12.4
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>
> .