[virt-tools-list] [virt-manager PATCH 1/2] domain: add support to rename domain with nvram vars file

[Pavel Hrdina]

On Mon, Mar 06, 2017 at 06:48:34PM -0500, Cole Robinson wrote:
> On 03/06/2017 03:43 AM, Pavel Hrdina wrote:
> > Libvirt storage API doesn't support renaming storage volumes so
> > we need to copy the nvram file and remove the old one.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1368922
> > 
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > ---
> >  virtManager/details.py |  3 ++-
> >  virtManager/domain.py  | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 48 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virtManager/details.py b/virtManager/details.py
> > index d6fef967..0b2754d8 100644
> > --- a/virtManager/details.py
> > +++ b/virtManager/details.py
> > @@ -1975,7 +1975,8 @@ class vmmDetails(vmmGObjectUI):
> >          # This needs to be last
> >          if self.edited(EDIT_NAME):
> >              # Renaming is pretty convoluted, so do it here synchronously
> > -            self.vm.define_name(self.widget("overview-name").get_text())
> > +            self.vm.rename_domain(self.widget("overview-name").get_text(),
> > +                                  kwargs)
> >  
> >              if not kwargs and not hotplug_args:
> >                  # Saves some useless redefine attempts
> > diff --git a/virtManager/domain.py b/virtManager/domain.py
> > index 336584e5..79303890 100644
> > --- a/virtManager/domain.py
> > +++ b/virtManager/domain.py
> > @@ -32,6 +32,7 @@ from virtinst import DomainSnapshot
> >  from virtinst import Guest
> >  from virtinst import util
> >  from virtinst import VirtualController
> > +from virtinst import VirtualDisk
> >  
> >  from .libvirtobject import vmmLibvirtObject
> >  
> > @@ -557,6 +558,42 @@ class vmmDomain(vmmLibvirtObject):
> >      # Persistent XML change APIs #
> >      ##############################
> >  
> > +    def rename_domain(self, new_name, kwargs):
> > +        old_name = self.get_name()
> > +
> > +        self.define_name(new_name)
> > +
> > +        # We need to do this copy magic because there is no Libvirt storage API
> > +        # to rename storage volume.
> > +        try:
> > +            old_nvram = VirtualDisk(self.conn.get_backend())
> > +            old_nvram.path = self.get_xmlobj().os.nvram
> > +
> > +            nvram_dir = os.path.dirname(old_nvram.path)
> > +            new_nvram_path = os.path.join(nvram_dir, "%s_VARS.fd" % new_name)
> > +
> > +            new_nvram = VirtualDisk(self.conn.get_backend())
> > +            new_nvram.path = new_nvram_path
> > +
> > +            nvram_install = VirtualDisk.build_vol_install(
> > +                    self.conn.get_backend(), os.path.basename(new_nvram.path),
> > +                    new_nvram.get_parent_pool(), new_nvram.get_size(), False)
> > +            nvram_install.input_vol = old_nvram.get_vol_object()
> > +            nvram_install.sync_input_vol(only_format=True)
> > +
> > +            new_nvram.set_vol_install(nvram_install)
> > +            new_nvram.validate()
> > +            new_nvram.setup()
> > +        except Exception as e:
> > +            self.define_name(old_name)
> > +            raise RuntimeError("Cannot rename nvram VARS: '%s'" % e)
> 
> This breaks when the VM doesn't have any nvram. virt-manager --connect
> test:///default  and try renaming the 'test' VM.
> 
> Please break all the nvram copying out into a separate function, for code clarity
> 
> I think the ordering here should be:
> * If the VM has NVRAM, try to copy it. If it fails, exit.
> * Rename the VM. If it fails, try to delete the new NVRAM file, but don't
> overwrite the error
> * Delete the original NVRAM file. If it fails, raise the error but don't try
> to rollback the rename
> 
> > +
> > +        kwargs["nvram"] = new_nvram_path
> > +        try:
> > +            old_nvram.get_vol_object().delete(0)
> > +        except Exception as e:
> > +            logging.debug("old nvram file was not removed: '%s'", e)
> > +
> >      # Device Add/Remove
> >      def add_device(self, devobj):
> >          """
> > @@ -621,7 +658,8 @@ class vmmDomain(vmmLibvirtObject):
> >          self._redefine_xmlobj(guest)
> >  
> >      def define_overview(self, machine=_SENTINEL, description=_SENTINEL,
> > -        title=_SENTINEL, idmap_list=_SENTINEL, loader=_SENTINEL):
> > +        title=_SENTINEL, idmap_list=_SENTINEL, loader=_SENTINEL,
> > +        nvram=_SENTINEL):
> >          guest = self._make_xmlobj_to_define()
> >          if machine != _SENTINEL:
> >              guest.os.machine = machine
> > @@ -644,6 +682,9 @@ class vmmDomain(vmmLibvirtObject):
> >                  guest.os.loader_type = "pflash"
> >                  guest.os.loader_ro = True
> >  
> > +        if nvram != _SENTINEL and guest.os.loader is not None:
> > +            guest.os.nvram = nvram
> > +
> >          if idmap_list != _SENTINEL:
> >              if idmap_list is not None:
> >                  # pylint: disable=unpacking-non-sequence
> > @@ -1434,6 +1475,10 @@ class vmmDomain(vmmLibvirtObject):
> >              if (self.get_xmlobj().os.loader_ro is True and
> >                  self.get_xmlobj().os.loader_type == "pflash"):
> >                  flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_NVRAM", 0)
> > +        else:
> > +            if (self.get_xmlobj().os.loader_ro is True and
> > +                self.get_xmlobj().os.loader_type == "pflash"):
> > +                flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_KEEP_NVRAM", 0)
> 
> Can you break out the duplicate loader check into a variable has_nvram or similar
> 
> Also can you add a comment here that the force=False path is used when
> renaming the VM, and KEEP_NVRAM is required for that case. It wasn't obvious to me

Thanks for review, I'll send v2.  This patch was not finished and apparently
I forgot to write it into the commit message for myself to not post it.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20170307/5b7fc3d9/attachment.sig>