[libvirt] [PATCH] conf: Fix backwards migration of pSeries guests
Recent commits made it so that pci-root controllers for pSeries guests are automatically assigned the spapr-pci-host-bridge model name; however, that prevents guests to migrate to older versions of libvirt which don't know about that model name at all, which at the moment is all of them :) To avoid the issue, just strip the model name from PHBs when formatting the migratable XML; guests that use more than one PHB are not going to be migratable anyway. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9320794..21bd7c7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21919,7 +21919,20 @@ virDomainControllerDefFormat(virBufferPtr buf, } if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (def->opts.pciopts.modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + bool formatModelName = true; + + if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + formatModelName = false; + + /* Don't format the model name for PHBs when migrating so that + * guests that only use the default one can be migrated to older + * libvirt version which don't know about PHBs at all */ + if (virDomainControllerIsPCIHostBridge(def) && + flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE) { + formatModelName = false; + } + + if (formatModelName) { modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); if (!modelName) { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.7.5
On Wed, Jul 19, 2017 at 14:10:27 +0200, Andrea Bolognani wrote:
Recent commits made it so that pci-root controllers for
Did we release this?
pSeries guests are automatically assigned the spapr-pci-host-bridge model name; however, that prevents guests to migrate to older versions of libvirt which don't know about that model name at all, which at the moment is all of them :)
To avoid the issue, just strip the model name from PHBs when formatting the migratable XML; guests that use more than one PHB are not going to be migratable anyway.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9320794..21bd7c7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21919,7 +21919,20 @@ virDomainControllerDefFormat(virBufferPtr buf, }
if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (def->opts.pciopts.modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + bool formatModelName = true; + + if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + formatModelName = false; + + /* Don't format the model name for PHBs when migrating so that + * guests that only use the default one can be migrated to older + * libvirt version which don't know about PHBs at all */ + if (virDomainControllerIsPCIHostBridge(def) &&
This function has a confusing name, since it's explicitly checking for a pSeries specific thing but the name does not really imply that.
+ flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE) { + formatModelName = false; + }
Won't this suppress the formatting of the type even if the user specified it explicitly? In that case we should not suppress it IMO. It will cause problems once there's a different model available. If the term 'PCI host bridge' is a p-series specific thing, we should make it more obvious in the code since it's too generic sounding without knowledge of the context.
On Wed, 2017-07-19 at 15:33 +0200, Peter Krempa wrote:
Recent commits made it so that pci-root controllers for Did we release this?
Nope, it will be in the next release.
+ /* Don't format the model name for PHBs when migrating so that + * guests that only use the default one can be migrated to older + * libvirt version which don't know about PHBs at all */ + if (virDomainControllerIsPCIHostBridge(def) && This function has a confusing name, since it's explicitly checking for a pSeries specific thing but the name does not really imply that.
Point taken about pSeries not being mentioned in the name, but PHBs are a pSeries-specific concept so it didn't really occur to me at the time.
+ flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE) { + formatModelName = false; + } Won't this suppress the formatting of the type even if the user specified it explicitly? In that case we should not suppress it IMO. It will cause problems once there's a different model available.
How would the code know whether the model was set manually by the user or automatically by libvirt? In any case, that made me realize that not sending the model, even if automatically filled in, could cause issues in the future if a new model is added and becomes the default, as the guest ABI would not be preserved during migration then. Any help on how to deal with this? I'm clearly not very good at the whole migration thing :(
If the term 'PCI host bridge' is a p-series specific thing, we should make it more obvious in the code since it's too generic sounding without knowledge of the context.
Maybe I can change it to virDomainControllerIsPSeriesPHB()? That would require passing in the virDomainDef though. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Jul 19, 2017 at 16:41:54 +0200, Andrea Bolognani wrote:
On Wed, 2017-07-19 at 15:33 +0200, Peter Krempa wrote:
Recent commits made it so that pci-root controllers for Did we release this?
Nope, it will be in the next release.
+ /* Don't format the model name for PHBs when migrating so that + * guests that only use the default one can be migrated to older + * libvirt version which don't know about PHBs at all */ + if (virDomainControllerIsPCIHostBridge(def) && This function has a confusing name, since it's explicitly checking for a pSeries specific thing but the name does not really imply that.
Point taken about pSeries not being mentioned in the name, but PHBs are a pSeries-specific concept so it didn't really occur to me at the time.
So I thin that function should be renamed, because the "PCI Host Bridge" name sounds really generic.
+ flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE) { + formatModelName = false; + } Won't this suppress the formatting of the type even if the user specified it explicitly? In that case we should not suppress it IMO. It will cause problems once there's a different model available.
How would the code know whether the model was set manually by the user or automatically by libvirt?
You can record it in a boolean.
In any case, that made me realize that not sending the model, even if automatically filled in, could cause issues in the future if a new model is added and becomes the default, as the guest ABI would not be preserved during migration then.
That is the main reason to fill all the values in right away. Since there apparently was a period, where a default would be used, but not recorded, it needs some trickery unfortunately. In such case you basically standardize, that the now-filled-in default model (which can't ever change) if it was not provided by the user is to be dropped from the migratable XML. Once you start assign a new default model, that then needs to be explicitly sent over, so that the migration will not be successufl unless the destination is able to use the new model. This means basically, that a missing model name becomes assigned to a particular value.
Any help on how to deal with this? I'm clearly not very good at the whole migration thing :(
If the term 'PCI host bridge' is a p-series specific thing, we should make it more obvious in the code since it's too generic sounding without knowledge of the context.
Maybe I can change it to virDomainControllerIsPSeriesPHB()? That would require passing in the virDomainDef though.
That looks way better. It's clear at first glance that it's not a general thing. (At least for the lazy not reading the documentation for the function)
On Thu, 2017-07-20 at 09:55 +0200, Peter Krempa wrote:
In any case, that made me realize that not sending the model, even if automatically filled in, could cause issues in the future if a new model is added and becomes the default, as the guest ABI would not be preserved during migration then. That is the main reason to fill all the values in right away. Since there apparently was a period, where a default would be used, but not recorded, it needs some trickery unfortunately. In such case you basically standardize, that the now-filled-in default model (which can't ever change) if it was not provided by the user is to be dropped from the migratable XML. Once you start assign a new default model, that then needs to be explicitly sent over, so that the migration will not be successufl unless the destination is able to use the new model. This means basically, that a missing model name becomes assigned to a particular value.
That makes sense. Doesn't it also mean that we don't really need to record whether the user set the model name explicitly or not? We can just skip formatting it if it's spapr-pci-host-bridge and all versions of libvirt, past or future, will handle that correctly. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, 2017-07-21 at 10:07 +0200, Andrea Bolognani wrote:
That is the main reason to fill all the values in right away. Since there apparently was a period, where a default would be used, but not recorded, it needs some trickery unfortunately. In such case you basically standardize, that the now-filled-in default model (which can't ever change) if it was not provided by the user is to be dropped from the migratable XML. Once you start assign a new default model, that then needs to be explicitly sent over, so that the migration will not be successufl unless the destination is able to use the new model. This means basically, that a missing model name becomes assigned to a particular value. That makes sense. Doesn't it also mean that we don't really need to record whether the user set the model name explicitly or not? We can just skip formatting it if it's spapr-pci-host-bridge and all versions of libvirt, past or future, will handle that correctly.
Wait, that's exactly how this patch behaves, except it's not immediately apparent because the check for the model name happens inside virDomainControllerIsPCIHostBridge() rather than being explicit. So maybe I'm still missing something? -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jul 21, 2017 at 10:39:05 +0200, Andrea Bolognani wrote:
On Fri, 2017-07-21 at 10:07 +0200, Andrea Bolognani wrote:
[...]
That makes sense. Doesn't it also mean that we don't really need to record whether the user set the model name explicitly or not? We can just skip formatting it if it's spapr-pci-host-bridge and all versions of libvirt, past or future, will handle that correctly.
Wait, that's exactly how this patch behaves, except it's not immediately apparent because the check for the model name happens inside virDomainControllerIsPCIHostBridge() rather than being explicit.
So maybe I'm still missing something?
You are right. The not-formatting only the first model ever added should be fine in this case also in the future. Please send a fixed version of this patch that will make sure that it states that the PHB thing is a pSeries thing, so that it does not confuse others as it did me.
participants (2)
-
Andrea Bolognani -
Peter Krempa