Re: [PATCH] Fix restore handling checks

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Jun 22, 2010 at 02:56:24PM +0200, Michal Novotny wrote:
> On 06/22/2010 08:17 AM, Michal Novotny wrote:
> >On 06/22/2010 08:14 AM, Keir Fraser wrote:
> >>Okay, hopefully someone will be able to Ack this patch with
> >>better knwoledge
> >>of xend than me.
> >>
> >>  -- Keir
> >>
> >Ok, good. If it will be accepted and put into
> >xen-staging/xen-unstable tree please let me know about it.
> >
> >Thanks,
> >Michal
> >
> 
> Well, this is new version of the patch since I've tried now to both
> migrate from RHEL-5 host (python-2.4) to RHEL-6 host (python-2.6)
> using the RHEL-5 host as a source machine for migrations and also
> RHEL-6 host to restore the the guest locally using `xm restore` to
> see whether the guest memory calculated correctly and it did pass
> when there was enough memory to create the guest and failed when
> there was not enough memory to create the guest. Also, some fixing
> for comparing the values was necessary since it was treated as
> string comparison on one of the hosts.
> 
> Both the host machines were having the latest Xen-4.1 installed.
> 
> Michal
> 
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> 
> -- 
> Michal Novotny<minovotn@redhat.com>, RHCE
> Virtualization Team (xen userspace), Red Hat
> 

> diff -r a24dbfcbdf69 tools/python/xen/xend/XendCheckpoint.py
> --- a/tools/python/xen/xend/XendCheckpoint.py	Tue Jun 22 07:19:38 2010 +0100
> +++ b/tools/python/xen/xend/XendCheckpoint.py	Tue Jun 22 12:52:56 2010 +0200
> @@ -64,6 +64,78 @@ def insert_after(list, pred, value):
>                list.insert (i+1, value)
>      return
>  
> +def get_avail_memory():
> +    """Get available memory for new guest creation (in KiB, for restore)"""
> +    from xen.xend import XendOptions
> +
> +    # First get total memory in KiB
> +    xc = xen.lowlevel.xc.xc()
> +    info = xc.domain_getinfo()
> +    total_mem  = xc.physinfo()['total_memory']
> +    del xc
> +
> +    # Count memory of all running guests in KiB
> +    mem_used = 0L
> +    for x in info:
> +        if x['domid'] != 0:
> +            # If blocked & paused & !paused & no online_vcpus we most
> +            # probably migrate so we get maxmem instead (since mem_kb
> +            # is having currently transferred amount of memory so we
> +            # cannot use it for good calculations)
> +            if (x['blocked'] == 1 and x['paused'] == 1 and
> +                x['running'] == 0 and x['online_vcpus'] == 0):
> +                log.debug("Domain %d seems to be restoring" % x['domid'])
> +                mem_used += x['maxmem_kb']
> +            else:
> +                mem_used += x['mem_kb']
> +
> +    # Get minimal memory for dom0 and convert to KiB
> +    min_mem = XendOptions.instance().get_dom0_min_mem() * 1024
> +
> +    return total_mem - mem_used - min_mem
> +
> +def check_for_restore_bail(msg):
> +    raise VmError("Cannot restore: %s" % msg)
> +
> +def check_for_enough_mem(val):
> +    mem_avail = get_avail_memory() / 1024
> +    log.debug("Available memory: %s MiB, guest requires: %s MiB" % (mem_avail, val))
> +    return int(mem_avail) > int(val)
> +
> +def check_for_restore_is_hvm(cfg):
> +    for item in cfg:
> +        if (type(item) == list):
> +           if item[0] == 'image':
> +               return type(item[1]) == list and item[1][0] == 'hvm'

There is no chance that 'hvm' would be in different case? Say 'HVM' ?

