[libvirt] [PATCH 0/3] add option to keep nvram file on undefine
There is already a patch [1] on this topic with a different approach - keep nvram file by default. There is also some discussion there. To sum up keeping nvram on undefine could be useful in some usecases so there should be an option to do it. On the other hand there is a danger of leaving domain assets after its undefine and unsing them unintentionally on defining domain with the same name. AFAIU keeping nvram by default was motivated by domain disks behaviour. I think there is a difference as libvirt never create disks for domain as opposed to nvram and managed save and without disks domain will not start so user is quite aware of disks files. On the other hand one can start using nvram file solely putting <nvram> in config and managed save is created on daemon shutdown. So user is much less aware of nvram and managed save existence. Thus one can easily mess up by unaware define $name/using/undefine/define $name again usecase. Thus I vote for keeping said assets only if it is specified explicitly so user knows what he is doing. Adding option to undefine is best solution I come up with. The other options are add checks on define or start and both are impossible. Such a check should be done without any extra flags for it to be useful but this way we break existing users. As this a proof of concept this series does not add extra flag for managed save. [1] https://www.redhat.com/archives/libvir-list/2015-February/msg00915.html Nikolay Shirokovskiy (3): api: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM flag qemu: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM support virsh: add --keep-nvram option to undefine command include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_driver.c | 26 +++++++++++++++++--------- tools/virsh-domain.c | 8 ++++++++ tools/virsh.pod | 6 +++--- 4 files changed, 29 insertions(+), 12 deletions(-) -- 1.8.3.1
This flags specifies to keep nvram file if it is existed for inactive domains. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- include/libvirt/libvirt-domain.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 27ed29a..18cb793 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1572,6 +1572,7 @@ typedef enum { snapshot metadata */ VIR_DOMAIN_UNDEFINE_NVRAM = (1 << 2), /* Also remove any nvram file */ + VIR_DOMAIN_UNDEFINE_KEEP_NVRAM = (1 << 3), /* Keep nvram file */ /* Future undefine control flags should come here. */ } virDomainUndefineFlagsValues; -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 249393a..2f57324 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7341,7 +7341,15 @@ qemuDomainUndefineFlags(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA | - VIR_DOMAIN_UNDEFINE_NVRAM, -1); + VIR_DOMAIN_UNDEFINE_NVRAM | + VIR_DOMAIN_UNDEFINE_KEEP_NVRAM, -1); + + if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM) && + (flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot both keep and delete nvram")); + return -1; + } if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -7393,18 +7401,18 @@ qemuDomainUndefineFlags(virDomainPtr dom, if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && virFileExists(vm->def->os.loader->nvram)) { - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { + if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { + if (unlink(vm->def->os.loader->nvram) < 0) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; + } + } else if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot delete inactive domain with nvram")); goto cleanup; } - - if (unlink(vm->def->os.loader->nvram) < 0) { - virReportSystemError(errno, - _("failed to remove nvram: %s"), - vm->def->os.loader->nvram); - goto cleanup; - } } if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0) -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- tools/virsh-domain.c | 8 ++++++++ tools/virsh.pod | 6 +++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8d7ff61..30922d1 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3604,6 +3604,10 @@ static const vshCmdOptDef opts_undefine[] = { .type = VSH_OT_BOOL, .help = N_("remove nvram file, if inactive") }, + {.name = "keep-nvram", + .type = VSH_OT_BOOL, + .help = N_("keep nvram file, if inactive") + }, {.name = NULL} }; @@ -3629,6 +3633,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) bool remove_all_storage = vshCommandOptBool(cmd, "remove-all-storage"); bool delete_snapshots = vshCommandOptBool(cmd, "delete-snapshots"); bool nvram = vshCommandOptBool(cmd, "nvram"); + bool keep_nvram = vshCommandOptBool(cmd, "keep-nvram"); /* Positive if these items exist. */ int has_managed_save = 0; int has_snapshots_metadata = 0; @@ -3657,6 +3662,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) virshControlPtr priv = ctl->privData; VSH_REQUIRE_OPTION("delete-snapshots", "remove-all-storage"); + VSH_EXCLUSIVE_OPTIONS("nvram", "keep-nvram"); ignore_value(vshCommandOptStringQuiet(ctl, cmd, "storage", &vol_string)); @@ -3680,6 +3686,8 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) } if (nvram) flags |= VIR_DOMAIN_UNDEFINE_NVRAM; + if (keep_nvram) + flags |= VIR_DOMAIN_UNDEFINE_KEEP_NVRAM; if (!(dom = virshCommandOptDomain(ctl, cmd, &name))) return false; diff --git a/tools/virsh.pod b/tools/virsh.pod index 6844823..c2aea4c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2373,7 +2373,7 @@ Output the device used for the TTY console of the domain. If the information is not available the processes will provide an exit code of 1. =item B<undefine> I<domain> [I<--managed-save>] [I<--snapshots-metadata>] -[I<--nvram>] +[I<--nvram>] [I<--keep-nvram>] [ {I<--storage> B<volumes> | I<--remove-all-storage> [I<--delete-snapshots>]} I<--wipe-storage>] @@ -2391,8 +2391,8 @@ domain. Without the flag, attempts to undefine an inactive domain with snapshot metadata will fail. If the domain is active, this flag is ignored. -The I<--nvram> flag ensures no nvram (/domain/os/nvram/) file is -left behind. If the domain has an nvram file and the flag is +I<--nvram> and I<--keep-nvram> specify accordingly to delete or keep nvram +(/domain/os/nvram/) file. If the domain has an nvram file and the flags are omitted, the undefine will fail. The I<--storage> flag takes a parameter B<volumes>, which is a comma separated -- 1.8.3.1
ping On 27.05.2016 11:05, Nikolay Shirokovskiy wrote:
There is already a patch [1] on this topic with a different approach - keep nvram file by default. There is also some discussion there. To sum up keeping nvram on undefine could be useful in some usecases so there should be an option to do it. On the other hand there is a danger of leaving domain assets after its undefine and unsing them unintentionally on defining domain with the same name.
AFAIU keeping nvram by default was motivated by domain disks behaviour. I think there is a difference as libvirt never create disks for domain as opposed to nvram and managed save and without disks domain will not start so user is quite aware of disks files. On the other hand one can start using nvram file solely putting <nvram> in config and managed save is created on daemon shutdown. So user is much less aware of nvram and managed save existence. Thus one can easily mess up by unaware define $name/using/undefine/define $name again usecase. Thus I vote for keeping said assets only if it is specified explicitly so user knows what he is doing.
Adding option to undefine is best solution I come up with. The other options are add checks on define or start and both are impossible. Such a check should be done without any extra flags for it to be useful but this way we break existing users.
As this a proof of concept this series does not add extra flag for managed save.
[1] https://www.redhat.com/archives/libvir-list/2015-February/msg00915.html
Nikolay Shirokovskiy (3): api: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM flag qemu: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM support virsh: add --keep-nvram option to undefine command
include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_driver.c | 26 +++++++++++++++++--------- tools/virsh-domain.c | 8 ++++++++ tools/virsh.pod | 6 +++--- 4 files changed, 29 insertions(+), 12 deletions(-)
ping On 27.05.2016 11:05, Nikolay Shirokovskiy wrote:
There is already a patch [1] on this topic with a different approach - keep nvram file by default. There is also some discussion there. To sum up keeping nvram on undefine could be useful in some usecases so there should be an option to do it. On the other hand there is a danger of leaving domain assets after its undefine and unsing them unintentionally on defining domain with the same name.
AFAIU keeping nvram by default was motivated by domain disks behaviour. I think there is a difference as libvirt never create disks for domain as opposed to nvram and managed save and without disks domain will not start so user is quite aware of disks files. On the other hand one can start using nvram file solely putting <nvram> in config and managed save is created on daemon shutdown. So user is much less aware of nvram and managed save existence. Thus one can easily mess up by unaware define $name/using/undefine/define $name again usecase. Thus I vote for keeping said assets only if it is specified explicitly so user knows what he is doing.
Adding option to undefine is best solution I come up with. The other options are add checks on define or start and both are impossible. Such a check should be done without any extra flags for it to be useful but this way we break existing users.
As this a proof of concept this series does not add extra flag for managed save.
[1] https://www.redhat.com/archives/libvir-list/2015-February/msg00915.html
Nikolay Shirokovskiy (3): api: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM flag qemu: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM support virsh: add --keep-nvram option to undefine command
include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_driver.c | 26 +++++++++++++++++--------- tools/virsh-domain.c | 8 ++++++++ tools/virsh.pod | 6 +++--- 4 files changed, 29 insertions(+), 12 deletions(-)
27-May-16 11:05, Nikolay Shirokovskiy пишет:
There is already a patch [1] on this topic with a different approach - keep nvram file by default. There is also some discussion there. To sum up keeping nvram on undefine could be useful in some usecases so there should be an option to do it. On the other hand there is a danger of leaving domain assets after its undefine and unsing them unintentionally on defining domain with the same name.
AFAIU keeping nvram by default was motivated by domain disks behaviour. I think there is a difference as libvirt never create disks for domain as opposed to nvram and managed save and without disks domain will not start so user is quite aware of disks files. On the other hand one can start using nvram file solely putting <nvram> in config and managed save is created on daemon shutdown. So user is much less aware of nvram and managed save existence. Thus one can easily mess up by unaware define $name/using/undefine/define $name again usecase. Thus I vote for keeping said assets only if it is specified explicitly so user knows what he is doing.
Adding option to undefine is best solution I come up with. The other options are add checks on define or start and both are impossible. Such a check should be done without any extra flags for it to be useful but this way we break existing users.
As this a proof of concept this series does not add extra flag for managed save.
[1] https://www.redhat.com/archives/libvir-list/2015-February/msg00915.html
I'm OK with your approach. ACK from me. Any other opinion? Maxim
On 27.05.2016 10:05, Nikolay Shirokovskiy wrote:
There is already a patch [1] on this topic with a different approach - keep nvram file by default. There is also some discussion there. To sum up keeping nvram on undefine could be useful in some usecases so there should be an option to do it. On the other hand there is a danger of leaving domain assets after its undefine and unsing them unintentionally on defining domain with the same name.
AFAIU keeping nvram by default was motivated by domain disks behaviour. I think there is a difference as libvirt never create disks for domain as opposed to nvram and managed save and without disks domain will not start so user is quite aware of disks files. On the other hand one can start using nvram file solely putting <nvram> in config and managed save is created on daemon shutdown. So user is much less aware of nvram and managed save existence. Thus one can easily mess up by unaware define $name/using/undefine/define $name again usecase. Thus I vote for keeping said assets only if it is specified explicitly so user knows what he is doing.
Adding option to undefine is best solution I come up with. The other options are add checks on define or start and both are impossible. Such a check should be done without any extra flags for it to be useful but this way we break existing users.
As this a proof of concept this series does not add extra flag for managed save.
[1] https://www.redhat.com/archives/libvir-list/2015-February/msg00915.html
Nikolay Shirokovskiy (3): api: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM flag qemu: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM support virsh: add --keep-nvram option to undefine command
include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_driver.c | 26 +++++++++++++++++--------- tools/virsh-domain.c | 8 ++++++++ tools/virsh.pod | 6 +++--- 4 files changed, 29 insertions(+), 12 deletions(-)
The patches look good to me (although the commit messages could be a little more verbose, e.g. explain why we need to have X and !X flags at the same time). I've went trough the discussion you linked and even though I usually agree with Dan, this time I think we must not change the behaviour of undefine and thus need a special flag for keeping NVRAM. ACK, but we might want to give him (or anybody else) chance to chime in, so please postpone the push for a while. Thanks! Michal
On Mon, 2016-09-12 at 10:20 +0200, Michal Privoznik wrote:
There is already a patch [1] on this topic with a different approach - keep nvram file by default. There is also some discussion there. To sum up keeping nvram on undefine could be useful in some usecases so there should be an option to do it. On the other hand there is a danger of leaving domain assets after its undefine and unsing them unintentionally on defining domain with the same name. AFAIU keeping nvram by default was motivated by domain disks behaviour. I think there is a difference as libvirt never create disks for domain as opposed to nvram and managed save and without disks domain will not start so user is quite aware of disks files. On the other hand one can start using nvram file solely putting <nvram> in config and managed save is created on daemon shutdown. So user is much less aware of nvram and managed save existence. Thus one can easily mess up by unaware define $name/using/undefine/define $name again usecase. Thus I vote for keeping said assets only if it is specified explicitly so user knows what he is doing. Adding option to undefine is best solution I come up with. The other options are add checks on define or start and both are impossible. Such a check should be done without any extra flags for it to be useful but this way we break existing users. As this a proof of concept this series does not add extra flag for managed save. [1] https://www.redhat.com/archives/libvir-list/2015-February/msg00915.html Nikolay Shirokovskiy (3): api: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM flag qemu: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM support virsh: add --keep-nvram option to undefine command include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_driver.c | 26 +++++++++++++++++--------- tools/virsh-domain.c | 8 ++++++++ tools/virsh.pod | 6 +++--- 4 files changed, 29 insertions(+), 12 deletions(-) The patches look good to me (although the commit messages could be a
On 27.05.2016 10:05, Nikolay Shirokovskiy wrote: little more verbose, e.g. explain why we need to have X and !X flags at the same time). I've went trough the discussion you linked and even though I usually agree with Dan, this time I think we must not change the behaviour of undefine and thus need a special flag for keeping NVRAM. ACK, but we might want to give him (or anybody else) chance to chime in, so please postpone the push for a while. Thanks!
I thought I would disagree with this approach, but after reading both the original thread and the current one again I've actually changed my mind :) It's very unfortunate that we have to patch stuff up this way, long after the fact, and it's even more unfortunate that the existing flag and virsh command line option have not been named something like REMOVE_NVRAM. Oh well. At least this solution, while a bit unconvenient for the user, will avoid any change in behavior. Still, I'd like to have Dan's opinion before merging this, since he did not like this approach a year or so ago. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Sep 12, 2016 at 07:01:38PM +0200, Andrea Bolognani wrote:
On Mon, 2016-09-12 at 10:20 +0200, Michal Privoznik wrote:
There is already a patch [1] on this topic with a different approach - keep nvram file by default. There is also some discussion there. To sum up keeping nvram on undefine could be useful in some usecases so there should be an option to do it. On the other hand there is a danger of leaving domain assets after its undefine and unsing them unintentionally on defining domain with the same name. AFAIU keeping nvram by default was motivated by domain disks behaviour. I think there is a difference as libvirt never create disks for domain as opposed to nvram and managed save and without disks domain will not start so user is quite aware of disks files. On the other hand one can start using nvram file solely putting <nvram> in config and managed save is created on daemon shutdown. So user is much less aware of nvram and managed save existence. Thus one can easily mess up by unaware define $name/using/undefine/define $name again usecase. Thus I vote for keeping said assets only if it is specified explicitly so user knows what he is doing. Adding option to undefine is best solution I come up with. The other options are add checks on define or start and both are impossible. Such a check should be done without any extra flags for it to be useful but this way we break existing users. As this a proof of concept this series does not add extra flag for managed save. [1] https://www.redhat.com/archives/libvir-list/2015-February/msg00915.html Nikolay Shirokovskiy (3): api: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM flag qemu: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM support virsh: add --keep-nvram option to undefine command include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_driver.c | 26 +++++++++++++++++--------- tools/virsh-domain.c | 8 ++++++++ tools/virsh.pod | 6 +++--- 4 files changed, 29 insertions(+), 12 deletions(-) The patches look good to me (although the commit messages could be a
On 27.05.2016 10:05, Nikolay Shirokovskiy wrote: little more verbose, e.g. explain why we need to have X and !X flags at the same time). I've went trough the discussion you linked and even though I usually agree with Dan, this time I think we must not change the behaviour of undefine and thus need a special flag for keeping NVRAM. ACK, but we might want to give him (or anybody else) chance to chime in, so please postpone the push for a while. Thanks!
I thought I would disagree with this approach, but after reading both the original thread and the current one again I've actually changed my mind :)
It's very unfortunate that we have to patch stuff up this way, long after the fact, and it's even more unfortunate that the existing flag and virsh command line option have not been named something like REMOVE_NVRAM. Oh well. At least this solution, while a bit unconvenient for the user, will avoid any change in behavior.
Still, I'd like to have Dan's opinion before merging this, since he did not like this approach a year or so ago.
No objection from me. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12.09.2016 10:20, Michal Privoznik wrote:
On 27.05.2016 10:05, Nikolay Shirokovskiy wrote:
There is already a patch [1] on this topic with a different approach - keep nvram file by default. There is also some discussion there. To sum up keeping nvram on undefine could be useful in some usecases so there should be an option to do it. On the other hand there is a danger of leaving domain assets after its undefine and unsing them unintentionally on defining domain with the same name.
AFAIU keeping nvram by default was motivated by domain disks behaviour. I think there is a difference as libvirt never create disks for domain as opposed to nvram and managed save and without disks domain will not start so user is quite aware of disks files. On the other hand one can start using nvram file solely putting <nvram> in config and managed save is created on daemon shutdown. So user is much less aware of nvram and managed save existence. Thus one can easily mess up by unaware define $name/using/undefine/define $name again usecase. Thus I vote for keeping said assets only if it is specified explicitly so user knows what he is doing.
Adding option to undefine is best solution I come up with. The other options are add checks on define or start and both are impossible. Such a check should be done without any extra flags for it to be useful but this way we break existing users.
As this a proof of concept this series does not add extra flag for managed save.
[1] https://www.redhat.com/archives/libvir-list/2015-February/msg00915.html
Nikolay Shirokovskiy (3): api: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM flag qemu: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM support virsh: add --keep-nvram option to undefine command
include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_driver.c | 26 +++++++++++++++++--------- tools/virsh-domain.c | 8 ++++++++ tools/virsh.pod | 6 +++--- 4 files changed, 29 insertions(+), 12 deletions(-)
The patches look good to me (although the commit messages could be a little more verbose, e.g. explain why we need to have X and !X flags at the same time). I've went trough the discussion you linked and even though I usually agree with Dan, this time I think we must not change the behaviour of undefine and thus need a special flag for keeping NVRAM.
ACK, but we might want to give him (or anybody else) chance to chime in, so please postpone the push for a while. Thanks!
Okay, this is pushed now. Thank you! Michal
participants (5)
-
Andrea Bolognani -
Daniel P. Berrange -
Maxim Nestratov -
Michal Privoznik -
Nikolay Shirokovskiy