Re: [PATCH v3 3/4] build: move COLO under CONFIG_REPLICATION

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> We don't allow to use x-colo capability when replication is not
> configured. So, no reason to build COLO when replication is disabled,
> it's unusable in this case.
>
> Note also that the check in migrate_caps_check() is not the only
> restriction: some functions in migration/colo.c will just abort if
> called with not defined CONFIG_REPLICATION, for example:
>
>     migration_iteration_finish()
>        case MIGRATION_STATUS_COLO:
>            migrate_start_colo_process()
>                colo_process_checkpoint()
>                    abort()
>
> It could probably make sense to have possibility to enable COLO without
> REPLICATION, but this requires deeper audit of colo & replication code,
> which may be done later if needed.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Nice patch.  Thanks.

> @@ -68,7 +66,6 @@ static bool colo_runstate_is_stopped(void)
>  static void secondary_vm_do_failover(void)
>  {
>  /* COLO needs enable block-replication */
> -#ifdef CONFIG_REPLICATION
>      int old_state;
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      Error *local_err = NULL;
> @@ -133,14 +130,10 @@ static void secondary_vm_do_failover(void)
>      if (mis->migration_incoming_co) {
>          qemu_coroutine_enter(mis->migration_incoming_co);
>      }
> -#else
> -    abort();
> -#endif
>  }

With only this chunks you have proved that your argument is right.
abort() is never a solution.

> diff --git a/migration/options.c b/migration/options.c
> index 912cbadddb..eef2bd0f16 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -171,7 +171,9 @@ Property migration_properties[] = {
>      DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
>      DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
>                          MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
> +#ifdef CONFIG_REPLICATION
>      DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
> +#endif
>      DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
>      DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
>      DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
> @@ -209,9 +211,13 @@ bool migrate_block(void)

>  bool migrate_colo(void)
>  {
> +#ifdef CONFIG_REPLICATION
>      MigrationState *s = migrate_get_current();
>  
>      return s->capabilities[MIGRATION_CAPABILITY_X_COLO];
> +#else
> +    return false;
> +#endif
>  }

#ifdef 1

>  
>  bool migrate_compress(void)
> @@ -401,7 +407,9 @@ 
> INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
>      MIGRATION_CAPABILITY_RDMA_PIN_ALL,
>      MIGRATION_CAPABILITY_COMPRESS,
>      MIGRATION_CAPABILITY_XBZRLE,
> +#ifdef CONFIG_REPLICATION
>      MIGRATION_CAPABILITY_X_COLO,
> +#endif
>      MIGRATION_CAPABILITY_VALIDATE_UUID,
>      MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>  

#ifdef 2

> @@ -428,15 +436,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, 
> Error **errp)
>      }
>  #endif
>  
> -#ifndef CONFIG_REPLICATION
> -    if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
> -        error_setg(errp, "QEMU compiled without replication module"
> -                   " can't enable COLO");
> -        error_append_hint(errp, "Please enable replication before COLO.\n");
> -        return false;
> -    }
> -#endif
> -
>      if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
>          /* This check is reasonably expensive, so only when it's being
>           * set the first time, also it's only the destination that needs

I would preffer if you removed #ifdef 1 and 2 and let this one in.  I am
trying to get all capabilities to this format.


> diff --git a/stubs/colo.c b/stubs/colo.c
> new file mode 100644
> index 0000000000..f306ab45d6
> --- /dev/null
> +++ b/stubs/colo.c
> @@ -0,0 +1,37 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +#include "migration/colo.h"
> +#include "migration/migration.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
> +
> +void colo_shutdown(void)
> +{
> +    abort();
> +}

This is wrong, it should be empty.

void migration_shutdown(void)
{
    /*
     * When the QEMU main thread exit, the COLO thread
     * may wait a semaphore. So, we should wakeup the
     * COLO thread before migration shutdown.
     */
    colo_shutdown();

    ......

}



> +void *colo_process_incoming_thread(void *opaque)
> +{
> +    abort();
> +}

At least print an error message?

> +void colo_checkpoint_notify(void *opaque)
> +{
> +    abort();
> +}

Another error message.

It is independently of this patch, but I am thinking about changing the
interface and doing something like this in options.c

changing

    if (params->has_x_checkpoint_delay) {
        s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
        if (migration_in_colo_state()) {
            colo_checkpoint_notify(s);
        }
    }

To

    if (params->has_x_checkpoint_delay) {
        s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
        colo_checkpoint_refresh(s);
    }

That way we can convert it to an empty function.

> +void migrate_start_colo_process(MigrationState *s)
> +{
> +    abort();
> +}

Another case of changing the function interface?

    case MIGRATION_STATUS_COLO:
        if (!migrate_colo()) {
            error_report("%s: critical error: calling COLO code without "
                         "COLO enabled", __func__);
        }
        migrate_start_colo_process(s);

The changes of functions interfaces are independent of this patch.

Later, Juan.