> +
> +def check_for_restore_hvm_have_readonly_ide(cfg):
> +    """Check the configuration for read-only IDE devices
> +       Fail if such a device is found."""
> +    disallow = None
> +    if type(cfg) == list and cfg[0] in ('tap', 'vbd'):
> +        for p in cfg:
> +            if (type(p) != str):

Is that right? should it not == list ?
> +                if p[0] == 'dev':
> +                    disallow = ((p[1].find('hd') >= 0) and
> +                                (p[1].find('cdrom') == -1))
> +                if p[0] == 'mode' and disallow:
> +                    if p[1] == 'r':
> +                        return True
> +    return False
> +
> +def check_for_restore(cfg):
> +    is_hvm = check_for_restore_is_hvm(cfg)
> +    name = None
> +    for item in cfg:
> +        if (type(item) == list):
> +            # Check for enough memory to create the guest
> +            if item[0] == 'memory':
> +                if not check_for_enough_mem(item[1]):
> +                    check_for_restore_bail('Host machine doesn\'t have enough memory to create the guest')
> +            # We disable read-only IDE drives only for HVM guests
> +            if item[0] == 'device' and is_hvm:
> +                if check_for_restore_hvm_have_readonly_ide(item[1]):
> +                    check_for_restore_bail('HVM domains cannot be using read-only IDE drives')
>  
>  def save(fd, dominfo, network, live, dst, checkpoint=False, node=-1):
>      from xen.xend import XendDomain
> @@ -220,6 +292,7 @@ def restore(xd, fd, dominfo = None, paus
>              othervm = xd.domain_lookup_nr(domconfig["uuid"])
>          if othervm is not None and othervm.domid is not None: 
>              raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
> +        check_for_restore(vmconfig)
>  
>      if dominfo:
>          dominfo.resume()
> diff -r a24dbfcbdf69 tools/python/xen/xend/XendDomainInfo.py
> --- a/tools/python/xen/xend/XendDomainInfo.py	Tue Jun 22 07:19:38 2010 +0100
> +++ b/tools/python/xen/xend/XendDomainInfo.py	Tue Jun 22 12:52:56 2010 +0200
> @@ -82,6 +82,32 @@ log = logging.getLogger("xend.XendDomain
>  log = logging.getLogger("xend.XendDomainInfo")
>  #log.setLevel(logging.TRACE)
>  
> +def cfg_is_hvm(cfg):
> +    for item in cfg:
> +        if (type(item) == list):
> +           if item[0] == 'image':
> +               return type(item[1]) == list and item[1][0] == 'hvm'
> +
> +def cfg_hvm_have_readonly_ide_disks(cfg):
> +    """Check whether the configuration for read-only IDE disks since
> +       they are not supported according to IDE specifications."""
> +    if not cfg_is_hvm(cfg):
> +        return False
> +
> +    disallow = None
> +    for item in cfg:
> +        if (type(item) == list):
> +            if item[0] == 'device':
> +                if type(item[1]) == list and item[1][0] in ('tap', 'vbd'):
> +                    for p in item[1]:
> +                        if (type(p) != str):
> +                            if p[0] == 'dev':
> +                              disallow = ((p[1].find('hd') >= 0) and
> +                                          (p[1].find('cdrom') == -1))
> +                        if p[0] == 'mode' and disallow:
> +                            if p[1] == 'r':
> +                                return True
> +    return False
>  
>  def create(config):
>      """Creates and start a VM using the supplied configuration. 
> @@ -100,6 +126,10 @@ def create(config):
>          othervm = XendDomain.instance().domain_lookup_nr(domconfig["uuid"])
>      if othervm is not None and othervm.domid is not None:
>          raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
> +
> +    if cfg_hvm_have_readonly_ide_disks(config):
> +        raise VmError("HVM domains cannot be using read-only IDE drives")
> +
>      log.debug("XendDomainInfo.create(%s)", scrub_password(config))
>      vm = XendDomainInfo(domconfig)
>      try:

Otherwise looks good to me.