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

[Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>]

On 28.04.23 10:30, Juan Quintela wrote:
> 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.

OK, let's start with your idea, interface may be removed later if we want.

> 
> 
>> 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.

Oops right. Good catch

> 
> 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?


OK

> 
>> +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.

Sounds good. I can make a patch and include it to v4

> 
>> +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__);
>          }

I think, it could be abort()

>          migrate_start_colo_process(s);
> 
> The changes of functions interfaces are independent of this patch.
> 

Still, making empty stubs is clearer than stubs with abort(). I'll try and if changing the interface is not too much, will include it in v4.

-- 
Best regards,
Vladimir