Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U
On Wed, Jan 10, 2018 at 04:43:22PM +, Nir Soffer wrote: > On Wed, Jan 10, 2018 at 4:04 PM Kashyap Chamarthy > wrote: [...] > > Yes, for completness' sake, Nova upstream is already patched to use the > > `qemu-img` '--force-share' flag that comes with QEMU >= 2.10. > > > > What abut users running released Nova, upgrading to 2.10? There are three stable Nova branches at any time, in various phases. It is already backported to the current newest stable Nova branch (where all bug fixes that meet certain criteria are accepted). Then there are two other stable releases. One is in "Only critical bugfixes and security patches" phase; the other is in "Only security patches" phase. So this change wasn't backported there, and users would be using QEMU 2.9 or below with those two versions of Nova. -- /kashyap
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U
On Wed, Jan 10, 2018 at 4:04 PM Kashyap Chamarthy wrote: > On Mon, Jan 08, 2018 at 03:41:36PM +0100, Kevin Wolf wrote: > > Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben: > > > Management and users are accustomed to "qemu-img info" to query status > of > > > images even when they are used by guests. Since image locking was > added, the -U > > > (--force-share) option is needed for that to work. The reason has been > that due > > > to possible race with image header update, the output can be > misleading. > > > > > > But what are likely to happen after we emit the error are that, for > interactive > > > users, '-U' will be used and the command retried; for management > (nova, RHV, > > > etc.), the operation is broken with no knob to workaround this. > > > > > > This series changes that error to a warning so that it doesn't get in > the way. > > > > Are management tools actually doing this? There is no good reason to > > call 'qemu-img info' for an image that is in use by a VM. > > Yes, Nova does use 'qemu-img info' in a few places (haven't audited them > all) for a running guest. From a quick look at the code, at-least > during Nova's live snaphot (as part of libvirt's "shallow rebase") it is > used. > > > If no, NACK. Automatically disabling locking because it can be > > inconvenient defeats the purpose of locking. > > > > If yes, clearly indicate that this usage is deprecated and we'll turn > > this into an error again with 2.13. Then management tools can be fixed > > in time. > > Yes, for completness' sake, Nova upstream is already patched to use the > `qemu-img` '--force-share' flag that comes with QEMU >= 2.10. > What abut users running released Nova, upgrading to 2.10? Nir
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U
On Mon, Jan 08, 2018 at 06:57:29PM +0100, Kevin Wolf wrote: > Am 08.01.2018 um 18:07 hat Nir Soffer geschrieben: > > On Mon, Jan 8, 2018 at 4:48 PM Kevin Wolf wrote: > > > > > Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben: > > > > Management and users are accustomed to "qemu-img info" to query status > > > > of > > > > images even when they are used by guests. Since image locking was added, > > > the -U > > > > (--force-share) option is needed for that to work. The reason has been > > > that due > > > > to possible race with image header update, the output can be misleading. > > > > > > > > But what are likely to happen after we emit the error are that, for > > > interactive > > > > users, '-U' will be used and the command retried; for management (nova, > > > RHV, > > > > etc.), the operation is broken with no knob to workaround this. > > > > > > > > This series changes that error to a warning so that it doesn't get in > > > the way. > > > > > > Are management tools actually doing this? There is no good reason to > > > call 'qemu-img info' for an image that is in use by a VM. > > > > > > > Yes, ovirt/RHV is using this to get the qcow2 compat of an image since 4.1. > > > > We asked about this here and in private mail, and there was an > > agreement that using qemu-img info on an image is safe for this > > purpose. We know that accessing image header when a guest is using is > > may be racy, but we control both the guest and the image. Nobody is > > modifying the image properties behind our back. > > Yes, it's probably safe enough, though it's clearly a case for -U rather > than allowing it unconditionally. > > > In 4.2 we will use the new flags[1], but we cannot fix released code. > > Introducing this locking in qemu-img info will break existing > > installations. > > We should have the proper deprecation process and printed a warning for > two releases. Anyway, it's too late for this and now we've already had > two releases where this was a hard error. > > I'm not sure if going back to the old behaviour for a while now would be > helpful, you'd just end up with an even more confusing set of qemu > versions, for example: > > <= 2.9 - works without a warning > 2.10 and 2.11 - errors out > 2.12- prints a warning, but works > >= 2.13 - errors out again I think this is really undesirable to have 2.12 suddenly behave diferently after 2 releases of having this change. Apps are pretty much forced to take the change to use -U regardless because 2.10 & 2.11 are already in the wild. Printing a warning would only be useful if we had done it in 2.10. Now it is too late to be useful and will just confuse life even more. So IMHO just drop this patch. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U
On Tue, Jan 9, 2018 at 10:11 PM, Eric Blake wrote: > On 01/09/2018 01:58 PM, Ala Hino wrote: > > >>> I know this is debatable but I think the #1 purpose of image locking is > >> to > >>> prevent data corruption; > >> > >> Isn't potentially wrong output of 'qemu-img info' a form of data > >> corruption? Not data as in disk content, but metadata is data, too. > >> > >>> #2 IMO is to reduce confusion and misinformation. > >>> While inconsistent output of "qemu-img info" is misinformation, it not > >> working > >>> as before is actually confusion. Though the current behavior is indeed > >> ideal, > >>> the proposed patch is a bit more pragmatical. > >> > >> To be clear: If we want to minimise confusing by change, we have to > >> disable locking by default, not only here but everywhere else. And > >> therefore make it completely useless. > >> > >> The whole point of locking is to change something in order to make > >> things safer. Yes, this may inconvenience users who want to do something > >> unsafe. Tough luck. The only other option is not making things safer. > >> > > > > Safer is better for sure. > > It is not about doing something unsafe, it is about breaking a released > > version - > > RHV 4.1 is already out and when customers upgrade to RHEL 7.5, they will > not > > be able to create snapshots. > > I see two options: > > 1. As mentioned by Fam, settle on warning for good > > Which is a downgrade in qemu behavior, since we've already had releases > where it was an error (and if you have to worry about THOSE qemu > releases, then the warning doesn't buy you anything). > We test our code on RHEL, and currently require qemu-kvm-rhev >= 2.6.0. On RHEL 7.4 qemu-kvm-rhev version is 2.9.0 - and everything works good. However, on RHEL 7.5 the version is 2.10.0, which breaks RHV 4.1. If we got qemu-kvm-rhev 2.10.0 in RHEL 7.4 repositories, we could detect the failure earlier and maybe we would be able to adapt the code before releasing 4.1. > > > 2. Conflict qemu with vdsm < 4.2 > > If I'm understanding this option correctly, you are proposing that the > distribution is set up so that you have to upgrade vdsm prior to being > able to upgrade to newer qemu that enables locking (so you can use old > vdsm, old qemu; new vdsm, old qemu; new vdsm, new qemu; but you get > rejected on attempts to use old vdsm, new qemu). That's outside the > scope of qemu proper and falls instead into the distro's packaging > scheme, but sounds like a better alternative than trying to fix new qemu > to work around an issue that released qemu already has, all on behalf of > vdsm where old vdsm plays nice with old qemu, and where new vdsm already > knows how to play nice with both flavors of qemu. > I agree that this might be the better option. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > >
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U
On 01/09/2018 01:58 PM, Ala Hino wrote: >>> I know this is debatable but I think the #1 purpose of image locking is >> to >>> prevent data corruption; >> >> Isn't potentially wrong output of 'qemu-img info' a form of data >> corruption? Not data as in disk content, but metadata is data, too. >> >>> #2 IMO is to reduce confusion and misinformation. >>> While inconsistent output of "qemu-img info" is misinformation, it not >> working >>> as before is actually confusion. Though the current behavior is indeed >> ideal, >>> the proposed patch is a bit more pragmatical. >> >> To be clear: If we want to minimise confusing by change, we have to >> disable locking by default, not only here but everywhere else. And >> therefore make it completely useless. >> >> The whole point of locking is to change something in order to make >> things safer. Yes, this may inconvenience users who want to do something >> unsafe. Tough luck. The only other option is not making things safer. >> > > Safer is better for sure. > It is not about doing something unsafe, it is about breaking a released > version - > RHV 4.1 is already out and when customers upgrade to RHEL 7.5, they will not > be able to create snapshots. > I see two options: > 1. As mentioned by Fam, settle on warning for good Which is a downgrade in qemu behavior, since we've already had releases where it was an error (and if you have to worry about THOSE qemu releases, then the warning doesn't buy you anything). > 2. Conflict qemu with vdsm < 4.2 If I'm understanding this option correctly, you are proposing that the distribution is set up so that you have to upgrade vdsm prior to being able to upgrade to newer qemu that enables locking (so you can use old vdsm, old qemu; new vdsm, old qemu; new vdsm, new qemu; but you get rejected on attempts to use old vdsm, new qemu). That's outside the scope of qemu proper and falls instead into the distro's packaging scheme, but sounds like a better alternative than trying to fix new qemu to work around an issue that released qemu already has, all on behalf of vdsm where old vdsm plays nice with old qemu, and where new vdsm already knows how to play nice with both flavors of qemu. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U
On Tue, Jan 9, 2018 at 11:58 AM, Kevin Wolf wrote: > Am 09.01.2018 um 07:24 hat Fam Zheng geschrieben: > > On Mon, 01/08 18:57, Kevin Wolf wrote: > > > I'm not sure if going back to the old behaviour for a while now would > be > > > helpful, you'd just end up with an even more confusing set of qemu > > > versions, for example: > > > > > > <= 2.9 - works without a warning > > > 2.10 and 2.11 - errors out > > > 2.12- prints a warning, but works > > > >= 2.13 - errors out again > > > > What I had in mind is settle on warning for good. QEMU (including > qemu-img) is a > > low level tool that can be used in many ways that it isn't supposed to, > this one > > is not more harmful than others (e.g. "qemu-img snapshot ..." on > iscsi:// qcow2 > > image) we allow siliently. > > > > I know this is debatable but I think the #1 purpose of image locking is > to > > prevent data corruption; > > Isn't potentially wrong output of 'qemu-img info' a form of data > corruption? Not data as in disk content, but metadata is data, too. > > > #2 IMO is to reduce confusion and misinformation. > > While inconsistent output of "qemu-img info" is misinformation, it not > working > > as before is actually confusion. Though the current behavior is indeed > ideal, > > the proposed patch is a bit more pragmatical. > > To be clear: If we want to minimise confusing by change, we have to > disable locking by default, not only here but everywhere else. And > therefore make it completely useless. > > The whole point of locking is to change something in order to make > things safer. Yes, this may inconvenience users who want to do something > unsafe. Tough luck. The only other option is not making things safer. > Safer is better for sure. It is not about doing something unsafe, it is about breaking a released version - RHV 4.1 is already out and when customers upgrade to RHEL 7.5, they will not be able to create snapshots. I see two options: 1. As mentioned by Fam, settle on warning for good 2. Conflict qemu with vdsm < 4.2 > > We already successfully made a point that tools (libvirt with shared > images, or libguestfs) need to update their qemu invocation for qemu > proper. They didn't like it, but they could see the point, and it has > been this way for two releases already. So I don't see a compelling > reason why now we should give up again some of the safety we achieved > long-term. > > If we were designing qemu-img from scratch, it would be an error. So > that's what it should be in the long term. Tools should preferably use > 'query-block' and friends rather 'qemu-img info' if the image is in use. > > Kevin >
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U
Am 09.01.2018 um 07:24 hat Fam Zheng geschrieben: > On Mon, 01/08 18:57, Kevin Wolf wrote: > > I'm not sure if going back to the old behaviour for a while now would be > > helpful, you'd just end up with an even more confusing set of qemu > > versions, for example: > > > > <= 2.9 - works without a warning > > 2.10 and 2.11 - errors out > > 2.12- prints a warning, but works > > >= 2.13 - errors out again > > What I had in mind is settle on warning for good. QEMU (including qemu-img) > is a > low level tool that can be used in many ways that it isn't supposed to, this > one > is not more harmful than others (e.g. "qemu-img snapshot ..." on iscsi:// > qcow2 > image) we allow siliently. > > I know this is debatable but I think the #1 purpose of image locking is to > prevent data corruption; Isn't potentially wrong output of 'qemu-img info' a form of data corruption? Not data as in disk content, but metadata is data, too. > #2 IMO is to reduce confusion and misinformation. > While inconsistent output of "qemu-img info" is misinformation, it not working > as before is actually confusion. Though the current behavior is indeed ideal, > the proposed patch is a bit more pragmatical. To be clear: If we want to minimise confusing by change, we have to disable locking by default, not only here but everywhere else. And therefore make it completely useless. The whole point of locking is to change something in order to make things safer. Yes, this may inconvenience users who want to do something unsafe. Tough luck. The only other option is not making things safer. We already successfully made a point that tools (libvirt with shared images, or libguestfs) need to update their qemu invocation for qemu proper. They didn't like it, but they could see the point, and it has been this way for two releases already. So I don't see a compelling reason why now we should give up again some of the safety we achieved long-term. If we were designing qemu-img from scratch, it would be an error. So that's what it should be in the long term. Tools should preferably use 'query-block' and friends rather 'qemu-img info' if the image is in use. Kevin
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U
On Mon, 01/08 18:57, Kevin Wolf wrote: > I'm not sure if going back to the old behaviour for a while now would be > helpful, you'd just end up with an even more confusing set of qemu > versions, for example: > > <= 2.9 - works without a warning > 2.10 and 2.11 - errors out > 2.12- prints a warning, but works > >= 2.13 - errors out again What I had in mind is settle on warning for good. QEMU (including qemu-img) is a low level tool that can be used in many ways that it isn't supposed to, this one is not more harmful than others (e.g. "qemu-img snapshot ..." on iscsi:// qcow2 image) we allow siliently. I know this is debatable but I think the #1 purpose of image locking is to prevent data corruption; #2 IMO is to reduce confusion and misinformation. While inconsistent output of "qemu-img info" is misinformation, it not working as before is actually confusion. Though the current behavior is indeed ideal, the proposed patch is a bit more pragmatical. Fam
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U
Am 08.01.2018 um 18:07 hat Nir Soffer geschrieben: > On Mon, Jan 8, 2018 at 4:48 PM Kevin Wolf wrote: > > > Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben: > > > Management and users are accustomed to "qemu-img info" to query status of > > > images even when they are used by guests. Since image locking was added, > > the -U > > > (--force-share) option is needed for that to work. The reason has been > > that due > > > to possible race with image header update, the output can be misleading. > > > > > > But what are likely to happen after we emit the error are that, for > > interactive > > > users, '-U' will be used and the command retried; for management (nova, > > RHV, > > > etc.), the operation is broken with no knob to workaround this. > > > > > > This series changes that error to a warning so that it doesn't get in > > the way. > > > > Are management tools actually doing this? There is no good reason to > > call 'qemu-img info' for an image that is in use by a VM. > > > > Yes, ovirt/RHV is using this to get the qcow2 compat of an image since 4.1. > > We asked about this here and in private mail, and there was an > agreement that using qemu-img info on an image is safe for this > purpose. We know that accessing image header when a guest is using is > may be racy, but we control both the guest and the image. Nobody is > modifying the image properties behind our back. Yes, it's probably safe enough, though it's clearly a case for -U rather than allowing it unconditionally. > In 4.2 we will use the new flags[1], but we cannot fix released code. > Introducing this locking in qemu-img info will break existing > installations. We should have the proper deprecation process and printed a warning for two releases. Anyway, it's too late for this and now we've already had two releases where this was a hard error. I'm not sure if going back to the old behaviour for a while now would be helpful, you'd just end up with an even more confusing set of qemu versions, for example: <= 2.9 - works without a warning 2.10 and 2.11 - errors out 2.12- prints a warning, but works >= 2.13 - errors out again Is it expected that you use old oVirt versions with newer qemu versions? If so, why didn't we already notice this with the 2.10 release? Basically I think it's reasonable to expect that new qemu versions may require new management versions, too. We try to stay compatible, but sometimes that just doesn't quite work out as we'd like. > > If no, NACK. Automatically disabling locking because it can be > > inconvenient defeats the purpose of locking. > > > > If yes, clearly indicate that this usage is deprecated and we'll turn > > this into an error again with 2.13. Then management tools can be fixed > > in time. > > > This will work for us in general, but I'm not sure that when 2.13 will > be released, no user will run code assuming the previous behavior. It > will be best to wait with incompatible changes like this to next major > version. qemu doesn't currently use a versioning schema that works like that. Our deprecation policy is to print a warning for two releases before the feature is removed or changed incompatibly. We messed this up this time, but the two releases after the change are already over, so chances are that nobody would have noticed the problem in time even if we hadn't messed up. Kevin
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U
On Mon, Jan 8, 2018 at 4:48 PM Kevin Wolf wrote: > Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben: > > Management and users are accustomed to "qemu-img info" to query status of > > images even when they are used by guests. Since image locking was added, > the -U > > (--force-share) option is needed for that to work. The reason has been > that due > > to possible race with image header update, the output can be misleading. > > > > But what are likely to happen after we emit the error are that, for > interactive > > users, '-U' will be used and the command retried; for management (nova, > RHV, > > etc.), the operation is broken with no knob to workaround this. > > > > This series changes that error to a warning so that it doesn't get in > the way. > > Are management tools actually doing this? There is no good reason to > call 'qemu-img info' for an image that is in use by a VM. > Yes, ovirt/RHV is using this to get the qcow2 compat of an image since 4.1. We asked about this here and in private mail, and there was an agreement that using qemu-img info on an image is safe for this purpose. We know that accessing image header when a guest is using is may be racy, but we control both the guest and the image. Nobody is modifying the image properties behind our back. In 4.2 we will use the new flags[1], but we cannot fix released code. Introducing this locking in qemu-img info will break existing installations. > If no, NACK. Automatically disabling locking because it can be > inconvenient defeats the purpose of locking. > > If yes, clearly indicate that this usage is deprecated and we'll turn > this into an error again with 2.13. Then management tools can be fixed > in time. This will work for us in general, but I'm not sure that when 2.13 will be released, no user will run code assuming the previous behavior. It will be best to wait with incompatible changes like this to next major version. [1] https://gerrit.ovirt.org/85874/ Nir
