On Mon, Sep 5, 2016 at 8:06 PM, Daniel P. Berrange <berra...@redhat.com> wrote:
> On Mon, Sep 05, 2016 at 07:56:26PM +0530, Ashijeet Acharya wrote:
>> Include migrate_set_speed and migrate_set_downtime inside
>> migrate_set_parameters for setting maximum migration speed and expected
>> downtime parameters respectively. Also update the query part for both in qmp
>> and hmp qemu control interfaces.
>
> Please put line breaks in your commit message at 72 characters
>
>
> Also, when sending v2, v3, etc it is preferred to send it as a new
> top-level thread. ie, don't set headers to be a reply to your v1.
Alright i will keep that in mind from now onwards.
>
>> Signed-off-by: Ashijeet Acharya <ashijeetacha...@gmail.com>
>> ---
>> hmp.c | 33 +++++++++++++++++++++++++++++++++
>> include/migration/migration.h | 1 -
>> migration/migration.c | 30 ++++++++++++++++++++++++++----
>> qapi-schema.json | 26 +++++++++++++++++++++++---
>> qmp-commands.hx | 13 ++++++++++---
>> 5 files changed, 92 insertions(+), 11 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index cc2056e..c92769b 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -304,6 +304,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const
>> QDict *qdict)
>> monitor_printf(mon, " %s: '%s'",
>> MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
>> params->tls_hostname ? : "");
>> + monitor_printf(mon, " %s: %" PRId64,
>> + MigrationParameter_lookup[MIGRATION_PARAMETER_MAX_BANDWIDTH],
>> + params->max_bandwidth);
>> + monitor_printf(mon, " %s: %" PRId64,
>> + MigrationParameter_lookup[MIGRATION_PARAMETER_DOWNTIME_LIMIT],
>> + params->downtime_limit);
>> monitor_printf(mon, "\n");
>> }
>>
>> @@ -1193,6 +1199,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict
>> *qdict)
>> hmp_handle_error(mon, &err);
>> }
>>
>> +/* Kept for old-commands compatibility */
>
> I don't think this comment really adds any value. Instead, in the qapi
> schema, you should mark the legacy methods as deprecated though.
Hmm, that would make more sense.
>
>> void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
>> {
>> double value = qdict_get_double(qdict, "value");
>> @@ -1211,6 +1218,7 @@ void hmp_migrate_set_cache_size(Monitor *mon, const
>> QDict *qdict)
>> }
>> }
>>
>> +/* Kept for old-commands compatibility */
>> void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
>> {
>> int64_t value = qdict_get_int(qdict, "value");
>> @@ -1251,7 +1259,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const
>> QDict *qdict)
>> {
>> const char *param = qdict_get_str(qdict, "parameter");
>> const char *valuestr = qdict_get_str(qdict, "value");
>> + int64_t valuebw = 0;
>> + double valuedowntime = 0;
>> long valueint = 0;
>> + char *endp;
>> Error *err = NULL;
>> bool has_compress_level = false;
>> bool has_compress_threads = false;
>> @@ -1260,6 +1271,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const
>> QDict *qdict)
>> bool has_cpu_throttle_increment = false;
>> bool has_tls_creds = false;
>> bool has_tls_hostname = false;
>> + bool has_max_bandwidth = false;
>> + bool has_downtime_limit = false;
>> bool use_int_value = false;
>> int i;
>>
>> @@ -1291,6 +1304,24 @@ void hmp_migrate_set_parameter(Monitor *mon, const
>> QDict *qdict)
>> case MIGRATION_PARAMETER_TLS_HOSTNAME:
>> has_tls_hostname = true;
>> break;
>> + case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>> + has_max_bandwidth = true;
>> + valuebw = qemu_strtosz(valuestr, &endp);
>> + if (valuebw < 0 || (size_t)valuebw != valuebw || *endp !=
>> '\0'
>> + || !is_power_of_2(valuebw)) {
>> + error_setg(&err, "Invalid size %s", valuestr);
>> + goto cleanup;
>> + }
>> + break;
>> + case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
>> + has_downtime_limit = true;
>> + valuedowntime = strtod(valuestr, &endp);
>> + if (valuestr == endp) {
>> + error_setg(&err, "Unable to parse '%s' as a number",
>> + valuestr);
>> + goto cleanup;
>> + }
>> + break;
>> }
>>
>> if (use_int_value) {
>> @@ -1308,6 +1339,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const
>> QDict *qdict)
>> has_cpu_throttle_increment, valueint,
>> has_tls_creds, valuestr,
>> has_tls_hostname, valuestr,
>> + has_max_bandwidth, valuebw,
>> + has_downtime_limit, valuedowntime,
>> &err);
>> break;
>> }
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index 3c96623..a5429ee 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -129,7 +129,6 @@ struct MigrationSrcPageRequest {
>>
>> struct MigrationState
>> {
>> - int64_t bandwidth_limit;
>> size_t bytes_xfer;
>> size_t xfer_limit;
>> QemuThread thread;
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 955d5ee..4b54b58 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -44,6 +44,9 @@
>> #define BUFFER_DELAY 100
>> #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
>>
>> +/* Amount of nanoseconds we are willing to wait for migration to be down. */
>> +#define DEFAULT_MIGRATE_SET_DOWNTIME 300000000
>> +
>> /* Default compression thread count */
>> #define DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT 8
>> /* Default decompression thread count, usually decompression is at
>> @@ -80,7 +83,6 @@ MigrationState *migrate_get_current(void)
>> static bool once;
>> static MigrationState current_migration = {
>> .state = MIGRATION_STATUS_NONE,
>> - .bandwidth_limit = MAX_THROTTLE,
>> .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
>> .mbps = -1,
>> .parameters = {
>> @@ -89,6 +91,8 @@ MigrationState *migrate_get_current(void)
>> .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
>> .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
>> .cpu_throttle_increment =
>> DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
>> + .max_bandwidth = MAX_THROTTLE,
>> + .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
>> },
>> };
>>
>> @@ -566,6 +570,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error
>> **errp)
>> params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
>> params->tls_creds = g_strdup(s->parameters.tls_creds);
>> params->tls_hostname = g_strdup(s->parameters.tls_hostname);
>> + params->max_bandwidth = s->parameters.max_bandwidth;
>> + params->downtime_limit = s->parameters.downtime_limit;
>>
>> return params;
>> }
>> @@ -773,6 +779,10 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>> const char *tls_creds,
>> bool has_tls_hostname,
>> const char *tls_hostname,
>> + bool has_max_bandwidth,
>> + int64_t max_bandwidth,
>> + bool has_downtime_limit,
>> + double downtime_limit,
>> Error **errp)
>> {
>> MigrationState *s = migrate_get_current();
>> @@ -832,6 +842,12 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>> g_free(s->parameters.tls_hostname);
>> s->parameters.tls_hostname = g_strdup(tls_hostname);
>> }
>> + if (has_max_bandwidth) {
>> + qmp_migrate_set_speed(max_bandwidth, NULL);
>> + }
>> + if (has_downtime_limit) {
>> + qmp_migrate_set_downtime(downtime_limit, NULL);
>> + }
>> }
>>
>>
>> @@ -1175,18 +1191,24 @@ void qmp_migrate_set_speed(int64_t value, Error
>> **errp)
>> }
>>
>> s = migrate_get_current();
>> - s->bandwidth_limit = value;
>> + s->parameters.max_bandwidth = value;
>> if (s->to_dst_file) {
>> qemu_file_set_rate_limit(s->to_dst_file,
>> - s->bandwidth_limit / XFER_LIMIT_RATIO);
>> + s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>> }
>> }
>>
>> void qmp_migrate_set_downtime(double value, Error **errp)
>> {
>> + MigrationState *s;
>> +
>> value *= 1e9;
>> value = MAX(0, MIN(UINT64_MAX, value));
>> +
>> + s = migrate_get_current();
>> +
>> max_downtime = (uint64_t)value;
>> + s->parameters.downtime_limit = max_downtime;
>> }
>>
>> bool migrate_postcopy_ram(void)
>> @@ -1858,7 +1880,7 @@ void migrate_fd_connect(MigrationState *s)
>>
>> qemu_file_set_blocking(s->to_dst_file, true);
>> qemu_file_set_rate_limit(s->to_dst_file,
>> - s->bandwidth_limit / XFER_LIMIT_RATIO);
>> + s->parameters.max_bandwidth /
>> XFER_LIMIT_RATIO);
>>
>> /* Notify before starting migration thread */
>> notifier_list_notify(&migration_state_notifiers, s);
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 5658723..250eac5 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -637,12 +637,18 @@
>> # hostname must be provided so that the server's x509
>> # certificate identity can be validated. (Since 2.7)
>> #
>> +# @max-bandwidth: to set maximum speed for migration. A value lesser than
>> +# zero will be automatically round upto zero. Since 2.8)
>
> Document the units for this ? eg is it bits-per-second, kb-per-second,
> mb-per-second, etc
>
Should I document it the way it is for old-commands? Like this;
# @migrate_set_speed
#
# Set maximum speed for migration.
#
# @value: maximum speed in bytes.
#
# Returns: nothing on success
#
# Notes: A value lesser than zero will be automatically round up to zero.
#
# Since: 0.14.0
##
>> +#
>> +# @downtime-limit: set maximum tolerated downtime for migration. Since 2.8)
>
> Again, units ? nanoseconds ? milliseconds ?
Like this;
# @migrate_set_downtime
#
# Set maximum tolerated downtime for migration.
#
# @value: maximum downtime in seconds
#
# Returns: nothing on success
#
# Since: 0.14.0
>
>> +#
>> # Since: 2.4
>> ##
>> { 'enum': 'MigrationParameter',
>> 'data': ['compress-level', 'compress-threads', 'decompress-threads',
>> 'cpu-throttle-initial', 'cpu-throttle-increment',
>> - 'tls-creds', 'tls-hostname'] }
>> + 'tls-creds', 'tls-hostname', 'max-bandwidth',
>> + 'downtime-limit'] }
>>
>> #
>> # @migrate-set-parameters
>> @@ -678,6 +684,11 @@
>> # hostname must be provided so that the server's x509
>> # certificate identity can be validated. (Since 2.7)
>> #
>> +# @max-bandwidth: to set maximum speed for migration. A value lesser than
>> +# zero will be automatically round upto zero. Since 2.8)
>
> Units
>
>> +#
>> +# @downtime-limit: set maximum tolerated downtime for migration. Since 2.8)
>
> Units
>
>> +#
>> # Since: 2.4
>> ##
>> { 'command': 'migrate-set-parameters',
>> @@ -687,7 +698,9 @@
>> '*cpu-throttle-initial': 'int',
>> '*cpu-throttle-increment': 'int',
>> '*tls-creds': 'str',
>> - '*tls-hostname': 'str'} }
>> + '*tls-hostname': 'str',
>> + '*max-bandwidth': 'int',
>> + '*downtime-limit': 'number'} }
>>
>> #
>> # @MigrationParameters
>> @@ -721,6 +734,11 @@
>> # hostname must be provided so that the server's x509
>> # certificate identity can be validated. (Since 2.7)
>> #
>> +# @max-bandwidth: to set maximum speed for migration. A value lesser than
>> +# zero will be automatically round upto zero. (Since
>> 2.8)
>> +#
>> +# @downtime-limit: set maximum tolerated downtime for migration. Since 2.8)
>> +#
>> # Since: 2.4
>> ##
>> { 'struct': 'MigrationParameters',
>> @@ -730,7 +748,9 @@
>> 'cpu-throttle-initial': 'int',
>> 'cpu-throttle-increment': 'int',
>> 'tls-creds': 'str',
>> - 'tls-hostname': 'str'} }
>> + 'tls-hostname': 'str',
>> + 'max-bandwidth': 'int',
>> + 'downtime-limit': 'int'} }
>> ##
>> # @query-migrate-parameters
>> #
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 6866264..0418cab 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -3790,7 +3790,9 @@ Set migration parameters
>> throttled for auto-converge (json-int)
>> - "cpu-throttle-increment": set throttle increasing percentage for
>> auto-converge (json-int)
>> -
>> +- "max-bandwidth": set maximum speed for migrations (json-int)
>
> Document units
>
>> +- "downtime-limit": set maximum tolerated downtime (in seconds) for
>> + migrations (json-number)
>
> I'm sure units for this are not "seconds" as that is way too coarse.
I dont know, that is exactly what was documented for the old-command
migrate_set_downtime.
This is what i found;
migrate_set_downtime
--------------------
Set maximum tolerated downtime (in seconds) for migrations.
Arguments:
- "value": maximum downtime (json-number)
Example:
-> { "execute": "migrate_set_downtime", "arguments": { "value": 0.1 } }
<- { "return": {} }
>
>> Arguments:
>>
>> Example:
>> @@ -3803,7 +3805,7 @@ EQMP
>> {
>> .name = "migrate-set-parameters",
>> .args_type =
>> -
>> "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?",
>> +
>> "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,max-bandwidth:i?,downtime-limit:T?",
>> .mhandler.cmd_new = qmp_marshal_migrate_set_parameters,
>> },
>> SQMP
>> @@ -3820,6 +3822,9 @@ Query current migration parameters
>> throttled (json-int)
>> - "cpu-throttle-increment" : throttle increasing percentage for
>> auto-converge (json-int)
>> + - "max-bandwidth" : maximium migration speed (json-int)
>> + - "downtime-limit" : maximum tolerated downtime of migration
>> + (json-int)
>
> Document units
>
>
> Regards,
> Daniel
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
