[libvirt] [PATCH v2] qemu: neglect cur_balloon in supplied xml
John Ferlan
jferlan at redhat.com
Sat Jul 30 13:04:30 UTC 2016
s/$subj/qemu: Filter cur_balloon ABI check for certain transactions
On 06/09/2016 10:32 AM, Nikolay Shirokovskiy wrote:
> cur_balloon value can change in between preparing external config and
> using it in operations like save and migrate. As a resutl operation will
> fail for ABI inconsistency. cur_balloon changes can not be predicted
> generally and thus operations will fail from time to time.
>
> Skip checking cur_balloon if domain lock can not be hold between
> preparing external config outside of libvirt and checking it against active
> config. Instead update cur_balloon value in external config from active config.
> This way it is protected from forges and is keeped up to date too.
>
s/$commit/
Since the domain lock is not held during preparation of an external XML
config, it is possible that the value can change resulting in unexpected
failures during ABI consistency checking for some save and migrate
operations.
This patch adds a new flag to skip the checking of the cur_balloon value
and then sets the destination value to the source value to ensure
subsequent checks without the skip flag will succeed.
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
> src/conf/domain_conf.c | 14 +++++++++++---
> src/conf/domain_conf.h | 9 +++++++++
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_domain.c | 29 ++++++++++++++++++++---------
> src/qemu/qemu_domain.h | 6 +++---
> src/qemu/qemu_driver.c | 5 +++--
> src/qemu/qemu_migration.c | 4 ++--
> 7 files changed, 49 insertions(+), 19 deletions(-)
>
This change seems reasonable to me and it doesn't seem to negate the
primary focus of commit id 'f2fc1eee' (which added the checks).
I do have a few suggestions and can make those locally before pushing
after the current release is cut.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 85f6e31..f1cf87f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18493,8 +18493,9 @@ virDomainDefVcpuCheckAbiStability(virDomainDefPtr src,
> * validation of custom XML config passed in during migration
> */
> bool
> -virDomainDefCheckABIStability(virDomainDefPtr src,
> - virDomainDefPtr dst)
> +virDomainDefCheckABIStabilityFlags(virDomainDefPtr src,
> + virDomainDefPtr dst,
> + unsigned int flags)
> {
> size_t i;
> virErrorPtr err;
> @@ -18538,7 +18539,8 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
> virDomainDefGetMemoryInitial(src));
> goto error;
> }
> - if (src->mem.cur_balloon != dst->mem.cur_balloon) {
> + if (!(flags & VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE) &&
> + src->mem.cur_balloon != dst->mem.cur_balloon) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Target domain current memory %lld does not match source %lld"),
> dst->mem.cur_balloon, src->mem.cur_balloon);
> @@ -18963,6 +18965,12 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
> return false;
> }
>
Need extra line between functions
> +bool
> +virDomainDefCheckABIStability(virDomainDefPtr src,
> + virDomainDefPtr dst)
> +{
> + return virDomainDefCheckABIStabilityFlags(src, dst, 0);
> +}
>
Same here.
> static int
> virDomainDefAddDiskControllersForType(virDomainDefPtr def,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3792562..cd7b966 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2623,6 +2623,11 @@ typedef enum {
> VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 9,
> } virDomainDefFormatFlags;
>
> +typedef enum {
> + /* skip checking values like cur_balloon that can be changed meanwhile */
> + VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE = 1 << 0,
> +} virDomainDefABICheckFlags;
> +
How about this instead?
/* Use these flags to skip specific domain ABI consistency checks done
* in virDomainDefCheckABIStabilityFlags.
*/
typedef enum {
/* Set when domain lock must be released and there exists the
possibility
* that some external action could alter the value, such as
cur_balloon. */
VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE = 1 << 0,
} virDomainDefABICheckFlags;
> virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr,
> const virDomainDef *def,
> virCapsPtr caps,
> @@ -2658,6 +2663,10 @@ virDomainObjPtr virDomainObjParseFile(const char *filename,
> bool virDomainDefCheckABIStability(virDomainDefPtr src,
> virDomainDefPtr dst);
>
> +bool virDomainDefCheckABIStabilityFlags(virDomainDefPtr src,
> + virDomainDefPtr dst,
> + unsigned int flags);
> +
> int virDomainDefAddImplicitDevices(virDomainDefPtr def);
>
> virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(const virDomainDef *def,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 42f664c..4e7840c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -208,6 +208,7 @@ virDomainDefAddController;
> virDomainDefAddImplicitDevices;
> virDomainDefAddUSBController;
> virDomainDefCheckABIStability;
> +virDomainDefCheckABIStabilityFlags;
> virDomainDefClearCCWAddresses;
> virDomainDefClearDeviceAliases;
> virDomainDefClearPCIAddresses;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d1f8175..4b45caf 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4620,21 +4620,32 @@ qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr driver,
> }
>
>
> -bool
> -qemuDomainDefCheckABIStability(virQEMUDriverPtr driver,
> - virDomainDefPtr src,
> - virDomainDefPtr dst)
> +int
> +qemuDomainDefUpdateVolatile(virQEMUDriverPtr driver,
> + virDomainDefPtr src,
> + virDomainDefPtr dst)
If we didn't change the name, then there would be less changes... I know
with the change it's now more than just a Check function, but I wish
there was some way to retain the ABI in the name. I don't have any
suggestions, but maybe with this bump someone will look and have one;
otherwise, I can leave it as is.
> {
> virDomainDefPtr migratableDefSrc = NULL;
> virDomainDefPtr migratableDefDst = NULL;
> - const int flags = VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_UPDATE_CPU | VIR_DOMAIN_XML_MIGRATABLE;
> - bool ret = false;
> + const unsigned int copy_flags = VIR_DOMAIN_XML_SECURE |
> + VIR_DOMAIN_XML_UPDATE_CPU |
> + VIR_DOMAIN_XML_MIGRATABLE;
> + const unsigned int check_flags = VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE;
> + int ret = -1;
>
> - if (!(migratableDefSrc = qemuDomainDefCopy(driver, src, flags)) ||
> - !(migratableDefDst = qemuDomainDefCopy(driver, dst, flags)))
Really didn't have to change 'flags', just introduce the new one.
> +
> + if (!(migratableDefSrc = qemuDomainDefCopy(driver, src, copy_flags)) ||
> + !(migratableDefDst = qemuDomainDefCopy(driver, dst, copy_flags)))
> + goto cleanup;
> +
> + if (!virDomainDefCheckABIStabilityFlags(migratableDefSrc,
> + migratableDefDst,
> + check_flags))
> goto cleanup;
>
> - ret = virDomainDefCheckABIStability(migratableDefSrc, migratableDefDst);
Added a comment...
/* Force update any skipped values from the volatile flag */
> + dst->mem.cur_balloon = src->mem.cur_balloon;
> +
> + ret = 0;
>
> cleanup:
> virDomainDefFree(migratableDefSrc);
The rest wouldn't be necessary if the function name stayed the same...
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 2443e97..7581fff 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -578,9 +578,9 @@ int qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> int asyncJob);
>
> -bool qemuDomainDefCheckABIStability(virQEMUDriverPtr driver,
> - virDomainDefPtr src,
> - virDomainDefPtr dst);
> +int qemuDomainDefUpdateVolatile(virQEMUDriverPtr driver,
> + virDomainDefPtr src,
> + virDomainDefPtr dst);
>
> bool qemuDomainAgentAvailable(virDomainObjPtr vm,
> bool reportError);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e70d3ce..a819f53 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3244,7 +3244,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
> VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) {
> goto endjob;
> }
> - if (!qemuDomainDefCheckABIStability(driver, vm->def, def)) {
> + if (qemuDomainDefUpdateVolatile(driver, vm->def, def) < 0) {
> virDomainDefFree(def);
> goto endjob;
> }
> @@ -15103,7 +15103,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> /* Transitions 5, 6, 8, 9 */
> /* Check for ABI compatibility. We need to do this check against
> * the migratable XML or it will always fail otherwise */
> - if (config && !qemuDomainDefCheckABIStability(driver, vm->def, config)) {
> + if (config
> + && qemuDomainDefUpdateVolatile(driver, vm->def, config) < 0) {
Put the && on the previous line
John
> virErrorPtr err = virGetLastError();
>
> if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 89350c8..38f471b 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3237,7 +3237,7 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
> VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
> goto cleanup;
>
> - if (!qemuDomainDefCheckABIStability(driver, vm->def, def))
> + if (qemuDomainDefUpdateVolatile(driver, vm->def, def) < 0)
> goto cleanup;
>
> rv = qemuDomainDefFormatLive(driver, def, false, true);
> @@ -3582,7 +3582,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> if (!newdef)
> goto cleanup;
>
> - if (!qemuDomainDefCheckABIStability(driver, *def, newdef)) {
> + if (qemuDomainDefUpdateVolatile(driver, *def, newdef) < 0) {
> virDomainDefFree(newdef);
> goto cleanup;
> }
>
More information about the libvir-list
mailing list