| Message ID | 20190827120221.15725-2-yury-kotov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | UUID validation during migration | expand |
On 8/27/19 7:02 AM, Yury Kotov wrote: > This capability realizes simple source validation by UUID. > It's useful for live migration between hosts. > > Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru> > --- > migration/migration.c | 9 +++++++++ > migration/migration.h | 1 + > migration/savevm.c | 45 +++++++++++++++++++++++++++++++++++++++++++ > qapi/migration.json | 5 ++++- > 4 files changed, 59 insertions(+), 1 deletion(-) Any reason why this is marked experimental? It seems useful enough that we could probably just add it as a fully-supported feature (dropping the x- prefix) - but I'll leave that up to the migration maintainers. In fact, do we even need this to be a tunable feature? Why not just always enable it? As long as the UUID is sent in a way that new->old doesn't break the old qemu from receiving the migration stream, and that old->new copes with UUID being absent, then new->new will always benefit from the additional safety check. > +++ b/qapi/migration.json > @@ -415,6 +415,9 @@ > # > # @x-ignore-shared: If enabled, QEMU will not migrate shared memory (since 4.0) > # > +# @x-validate-uuid: Check whether the UUID is the same on both sides or not. > +# (since 4.2) Maybe: @x-validate-uuid: Send the UUID of the source to allow the destination to ensure it is the same. if we even need a tunable capability. > +# > # Since: 1.2 > ## > { 'enum': 'MigrationCapability', > @@ -422,7 +425,7 @@ > 'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram', > 'block', 'return-path', 'pause-before-switchover', 'multifd', > 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', > - 'x-ignore-shared' ] } > + 'x-ignore-shared', 'x-validate-uuid' ] } > > ## > # @MigrationCapabilityStatus: >
27.08.2019, 17:02, "Eric Blake" <eblake@redhat.com>: > On 8/27/19 7:02 AM, Yury Kotov wrote: >> This capability realizes simple source validation by UUID. >> It's useful for live migration between hosts. >> >> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru> >> --- >> migration/migration.c | 9 +++++++++ >> migration/migration.h | 1 + >> migration/savevm.c | 45 +++++++++++++++++++++++++++++++++++++++++++ >> qapi/migration.json | 5 ++++- >> 4 files changed, 59 insertions(+), 1 deletion(-) > > Any reason why this is marked experimental? It seems useful enough that > we could probably just add it as a fully-supported feature (dropping the > x- prefix) - but I'll leave that up to the migration maintainers. > I thought that all new capabilities have x- prefix... May be it's really unnecessary here, I'm not sure. > In fact, do we even need this to be a tunable feature? Why not just > always enable it? As long as the UUID is sent in a way that new->old > doesn't break the old qemu from receiving the migration stream, and that > old->new copes with UUID being absent, then new->new will always benefit > from the additional safety check. > In such case we couldn't migrate from e.g. 4.2 to 3.1 If it's not a problem then we can always use it. >> +++ b/qapi/migration.json >> @@ -415,6 +415,9 @@ >> # >> # @x-ignore-shared: If enabled, QEMU will not migrate shared memory (since 4.0) >> # >> +# @x-validate-uuid: Check whether the UUID is the same on both sides or not. >> +# (since 4.2) > > Maybe: > > @x-validate-uuid: Send the UUID of the source to allow the destination > to ensure it is the same. > > if we even need a tunable capability. > Yes, it sounds better. Thanks! >> +# >> # Since: 1.2 >> ## >> { 'enum': 'MigrationCapability', >> @@ -422,7 +425,7 @@ >> 'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram', >> 'block', 'return-path', 'pause-before-switchover', 'multifd', >> 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', >> - 'x-ignore-shared' ] } >> + 'x-ignore-shared', 'x-validate-uuid' ] } >> >> ## >> # @MigrationCapabilityStatus: > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org Regards, Yury
On 8/27/19 10:36 AM, Yury Kotov wrote: > 27.08.2019, 17:02, "Eric Blake" <eblake@redhat.com>: >> On 8/27/19 7:02 AM, Yury Kotov wrote: >>> This capability realizes simple source validation by UUID. >>> It's useful for live migration between hosts. >>> >> >> Any reason why this is marked experimental? It seems useful enough that >> we could probably just add it as a fully-supported feature (dropping the >> x- prefix) - but I'll leave that up to the migration maintainers. >> > > I thought that all new capabilities have x- prefix... May be it's really > unnecessary here, I'm not sure. New features that need some testing or possible changes to behavior need x- to mark them as experimental, so we can make those changes without worrying about breaking back-compat. But new features that are outright useful and presumably in their final form, with no further experimentation needed, can skip going through an x- phase. > >> In fact, do we even need this to be a tunable feature? Why not just >> always enable it? As long as the UUID is sent in a way that new->old >> doesn't break the old qemu from receiving the migration stream, and that >> old->new copes with UUID being absent, then new->new will always benefit >> from the additional safety check. >> > > In such case we couldn't migrate from e.g. 4.2 to 3.1 I don't know the migration format enough to know if there is a way for 4.2 to unconditionally send a UUID as a subsection such that a receiving 3.1 will ignore the unknown subsection. If so, then you don't need a knob; if not, then you need something to say whether sending the subsection is safe (perhaps default on in new machine types, but default off for machine types that might still be migrated back to 3.1). That's where I'm hoping the migration experts will chime in.
* Eric Blake (eblake@redhat.com) wrote: > On 8/27/19 10:36 AM, Yury Kotov wrote: > > 27.08.2019, 17:02, "Eric Blake" <eblake@redhat.com>: > >> On 8/27/19 7:02 AM, Yury Kotov wrote: > >>> This capability realizes simple source validation by UUID. > >>> It's useful for live migration between hosts. > >>> > > >> > >> Any reason why this is marked experimental? It seems useful enough that > >> we could probably just add it as a fully-supported feature (dropping the > >> x- prefix) - but I'll leave that up to the migration maintainers. > >> > > > > I thought that all new capabilities have x- prefix... May be it's really > > unnecessary here, I'm not sure. > > New features that need some testing or possible changes to behavior need > x- to mark them as experimental, so we can make those changes without > worrying about breaking back-compat. But new features that are outright > useful and presumably in their final form, with no further > experimentation needed, can skip going through an x- phase. > > > > >> In fact, do we even need this to be a tunable feature? Why not just > >> always enable it? As long as the UUID is sent in a way that new->old > >> doesn't break the old qemu from receiving the migration stream, and that > >> old->new copes with UUID being absent, then new->new will always benefit > >> from the additional safety check. > >> > > > > In such case we couldn't migrate from e.g. 4.2 to 3.1 > > I don't know the migration format enough to know if there is a way for > 4.2 to unconditionally send a UUID as a subsection such that a receiving > 3.1 will ignore the unknown subsection. If so, then you don't need a > knob; if not, then you need something to say whether sending the > subsection is safe (perhaps default on in new machine types, but default > off for machine types that might still be migrated back to 3.1). That's > where I'm hoping the migration experts will chime in. Right; the migration format can't ignore chunks of data; so it does need to know somehow; the choice is either a capability or wiring it to the machine type; my preference is to wire it to the machine type; the arguments are: a) Machine type Pro: libvirt doesn't need to do anything Con: It doesn't protect old machine types It's not really part of the guest state b) Capability Pro: Works on all machine types Con: Libvirt needs to enable it So, no strong preference but I think I prefer (a). Dave > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Yury Kotov (yury-kotov@yandex-team.ru) wrote: > This capability realizes simple source validation by UUID. > It's useful for live migration between hosts. > > Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru> So, ignoring the question of whether it should be a capaibility or not, I'm actually OK with this; I would remove the x- - it's simple enough not to need to go through experimental I think. Dave > --- > migration/migration.c | 9 +++++++++ > migration/migration.h | 1 + > migration/savevm.c | 45 +++++++++++++++++++++++++++++++++++++++++++ > qapi/migration.json | 5 ++++- > 4 files changed, 59 insertions(+), 1 deletion(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 8b9f2fe30a..910e11b7d7 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2140,6 +2140,15 @@ bool migrate_ignore_shared(void) > return s->enabled_capabilities[MIGRATION_CAPABILITY_X_IGNORE_SHARED]; > } > > +bool migrate_validate_uuid(void) > +{ > + MigrationState *s; > + > + s = migrate_get_current(); > + > + return s->enabled_capabilities[MIGRATION_CAPABILITY_X_VALIDATE_UUID]; > +} > + > bool migrate_use_events(void) > { > MigrationState *s; > diff --git a/migration/migration.h b/migration/migration.h > index 3e1ea2b5dc..4f2fe193dc 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -290,6 +290,7 @@ bool migrate_postcopy_ram(void); > bool migrate_zero_blocks(void); > bool migrate_dirty_bitmaps(void); > bool migrate_ignore_shared(void); > +bool migrate_validate_uuid(void); > > bool migrate_auto_converge(void); > bool migrate_use_multifd(void); > diff --git a/migration/savevm.c b/migration/savevm.c > index 4a86128ac4..493dc24fd2 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -256,6 +256,7 @@ typedef struct SaveState { > uint32_t target_page_bits; > uint32_t caps_count; > MigrationCapability *capabilities; > + QemuUUID uuid; > } SaveState; > > static SaveState savevm_state = { > @@ -307,6 +308,7 @@ static int configuration_pre_save(void *opaque) > state->capabilities[j++] = i; > } > } > + state->uuid = qemu_uuid; > > return 0; > } > @@ -464,6 +466,48 @@ static const VMStateDescription vmstate_capabilites = { > } > }; > > +static bool vmstate_uuid_needed(void *opaque) > +{ > + return qemu_uuid_set && migrate_validate_uuid(); > +} > + > +static int vmstate_uuid_post_load(void *opaque, int version_id) > +{ > + SaveState *state = opaque; > + char uuid_src[UUID_FMT_LEN + 1]; > + char uuid_dst[UUID_FMT_LEN + 1]; > + > + if (!qemu_uuid_set) { > + /* > + * It's warning because user might not know UUID in some cases, > + * e.g. load an old snapshot > + */ > + qemu_uuid_unparse(&state->uuid, uuid_src); > + warn_report("UUID is received %s, but local uuid isn't set", > + uuid_src); > + return 0; > + } > + if (!qemu_uuid_is_equal(&state->uuid, &qemu_uuid)) { > + qemu_uuid_unparse(&state->uuid, uuid_src); > + qemu_uuid_unparse(&qemu_uuid, uuid_dst); > + error_report("UUID received is %s and local is %s", uuid_src, uuid_dst); > + return -EINVAL; > + } > + return 0; > +} > + > +static const VMStateDescription vmstate_uuid = { > + .name = "configuration/uuid", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = vmstate_uuid_needed, > + .post_load = vmstate_uuid_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8_ARRAY_V(uuid.data, SaveState, sizeof(QemuUUID), 1), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_configuration = { > .name = "configuration", > .version_id = 1, > @@ -478,6 +522,7 @@ static const VMStateDescription vmstate_configuration = { > .subsections = (const VMStateDescription*[]) { > &vmstate_target_page_bits, > &vmstate_capabilites, > + &vmstate_uuid, > NULL > } > }; > diff --git a/qapi/migration.json b/qapi/migration.json > index 9cfbaf8c6c..b7a8064745 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -415,6 +415,9 @@ > # > # @x-ignore-shared: If enabled, QEMU will not migrate shared memory (since 4.0) > # > +# @x-validate-uuid: Check whether the UUID is the same on both sides or not. > +# (since 4.2) > +# > # Since: 1.2 > ## > { 'enum': 'MigrationCapability', > @@ -422,7 +425,7 @@ > 'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram', > 'block', 'return-path', 'pause-before-switchover', 'multifd', > 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', > - 'x-ignore-shared' ] } > + 'x-ignore-shared', 'x-validate-uuid' ] } > > ## > # @MigrationCapabilityStatus: > -- > 2.22.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
03.09.2019, 14:25, "Dr. David Alan Gilbert" <dgilbert@redhat.com>: > * Eric Blake (eblake@redhat.com) wrote: >> On 8/27/19 10:36 AM, Yury Kotov wrote: >> > 27.08.2019, 17:02, "Eric Blake" <eblake@redhat.com>: >> >> On 8/27/19 7:02 AM, Yury Kotov wrote: >> >>> This capability realizes simple source validation by UUID. >> >>> It's useful for live migration between hosts. >> >>> >> >> >> >> >> Any reason why this is marked experimental? It seems useful enough that >> >> we could probably just add it as a fully-supported feature (dropping the >> >> x- prefix) - but I'll leave that up to the migration maintainers. >> >> >> > >> > I thought that all new capabilities have x- prefix... May be it's really >> > unnecessary here, I'm not sure. >> >> New features that need some testing or possible changes to behavior need >> x- to mark them as experimental, so we can make those changes without >> worrying about breaking back-compat. But new features that are outright >> useful and presumably in their final form, with no further >> experimentation needed, can skip going through an x- phase. >> >> > >> >> In fact, do we even need this to be a tunable feature? Why not just >> >> always enable it? As long as the UUID is sent in a way that new->old >> >> doesn't break the old qemu from receiving the migration stream, and that >> >> old->new copes with UUID being absent, then new->new will always benefit >> >> from the additional safety check. >> >> >> > >> > In such case we couldn't migrate from e.g. 4.2 to 3.1 >> >> I don't know the migration format enough to know if there is a way for >> 4.2 to unconditionally send a UUID as a subsection such that a receiving >> 3.1 will ignore the unknown subsection. If so, then you don't need a >> knob; if not, then you need something to say whether sending the >> subsection is safe (perhaps default on in new machine types, but default >> off for machine types that might still be migrated back to 3.1). That's >> where I'm hoping the migration experts will chime in. > > Right; the migration format can't ignore chunks of data; so it does need > to know somehow; the choice is either a capability or wiring it to the > machine type; my preference is to wire it to the machine type; the > arguments are: > a) Machine type > Pro: libvirt doesn't need to do anything > Con: It doesn't protect old machine types > It's not really part of the guest state > > b) Capability > Pro: Works on all machine types > Con: Libvirt needs to enable it > > So, no strong preference but I think I prefer (a). IIUC the (a) option requires to add a piece of code to every machine type. This is much more complicated than adding a capability. If you don't mind, I suggest to keep the current version. > > Dave > >> -- >> Eric Blake, Principal Software Engineer >> Red Hat, Inc. +1-919-301-3226 >> Virtualization: qemu.org | libvirt.org > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK Regards, Yury
* Yury Kotov (yury-kotov@yandex-team.ru) wrote: > 03.09.2019, 14:25, "Dr. David Alan Gilbert" <dgilbert@redhat.com>: > > * Eric Blake (eblake@redhat.com) wrote: > >> On 8/27/19 10:36 AM, Yury Kotov wrote: > >> > 27.08.2019, 17:02, "Eric Blake" <eblake@redhat.com>: > >> >> On 8/27/19 7:02 AM, Yury Kotov wrote: > >> >>> This capability realizes simple source validation by UUID. > >> >>> It's useful for live migration between hosts. > >> >>> > >> > >> >> > >> >> Any reason why this is marked experimental? It seems useful enough that > >> >> we could probably just add it as a fully-supported feature (dropping the > >> >> x- prefix) - but I'll leave that up to the migration maintainers. > >> >> > >> > > >> > I thought that all new capabilities have x- prefix... May be it's really > >> > unnecessary here, I'm not sure. > >> > >> New features that need some testing or possible changes to behavior need > >> x- to mark them as experimental, so we can make those changes without > >> worrying about breaking back-compat. But new features that are outright > >> useful and presumably in their final form, with no further > >> experimentation needed, can skip going through an x- phase. > >> > >> > > >> >> In fact, do we even need this to be a tunable feature? Why not just > >> >> always enable it? As long as the UUID is sent in a way that new->old > >> >> doesn't break the old qemu from receiving the migration stream, and that > >> >> old->new copes with UUID being absent, then new->new will always benefit > >> >> from the additional safety check. > >> >> > >> > > >> > In such case we couldn't migrate from e.g. 4.2 to 3.1 > >> > >> I don't know the migration format enough to know if there is a way for > >> 4.2 to unconditionally send a UUID as a subsection such that a receiving > >> 3.1 will ignore the unknown subsection. If so, then you don't need a > >> knob; if not, then you need something to say whether sending the > >> subsection is safe (perhaps default on in new machine types, but default > >> off for machine types that might still be migrated back to 3.1). That's > >> where I'm hoping the migration experts will chime in. > > > > Right; the migration format can't ignore chunks of data; so it does need > > to know somehow; the choice is either a capability or wiring it to the > > machine type; my preference is to wire it to the machine type; the > > arguments are: > > a) Machine type > > Pro: libvirt doesn't need to do anything > > Con: It doesn't protect old machine types > > It's not really part of the guest state > > > > b) Capability > > Pro: Works on all machine types > > Con: Libvirt needs to enable it > > > > So, no strong preference but I think I prefer (a). > > IIUC the (a) option requires to add a piece of code to every machine type. > This is much more complicated than adding a capability. Actually it doesn't - you just add a property, default it to true and then add an entry in hw_compat_4_1 to turn it off for older types. > If you don't mind, I suggest to keep the current version. That's OK. Dave > > > > Dave > > > >> -- > >> Eric Blake, Principal Software Engineer > >> Red Hat, Inc. +1-919-301-3226 > >> Virtualization: qemu.org | libvirt.org > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > Regards, > Yury -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/migration.c b/migration/migration.c index 8b9f2fe30a..910e11b7d7 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2140,6 +2140,15 @@ bool migrate_ignore_shared(void) return s->enabled_capabilities[MIGRATION_CAPABILITY_X_IGNORE_SHARED]; } +bool migrate_validate_uuid(void) +{ + MigrationState *s; + + s = migrate_get_current(); + + return s->enabled_capabilities[MIGRATION_CAPABILITY_X_VALIDATE_UUID]; +} + bool migrate_use_events(void) { MigrationState *s; diff --git a/migration/migration.h b/migration/migration.h index 3e1ea2b5dc..4f2fe193dc 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -290,6 +290,7 @@ bool migrate_postcopy_ram(void); bool migrate_zero_blocks(void); bool migrate_dirty_bitmaps(void); bool migrate_ignore_shared(void); +bool migrate_validate_uuid(void); bool migrate_auto_converge(void); bool migrate_use_multifd(void); diff --git a/migration/savevm.c b/migration/savevm.c index 4a86128ac4..493dc24fd2 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -256,6 +256,7 @@ typedef struct SaveState { uint32_t target_page_bits; uint32_t caps_count; MigrationCapability *capabilities; + QemuUUID uuid; } SaveState; static SaveState savevm_state = { @@ -307,6 +308,7 @@ static int configuration_pre_save(void *opaque) state->capabilities[j++] = i; } } + state->uuid = qemu_uuid; return 0; } @@ -464,6 +466,48 @@ static const VMStateDescription vmstate_capabilites = { } }; +static bool vmstate_uuid_needed(void *opaque) +{ + return qemu_uuid_set && migrate_validate_uuid(); +} + +static int vmstate_uuid_post_load(void *opaque, int version_id) +{ + SaveState *state = opaque; + char uuid_src[UUID_FMT_LEN + 1]; + char uuid_dst[UUID_FMT_LEN + 1]; + + if (!qemu_uuid_set) { + /* + * It's warning because user might not know UUID in some cases, + * e.g. load an old snapshot + */ + qemu_uuid_unparse(&state->uuid, uuid_src); + warn_report("UUID is received %s, but local uuid isn't set", + uuid_src); + return 0; + } + if (!qemu_uuid_is_equal(&state->uuid, &qemu_uuid)) { + qemu_uuid_unparse(&state->uuid, uuid_src); + qemu_uuid_unparse(&qemu_uuid, uuid_dst); + error_report("UUID received is %s and local is %s", uuid_src, uuid_dst); + return -EINVAL; + } + return 0; +} + +static const VMStateDescription vmstate_uuid = { + .name = "configuration/uuid", + .version_id = 1, + .minimum_version_id = 1, + .needed = vmstate_uuid_needed, + .post_load = vmstate_uuid_post_load, + .fields = (VMStateField[]) { + VMSTATE_UINT8_ARRAY_V(uuid.data, SaveState, sizeof(QemuUUID), 1), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_configuration = { .name = "configuration", .version_id = 1, @@ -478,6 +522,7 @@ static const VMStateDescription vmstate_configuration = { .subsections = (const VMStateDescription*[]) { &vmstate_target_page_bits, &vmstate_capabilites, + &vmstate_uuid, NULL } }; diff --git a/qapi/migration.json b/qapi/migration.json index 9cfbaf8c6c..b7a8064745 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -415,6 +415,9 @@ # # @x-ignore-shared: If enabled, QEMU will not migrate shared memory (since 4.0) # +# @x-validate-uuid: Check whether the UUID is the same on both sides or not. +# (since 4.2) +# # Since: 1.2 ## { 'enum': 'MigrationCapability', @@ -422,7 +425,7 @@ 'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram', 'block', 'return-path', 'pause-before-switchover', 'multifd', 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', - 'x-ignore-shared' ] } + 'x-ignore-shared', 'x-validate-uuid' ] } ## # @MigrationCapabilityStatus:
This capability realizes simple source validation by UUID. It's useful for live migration between hosts. Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru> --- migration/migration.c | 9 +++++++++ migration/migration.h | 1 + migration/savevm.c | 45 +++++++++++++++++++++++++++++++++++++++++++ qapi/migration.json | 5 ++++- 4 files changed, 59 insertions(+), 1 deletion(-)