Re: [PATCH] Fix restore handling checks

[Michal Novotny <minovotn@redhat.com>]

On 06/22/2010 04:10 PM, Konrad Rzeszutek Wilk wrote:
> 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' ?
>    

According to my testing it's always lowercase here and maybe everything 
on the SXP is lowercase. Nevertheless I am not sure about everything but 
what I'm sure is that the hvm is *always* lowercase.
>> +
>> +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 ?
>    

It's been tested and working fine. There are no more options than 'list' 
and 'str' so in fact this is the same to implement it like 'if .. != 
str' and 'if ... == 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.
>    
Good, thanks for review.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat