8:35 a.m.
We don't support all startup policies with all source types so to
correctly allow switching from a 'file' based cdrom with 'optional'
startup policy to a 'block' based one which doesn't support optional we
must update the startup policy field first. Obviously we need to have
fallback if the update fails.
Reported-by: Vojtech Juranek <vjuranek(a)redhat.com>
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_driver.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bd41ddbc3c..dfc27572c4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7038,6 +7038,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
{
virDomainDiskDef *disk = dev->data.disk;
virDomainDiskDef *orig_disk = NULL;
+ virDomainStartupPolicy origStartupPolicy;
virDomainDeviceDef oldDev = { .type = dev->type };
if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) {
@@ -7047,6 +7048,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
}
oldDev.data.disk = orig_disk;
+ origStartupPolicy = orig_disk->startupPolicy;
if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
VIR_DOMAIN_DEVICE_ACTION_UPDATE,
true) < 0)
@@ -7065,13 +7067,20 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
return -1;
}
+ /* update startup policy first before updating disk image */
+ orig_disk->startupPolicy = dev->data.disk->startupPolicy;
+
if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
- dev->data.disk->src, force) < 0)
+ dev->data.disk->src, force) < 0) {
+ /* revert startup policy before failing */
+ orig_disk->startupPolicy = origStartupPolicy;
return -1;
+ }
dev->data.disk->src = NULL;
}
+ /* in case when we aren't updating disk source we update startup policy here */
orig_disk->startupPolicy = dev->data.disk->startupPolicy;
orig_disk->snapshot = dev->data.disk->snapshot;
--
2.31.1
9:04 a.m.
On 9/10/21 3:35 PM, Peter Krempa wrote:
We don't support all startup policies with all source types so
to
correctly allow switching from a 'file' based cdrom with 'optional'
startup policy to a 'block' based one which doesn't support optional we
must update the startup policy field first. Obviously we need to have
fallback if the update fails.
Reported-by: Vojtech Juranek <vjuranek(a)redhat.com>
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_driver.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
2:04 p.m.
On Fri, Sep 10, 2021 at 4:35 PM Peter Krempa <pkrempa(a)redhat.com> wrote:
We don't support all startup policies with all source types so to
correctly allow switching from a 'file' based cdrom with 'optional'
startup policy to a 'block' based one which doesn't support optional we
must update the startup policy field first. Obviously we need to have
fallback if the update fails.
Why is there a difference between file and block for startup policy?
Is this documented?
Do we have a way to switch from file to block with current libvirt
(centos 8 stream, rhel 8.4) without this patch, or do we need to wait
until this fix is available? (rhel 8.5?)
Reported-by: Vojtech Juranek <vjuranek(a)redhat.com>
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_driver.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bd41ddbc3c..dfc27572c4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7038,6 +7038,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
{
virDomainDiskDef *disk = dev->data.disk;
virDomainDiskDef *orig_disk = NULL;
+ virDomainStartupPolicy origStartupPolicy;
virDomainDeviceDef oldDev = { .type = dev->type };
if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) {
@@ -7047,6 +7048,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
}
oldDev.data.disk = orig_disk;
+ origStartupPolicy = orig_disk->startupPolicy;
if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
VIR_DOMAIN_DEVICE_ACTION_UPDATE,
true) < 0)
@@ -7065,13 +7067,20 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
return -1;
}
+ /* update startup policy first before updating disk image */
+ orig_disk->startupPolicy = dev->data.disk->startupPolicy;
+
if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
- dev->data.disk->src, force) < 0)
+ dev->data.disk->src, force) < 0) {
+ /* revert startup policy before failing */
+ orig_disk->startupPolicy = origStartupPolicy;
return -1;
+ }
dev->data.disk->src = NULL;
}
+ /* in case when we aren't updating disk source we update startup policy here */
orig_disk->startupPolicy = dev->data.disk->startupPolicy;
orig_disk->snapshot = dev->data.disk->snapshot;
--
2.31.1
3:04 a.m.
On Fri, Sep 10, 2021 at 22:04:01 +0300, Nir Soffer wrote:
On Fri, Sep 10, 2021 at 4:35 PM Peter Krempa
<pkrempa(a)redhat.com> wrote:
>
> We don't support all startup policies with all source types so to
> correctly allow switching from a 'file' based cdrom with 'optional'
> startup policy to a 'block' based one which doesn't support optional we
> must update the startup policy field first. Obviously we need to have
> fallback if the update fails.
Why is there a difference between file and block for startup policy?
I'm not sure about the historic reasons for why 'block' was not
considered when this was implemented originally.
Is this documented?
(citation from 'formatdomain.rst' the source for
https://www.libvirt.org/formatdomain.html )
For a "file" or "volume" disk type which represents a cdrom or
floppy (the
``device`` attribute), it is possible to define policy what to do with the
disk if the source file is not accessible. (NB, ``startupPolicy`` is not
valid for "volume" disk unless the specified storage volume is of
"file"
type). This is done by the ``startupPolicy`` attribute ( :since:`since 0.9.7`
), accepting these values:
========= =====================================================================
mandatory fail if missing for any reason (the default)
requisite fail if missing on boot up, drop if missing on migrate/restore/revert
optional drop if missing at any start attempt
========= =====================================================================
:since:`Since 1.1.2` the ``startupPolicy`` is extended to support hard disks
besides cdrom and floppy. On guest cold bootup, if a certain disk is not
accessible or its disk chain is broken, with startupPolicy 'optional' the
guest will drop this disk. This feature doesn't support migration currently.
As you can notice only "file" and "volume" (pointing to a file) are
allowed.
Do we have a way to switch from file to block with current libvirt
(centos 8 stream, rhel 8.4) without this patch, or do we need to wait
until this fix is available? (rhel 8.5?)
You can issue two separate update calls. One updating the startup policy
first and then the second one updating the source.
On another note, I don't quite understand though why oVirt actually even
uses startup policy in the first place. Since oVirt is already doing a
pretty complicated storage management, checking whether the image file
exists is a trivial operation.
Similarly, when migrating you can use the destination XML (which can be
optionally used with the migration API) allows you to update storage
source and even provide empty storage for a cdrom. This can be used to
update paths to storage too. This allows superior flexibility when
compared to startup policy.
6:30 a.m.
On Mon, Sep 13, 2021 at 11:04 AM Peter Krempa <pkrempa(a)redhat.com> wrote:
On Fri, Sep 10, 2021 at 22:04:01 +0300, Nir Soffer wrote:
> On Fri, Sep 10, 2021 at 4:35 PM Peter Krempa <pkrempa(a)redhat.com> wrote:
> >
> > We don't support all startup policies with all source types so to
> > correctly allow switching from a 'file' based cdrom with
'optional'
> > startup policy to a 'block' based one which doesn't support
optional we
> > must update the startup policy field first. Obviously we need to have
> > fallback if the update fails.
>
> Why is there a difference between file and block for startup policy?
I'm not sure about the historic reasons for why 'block' was not
considered when this was implemented originally.
> Is this documented?
(citation from 'formatdomain.rst' the source for
https://www.libvirt.org/formatdomain.html )
For a "file" or "volume" disk type which represents a cdrom or
floppy (the
``device`` attribute), it is possible to define policy what to do with the
disk if the source file is not accessible. (NB, ``startupPolicy`` is not
valid for "volume" disk unless the specified storage volume is of
"file"
type). This is done by the ``startupPolicy`` attribute ( :since:`since 0.9.7`
), accepting these values:
========= =====================================================================
mandatory fail if missing for any reason (the default)
requisite fail if missing on boot up, drop if missing on migrate/restore/revert
optional drop if missing at any start attempt
========= =====================================================================
:since:`Since 1.1.2` the ``startupPolicy`` is extended to support hard disks
besides cdrom and floppy. On guest cold bootup, if a certain disk is not
accessible or its disk chain is broken, with startupPolicy 'optional' the
guest will drop this disk. This feature doesn't support migration currently.
As you can notice only "file" and "volume" (pointing to a file) are
allowed.
Looks like we missed this detail when adding support for cdrom on
block storage.
> Do we have a way to switch from file to block with current
libvirt
> (centos 8 stream, rhel 8.4) without this patch, or do we need to wait
> until this fix is available? (rhel 8.5?)
You can issue two separate update calls. One updating the startup policy
first and then the second one updating the source.
If the second call fails, this leave us with half of the change.
On another note, I don't quite understand though why oVirt
actually even
uses startup policy in the first place. Since oVirt is already doing a
pretty complicated storage management, checking whether the image file
exists is a trivial operation.
In oVirt we know that the image exists since we prepare it before
starting the VM
(e.g. activate logical volume), and we will fail the operation if the
disk does not
exists, so I think this attribute is not needed for our use case and
it should be
removed.
Similarly, when migrating you can use the destination XML (which can
be
optionally used with the migration API) allows you to update storage
source and even provide empty storage for a cdrom. This can be used to
update paths to storage too. This allows superior flexibility when
compared to startup policy.
Even if we remove startupPolicy in new oVirt version, we have to
support migration
from an older version using startupPolicy in the xml.
Nir
6:55 a.m.
On Mon, Sep 13, 2021 at 14:30:45 +0300, Nir Soffer wrote:
On Mon, Sep 13, 2021 at 11:04 AM Peter Krempa
<pkrempa(a)redhat.com> wrote:
>
> On Fri, Sep 10, 2021 at 22:04:01 +0300, Nir Soffer wrote:
> > On Fri, Sep 10, 2021 at 4:35 PM Peter Krempa <pkrempa(a)redhat.com> wrote:
[...]
> > Do we have a way to switch from file to block with current
libvirt
> > (centos 8 stream, rhel 8.4) without this patch, or do we need to wait
> > until this fix is available? (rhel 8.5?)
>
> You can issue two separate update calls. One updating the startup policy
> first and then the second one updating the source.
If the second call fails, this leave us with half of the change.
Based on what you write below, where you state that you actually make
sure that the image exists it seems that it should not be a problem at
all.
Specifically just removing startupPolicy has no guest visible change so
it's allowed on migration and other cases.
Just removing it from an empty drive has no effect other than recording
it in the libvirt metadata.
Removing it from a full cdrom could have semantic effects on migration
but they are removed by oVirt actually checking before. Same for start
of a new VM where you always re-define the XML anyways.
> On another note, I don't quite understand though why oVirt
actually even
> uses startup policy in the first place. Since oVirt is already doing a
> pretty complicated storage management, checking whether the image file
> exists is a trivial operation.
In oVirt we know that the image exists since we prepare it before
starting the VM
(e.g. activate logical volume), and we will fail the operation if the
disk does not
exists, so I think this attribute is not needed for our use case and
it should be
removed.
> Similarly, when migrating you can use the destination XML (which can be
> optionally used with the migration API) allows you to update storage
> source and even provide empty storage for a cdrom. This can be used to
> update paths to storage too. This allows superior flexibility when
> compared to startup policy.
Even if we remove startupPolicy in new oVirt version, we have to
support migration
from an older version using startupPolicy in the xml.
Speaking of live migration you can remove it from the destination XML,
since startup policy is exempt from the ABI stability check ... well
technically it's not even ABI.
The only case where it could be a problem is when upgrading existing
host as the running VMs would still have it.
6:56 a.m.
On Monday, 13 September 2021 13:30:45 CEST Nir Soffer wrote:
On Mon, Sep 13, 2021 at 11:04 AM Peter Krempa
<pkrempa(a)redhat.com> wrote:
> On Fri, Sep 10, 2021 at 22:04:01 +0300, Nir Soffer wrote:
> > On Fri, Sep 10, 2021 at 4:35 PM Peter Krempa <pkrempa(a)redhat.com> wrote:
> > > We don't support all startup policies with all source types so to
> > > correctly allow switching from a 'file' based cdrom with
'optional'
> > > startup policy to a 'block' based one which doesn't support
optional
> > > we
> > > must update the startup policy field first. Obviously we need to have
> > > fallback if the update fails.
> >
> > Why is there a difference between file and block for startup policy?
>
> I'm not sure about the historic reasons for why 'block' was not
> considered when this was implemented originally.
>
> > Is this documented?
>
> (citation from 'formatdomain.rst' the source for
> https://www.libvirt.org/formatdomain.html )>
> For a "file" or "volume" disk type which represents a cdrom
or floppy
> (the
> ``device`` attribute), it is possible to define policy what to do with
> the
> disk if the source file is not accessible. (NB, ``startupPolicy`` is
> not
> valid for "volume" disk unless the specified storage volume is of
> "file"
> type). This is done by the ``startupPolicy`` attribute ( :since:`since
> 0.9.7` ), accepting these values:
>
> =========
> =====================================================================
> mandatory fail if missing for any reason (the default)
> requisite fail if missing on boot up, drop if missing on
> migrate/restore/revert optional drop if missing at any start attempt
> =========
> =====================================================================>
> :since:`Since 1.1.2` the ``startupPolicy`` is extended to support hard
> :disks
>
> besides cdrom and floppy. On guest cold bootup, if a certain disk is
> not
> accessible or its disk chain is broken, with startupPolicy 'optional'
> the
> guest will drop this disk. This feature doesn't support migration
> currently.>
> As you can notice only "file" and "volume" (pointing to a file)
are
> allowed.
Looks like we missed this detail when adding support for cdrom on
block storage.
> > Do we have a way to switch from file to block with current libvirt
> > (centos 8 stream, rhel 8.4) without this patch, or do we need to wait
> > until this fix is available? (rhel 8.5?)
>
> You can issue two separate update calls. One updating the startup policy
> first and then the second one updating the source.
If the second call fails, this leave us with half of the change.
> On another note, I don't quite understand though why oVirt actually even
> uses startup policy in the first place. Since oVirt is already doing a
> pretty complicated storage management, checking whether the image file
> exists is a trivial operation.
In oVirt we know that the image exists since we prepare it before
starting the VM
(e.g. activate logical volume), and we will fail the operation if the
disk does not
exists, so I think this attribute is not needed for our use case and
it should be
removed.
+1
> Similarly, when migrating you can use the destination XML (which
can be
> optionally used with the migration API) allows you to update storage
> source and even provide empty storage for a cdrom. This can be used to
> update paths to storage too. This allows superior flexibility when
> compared to startup policy.
Even if we remove startupPolicy in new oVirt version, we have to
support migration
from an older version using startupPolicy in the xml.
What is the problem here? The issue happens only when you try to change the
CD, not during the migration, so there shouldn't be any problem (I also tried
it now and I can migrate VM with block-based CD without any issue). The issue
is there only when VM is started with empty CD or file-based CD, as in these
cases we added startupPolicy into the XML.
7:01 a.m.
On Mon, Sep 13, 2021 at 13:56:45 +0200, Vojtech Juranek wrote:
On Monday, 13 September 2021 13:30:45 CEST Nir Soffer wrote:
> On Mon, Sep 13, 2021 at 11:04 AM Peter Krempa <pkrempa(a)redhat.com> wrote:
> > On Fri, Sep 10, 2021 at 22:04:01 +0300, Nir Soffer wrote:
> > > On Fri, Sep 10, 2021 at 4:35 PM Peter Krempa <pkrempa(a)redhat.com>
wrote:
[...]
> > On another note, I don't quite understand though why
oVirt actually even
> > uses startup policy in the first place. Since oVirt is already doing a
> > pretty complicated storage management, checking whether the image file
> > exists is a trivial operation.
>
> In oVirt we know that the image exists since we prepare it before
> starting the VM
> (e.g. activate logical volume), and we will fail the operation if the
> disk does not
> exists, so I think this attribute is not needed for our use case and
> it should be
> removed.
+1
> > Similarly, when migrating you can use the destination XML (which can be
> > optionally used with the migration API) allows you to update storage
> > source and even provide empty storage for a cdrom. This can be used to
> > update paths to storage too. This allows superior flexibility when
> > compared to startup policy.
>
> Even if we remove startupPolicy in new oVirt version, we have to
> support migration
> from an older version using startupPolicy in the xml.
What is the problem here? The issue happens only when you try to change the
CD, not during the migration, so there shouldn't be any problem (I also tried
it now and I can migrate VM with block-based CD without any issue). The issue
is there only when VM is started with empty CD or file-based CD, as in these
cases we added startupPolicy into the XML.
I assume that the problem is that removing 'startupPolicy' being added
to such configs would not fix the problem for older VMs which were
started with 'startupPolicy' being present thus you'd have to have the
code removing it anyways.
Live migration isn't a problem though as I've pointed out in a different
reply as you can drop it from the destination XML as it's not guest ABI.
11:25 p.m.
On Mon, Sep 13, 2021 at 4:10 PM Peter Krempa <pkrempa(a)redhat.com> wrote:
On Fri, Sep 10, 2021 at 22:04:01 +0300, Nir Soffer wrote:
> On Fri, Sep 10, 2021 at 4:35 PM Peter Krempa <pkrempa(a)redhat.com> wrote:
> >
> > We don't support all startup policies with all source types so to
> > correctly allow switching from a 'file' based cdrom with
'optional'
> > startup policy to a 'block' based one which doesn't support
optional we
> > must update the startup policy field first. Obviously we need to have
> > fallback if the update fails.
>
> Why is there a difference between file and block for startup policy?
I'm not sure about the historic reasons for why 'block' was not
considered when this was implemented originally.
Hi Peter,
I searched in the commits history and found the error message """
'startupPolicy' is only valid for 'file' type volume """
comes from
43404fee37(
https://listman.redhat.com/archives/libvir-list/2013-April/msg00419.html):
Author: Osier Yang <jyang(a)redhat.com>
Date: Fri Apr 5 03:37:58 2013 +0800
Support startupPolicy for 'volume' disk
"startupPolicy" is only valid for file type storage volume, otherwise
it fails on starting the domain.
But it's not clear why the author said "startupPolicy" is not valid for
block type.
Maybe we can have test on
https://listman.redhat.com/archives/libvir-list/2021-September/msg00903.html
,
to see if it fails to start the domain with startupPolicy and block disk
type.
> Is this documented?
(citation from 'formatdomain.rst' the source for
https://www.libvirt.org/formatdomain.html )
For a "file" or "volume" disk type which represents a cdrom or
floppy
(the
``device`` attribute), it is possible to define policy what to do with
the
disk if the source file is not accessible. (NB, ``startupPolicy`` is not
valid for "volume" disk unless the specified storage volume is of
"file"
type). This is done by the ``startupPolicy`` attribute ( :since:`since
0.9.7`
), accepting these values:
=========
=====================================================================
mandatory fail if missing for any reason (the default)
requisite fail if missing on boot up, drop if missing on
migrate/restore/revert
optional drop if missing at any start attempt
=========
=====================================================================
:since:`Since 1.1.2` the ``startupPolicy`` is extended to support hard
disks
besides cdrom and floppy. On guest cold bootup, if a certain disk is not
accessible or its disk chain is broken, with startupPolicy 'optional'
the
guest will drop this disk. This feature doesn't support migration
currently.
As you can notice only "file" and "volume" (pointing to a file) are
allowed.
> Do we have a way to switch from file to block with current libvirt
> (centos 8 stream, rhel 8.4) without this patch, or do we need to wait
> until this fix is available? (rhel 8.5?)
You can issue two separate update calls. One updating the startup policy
first and then the second one updating the source.
On another note, I don't quite understand though why oVirt actually even
uses startup policy in the first place. Since oVirt is already doing a
pretty complicated storage management, checking whether the image file
exists is a trivial operation.
Similarly, when migrating you can use the destination XML (which can be
optionally used with the migration API) allows you to update storage
source and even provide empty storage for a cdrom. This can be used to
update paths to storage too. This allows superior flexibility when
compared to startup policy.
1150
days inactive
1170
days old
8 comments
5 participants
participants (5)
-
Han Han -
Michal Prívozník -
Nir Soffer -
Peter Krempa -
Vojtech Juranek