[et-mgmt-tools] [PATCH] Resend: Add a --remove flag to allow virt-image to install new imageswhen an existing one with the same name is defined

[Cole Robinson]

Bryan Kearney wrote:
> 
> Cole Robinson wrote:
>> "Bryan M. Kearney <bkearney at redhat.com>"@redhat.com wrote:
>>> # HG changeset patch
>>> # User "Bryan M. Kearney <bkearney at redhat.com>"
>>> # Date 1215006150 14400
>>> # Node ID cada7b0a146fb7b4c187a9043ac2a33d2350268b
>>> # Parent  5d9cf6c2662494d21d84fbafbc0b7980096b1998
>>> Add a --remove flag to allow virt-image to install new images when an existing one with the same name is defined
>>>
>>> diff -r 5d9cf6c26624 -r cada7b0a146f virt-image
>>> --- a/virt-image	Wed Jul 02 10:50:05 2008 +0100
>>> +++ b/virt-image	Wed Jul 02 09:42:30 2008 -0400
>>> @@ -142,7 +142,10 @@ def parse_args():
>>>                        help=_("The zero-based index of the boot record to use"))
>>>      parser.add_option("", "--force", action="store_true", dest="force",
>>>                        help=_("Do not prompt for input. Answers yes where applicable, terminates for all other prompts"),
>>> -                      default=False)
>>> +                      default=False)    
>>> +    parser.add_option("", "--replace",action="store_true", dest="replace",
>>> +                      help=_("Overwrite, or destroy, an existing image with the same name"),
>>> +                      default=False)                      
>>>  
>>>      (options,args) = parser.parse_args()
>>>      if len(args) < 1:
>>> @@ -211,7 +214,7 @@ def main():
>>>          try:
>>>              print _("\n\nCreating guest %s...") % guest.name
>>>  
>>> -            dom = guest.start_install(None, progresscb)
>>> +            dom = guest.start_install(None, progresscb, options.replace)
>>>              if dom is None:
>>>                  print _("Guest creation failed")
>>>                  sys.exit(1)
>>> diff -r 5d9cf6c26624 -r cada7b0a146f virtinst/Guest.py
>>> --- a/virtinst/Guest.py	Wed Jul 02 10:50:05 2008 +0100
>>> +++ b/virtinst/Guest.py	Wed Jul 02 09:42:30 2008 -0400
>>> @@ -914,7 +914,7 @@ class Guest(object):
>>>          "action": action }
>>>  
>>>  
>>> -    def start_install(self, consolecb = None, meter = None):
>>> +    def start_install(self, consolecb = None, meter = None,  removeOld = False):
>>>          """Do the startup of the guest installation."""
>>>          self.validate_parms()
>>>  
>>> @@ -924,7 +924,7 @@ class Guest(object):
>>>  
>>>          self._prepare_install(meter)
>>>          try:
>>> -            return self._do_install(consolecb, meter)
>>> +            return self._do_install(consolecb, meter, removeOld)
>>>          finally:
>>>              self._installer.cleanup()
>>>  
>>> @@ -932,10 +932,22 @@ class Guest(object):
>>>          self._install_disks = self.disks[:]
>>>          self._install_nics = self.nics[:]
>>>  
>>> -    def _do_install(self, consolecb, meter):
>>> +    def _do_install(self, consolecb, meter ,removeOld = False):
>>>          try:
>>> -            if self.conn.lookupByName(self.name) is not None:
>>> -                raise RuntimeError, _("Domain named %s already exists!") %(self.name,)
>>> +            vm = self.conn.lookupByName(self.name)
>>> +            if removeOld:
>>> +                if vm is not None:
>>> +                    if vm.ID() != -1:
>>> +                        logging.info("Destroying image %s" %(self.name))           
>>> +                        vm.destroy()
>>> +                    try:                      
>>> +                        logging.info("Removing old definition for image %s" %(self.name))
>>> +                        vm.undefine()
>>> +                    except libvirt.libvirtError, e:
>>> +                        raise RuntimeError, _("Could not remove old vm '%s': %s") %(self.name, str(e))                       
>>> +            else:
>>> +                if vm is not None:
>>> +                    raise RuntimeError, _("Domain named %s already exists!") %(self.name,)
>>>          except libvirt.libvirtError:
>>>              pass
>>>  
>> Hi, I responded to your initial posting yesterday, but forgot to cc you
>> so maybe you missed it. Most of it is still relevant:
>>
>> https://www.redhat.com/archives/et-mgmt-tools/2008-July/msg00047.html
>>
>> If you address that issue I think the patch will be fine to commit.
>>
>> Thanks,
>> Cole
> 
> 
> I thought it did... the internal libvirt errors from the 
> undefine/destroy are turned into a Rentime error which should be 
> propegated up.
> 
> -- bk

If vm.destroy() raises an exception in the above code, it will only be
caught by the outer except: which ignores libvirterrors, so the failure will
be thrown away.

I would say move the changed code outside the outer exception handler.
If you initialize the vm variable as None, then everything will just
work provided you put the vm.destroy in the inner try block.

- Cole