Fix restore handling checks
PATCH 1/1 Fix restore handling checks
Hi,
this is the patch to fix restore handling to implement some more checks to support more checks than for UUID and name duplicity. This patch basically disallows the migration/restore of IDE drives with the read-only flag since this is not supported according to the ATAPI/IDE specifications so we should disallow this for both domain creation and domain migration/restore. This patch implements it for both create and restore/migrate functionality.
Also, the check whether the host machine does have enough memory available for the guest has been implemented which can be the real issue when you try to migrate a guest from one machine to another that is not having enough memory for this guest. The guest memory gets transferred but it fails to run so it's not running on either of those machines (i.e. domain is not on the destination nor source host machine).
I did try it with restore functionality now since I've been able to make it working for save once so I'm currently using one save image for the testing but unfortunately I'm having many issues with the common migration and save functionality since I've been able to make it working once to save it correctly. Fortunately the restores for this one particular save image is working fine. I was also thinking about 2 concurrent migrations to the guest and/or save with the concurrent migration and it should be the issue (although it's not been tested because of reasons described above) since the domain gets created and it's available in the XendDomain list (i.e. xc.domain_getinfo() list) so it shouldn't be an issue here.
Michal
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
tools/python/xen/xend/XendCheckpoint.py +65 -0
@@ -64,6 +64,70 @@ 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: + 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 = int(get_avail_memory() / 1024) + log.debug("Available memory: %s MiB, guest requires: %s MiB" % (mem_avail, val)) + return val > mem_avail + +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' + +def check_for_restore_hvm_have_readonly_ide(cfg): + """Check the configuration for read-only IDE devices + Fail if such a device is found.""" + is_ide = True + if type(cfg) == list and cfg[0] in ('tap', 'vbd'): + for p in cfg: + if (type(p) != str): + if p[0] == 'dev': + is_ide = (p[1].find('hd') >= 0) + if p[0] == 'mode' and is_ide: + 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): + if item[0] == 'name': + name = item[1] + # 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 for %s' % name) + # 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 +284,7 @@ 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()
tools/python/xen/xend/XendDomainInfo.py +29 -0
@@ -82,6 +82,31 @@ 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_devices(cfg): + """Check the configuration for read-only IDE devices + Fail if such a device is found.""" + if not cfg_is_hvm(cfg): + return False + + is_ide = True + 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': + is_ide = (p[1].find('hd') >= 0) + if p[0] == 'mode' and is_ide: + if p[1] == 'r': + return True + return False def create(config): """Creates and start a VM using the supplied configuration.
@@ -100,6 +125,10 @@ 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_devices(config): + raise VmError("HVM domains cannot be using read-only IDE drives") + log.debug("XendDomainInfo.create(%s)", scrub_password(config)) vm = XendDomainInfo(domconfig) try:
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 7 lines · Michal Novotny
Hi,
this is the patch to fix restore handling to implement some more checks to support more checks than for UUID and name duplicity. This patch basically disallows the migration/restore of IDE drives with the read-only flag since this is not supported according to the ATAPI/IDE specifications so we should disallow this for both domain creation and domain migration/restore.
What about CD-ROMs? This would break my test domain config, for example.
Quoted 9 lines
This patch implements it for both create and
restore/migrate functionality.Also, the check whether the host machine does have enough memory available for the guest has been implemented which can be the real issue when you try to migrate a guest from one machine to another that is not having enough memory for this guest. The guest memory gets transferred but it fails to run so it's not running on either of those machines (i.e. domain is not on the destination nor source host machine).
Failed restore should get reported back to the host that is saving the guest state, and cause that machine to resume execution of the original VM. Does that not work for you?
Possibly checking up front for available memory on the target is a good idea, but it shouldn't be *essential* if the error handling is up to par.
-- Keir
Quoted 15 lines
I did try it with restore functionality now since I've been able to make it working for save once so I'm currently using one save image for the testing but unfortunately I'm having many issues with the common migration and save functionality since I've been able to make it working once to save it correctly. Fortunately the restores for this one particular save image is working fine. I was also thinking about 2 concurrent migrations to the guest and/or save with the concurrent migration and it should be the issue (although it's not been tested because of reasons described above) since the domain gets created and it's available in the XendDomain list (i.e.
xc.domain_getinfo()list) so it shouldn't be an issue here.Michal
Signed-off-by Michal Novotny <[email protected]>
Quoted 11 lines · Keir Fraser
Quoted 8 lines · Michal Novotny
Hi,
this is the patch to fix restore handling to implement some more checks to support more checks than for UUID and name duplicity. This patch basically disallows the migration/restore of IDE drives with the read-only flag since this is not supported according to the ATAPI/IDE specifications so we should disallow this for both domain creation and domain migration/restore.
Quoted 3 lines
What about CD-ROMs? This would break my test domain config, for example.
Right, there's the exception for CD-ROMs according to the spec. I should implement this as well but read-only IDE disk devices are not supported according to the IDE specs.
Quoted 10 lines
Quoted 10 lines
This patch implements it for both create and
restore/migrate functionality.Also, the check whether the host machine does have enough memory available for the guest has been implemented which can be the real issue when you try to migrate a guest from one machine to another that is not having enough memory for this guest. The guest memory gets transferred but it fails to run so it's not running on either of those machines (i.e. domain is not on the destination nor source host machine).
Quoted 9 lines
Failed restore should get reported back to the host that is saving the guest state, and cause that machine to resume execution of the original VM. Does that not work for you?
Possibly checking up front for available memory on the target is a good idea, but it shouldn't be *essential* if the error handling is up to par.
-- Keir
It was not working but I'm still coping with some issues that are save/restore related so maybe this is the reason why I was unable to see this behaviour.
Michal
Quoted 19 lines
Quoted 17 lines
I did try it with restore functionality now since I've been able to make it working for save once so I'm currently using one save image for the testing but unfortunately I'm having many issues with the common migration and save functionality since I've been able to make it working once to save it correctly. Fortunately the restores for this one particular save image is working fine. I was also thinking about 2 concurrent migrations to the guest and/or save with the concurrent migration and it should be the issue (although it's not been tested because of reasons described above) since the domain gets created and it's available in the XendDomain list (i.e.
xc.domain_getinfo()list) so it shouldn't be an issue here.Michal
Signed-off-by Michal Novotny<[email protected]>
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Quoted 10 lines · Michal Novotny
Quoted 9 lines · Keir Fraser
Quoted 7 lines · Michal Novotny
Hi,
this is the patch to fix restore handling to implement some more checks to support more checks than for UUID and name duplicity. This patch basically disallows the migration/restore of IDE drives with the read-only flag since this is not supported according to the ATAPI/IDE specifications so we should disallow this for both domain creation and domain migration/restore.
Quoted 4 lines
Quoted 2 lines
What about CD-ROMs? This would break my test domain config, for example.
Right, there's the exception for CD-ROMs according to the spec. I should implement this as well but read-only IDE disk devices are not supported according to the IDE specs.
This is the updated version of my patch to allow read-only CD-ROM devices as you had a good point that read-only is supported for CD-ROM IDE drives (but only for them).
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
This patch implements it for both create and
restore/migrate functionality.Also, the check whether the host machine does have enough memory available for the guest has been implemented which can be the real issue when you try to migrate a guest from one machine to another that is not having enough memory for this guest. The guest memory gets transferred but it fails to run so it's not running on either of those machines (i.e. domain is not on the destination nor source host machine).
Quoted 9 lines
Quoted 8 lines
Failed restore should get reported back to the host that is saving the guest state, and cause that machine to resume execution of the original VM. Does that not work for you?
Possibly checking up front for available memory on the target is a good idea, but it shouldn't be *essential* if the error handling is up to par.
-- Keir
I was able to make it working now and this functionality seems to be working now however I think that the preliminary check before the transfer itself is a good idea. As far as I had it implemented already I just did some modifications to allow CD-ROM IDE read-only drives and this is the updated version.
Michal
Quoted 19 lines
Quoted 17 lines
Quoted 16 lines
I did try it with restore functionality now since I've been able to make it working for save once so I'm currently using one save image for the testing but unfortunately I'm having many issues with the common migration and save functionality since I've been able to make it working once to save it correctly. Fortunately the restores for this one particular save image is working fine. I was also thinking about 2 concurrent migrations to the guest and/or save with the concurrent migration and it should be the issue (although it's not been tested because of reasons described above) since the domain gets created and it's available in the XendDomain list (i.e.
xc.domain_getinfo()list) so it shouldn't be an issue here.Michal
Signed-off-by Michal Novotny<[email protected]>
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
tools/python/xen/xend/XendCheckpoint.py +66 -0
@@ -64,6 +64,71 @@ 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: + 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 = int(get_avail_memory() / 1024) + log.debug("Available memory: %s MiB, guest requires: %s MiB" % (mem_avail, val)) + return val > mem_avail + +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' + +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): + 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): + if item[0] == 'name': + name = item[1] + # 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 for %s' % name) + # 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 +285,7 @@ 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()
tools/python/xen/xend/XendDomainInfo.py +30 -0
@@ -82,6 +82,32 @@ 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 @@ 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:
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Okay, hopefully someone will be able to Ack this patch with better knwoledge of xend than me.
-- Keir
Quoted 79 lines · Michal Novotny
Quoted 10 lines · Michal Novotny
Quoted 9 lines · Keir Fraser
Quoted 7 lines · Michal Novotny
Hi,
this is the patch to fix restore handling to implement some more checks to support more checks than for UUID and name duplicity. This patch basically disallows the migration/restore of IDE drives with the read-only flag since this is not supported according to the ATAPI/IDE specifications so we should disallow this for both domain creation and domain migration/restore.Quoted 6 lines
Quoted 2 lines
What about CD-ROMs? This would break my test domain config, for example.
Right, there's the exception for CD-ROMs according to the spec. I should implement this as well but read-only IDE disk devices are not supported according to the IDE specs.
This is the updated version of my patch to allow read-only CD-ROM devices as you had a good point that read-only is supported for CD-ROM IDE drives (but only for them).
Quoted 17 lines
Quoted 17 lines
Quoted 11 lines
This patch implements it for both create and
restore/migrate functionality.Also, the check whether the host machine does have enough memory available for the guest has been implemented which can be the real issue
when you try to migrate a guest from one machine to another that is not having enough memory for this guest. The guest memory gets transferred but it fails to run so it's not running on either of those machines (i.e. domain is not on the destination nor source host machine).Failed restore should get reported back to the host that is saving the guest
state, and cause that machine to resume execution of the original VM. Does
that not work for you?Quoted 6 lines
Quoted 5 lines
Possibly checking up front for available memory on the target is a good idea, but it shouldn't be *essential* if the error handling is up to par.
-- Keir
I was able to make it working now and this functionality seems to be working now however I think that the preliminary check before the transfer itself is a good idea. As far as I had it implemented already I just did some modifications to allow CD-ROM IDE read-only drives and this is the updated version.
Michal
Quoted 22 lines
Quoted 20 lines
Quoted 19 lines
I did try it with restore functionality now since I've been able to make
it working for save once so I'm currently using one save image for the testing but unfortunately I'm having many issues with the common migration and save functionality since I've been able to make it working
once to save it correctly. Fortunately the restores for this one particular save image is working fine. I was also thinking about 2 concurrent migrations to the guest and/or save with the concurrent migration and it should be the issue (although it's not been tested because of reasons described above) since the domain gets created and it's available in the XendDomain list (i.e.xc.domain_getinfo()list) so
it shouldn't be an issue here.Michal
Signed-off-by Michal Novotny<[email protected]>
Quoted 6 lines · Keir Fraser
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
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Quoted 11 lines · Michal Novotny
Quoted 5 lines · Keir Fraser
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
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
tools/python/xen/xend/XendCheckpoint.py +73 -0
@@ -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' + +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): + 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()
tools/python/xen/xend/XendDomainInfo.py +30 -0
@@ -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:
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 76 lines · Michal Novotny
Quoted 5 lines · Michal Novotny
Quoted 4 lines · Keir Fraser
Okay, hopefully someone will be able to Ack this patch with better knwoledge
of xend than me.Quoted 8 lines
Quoted 2 lines
-- Keir
Ok, good. If it will be accepted and put into xen-staging/xen-unstable tree please let me know about it.
Thanks,
MichalWell, 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 restoreto 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 <[email protected]>Signature · 5 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat diff -r a24dbfcbdf69 tools/python/xen/xend/XendCheckpoint.pytools/python/xen/xend/XendCheckpoint.py +33
-0@@ -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) +
tools/python/xen/xend/XendCheckpoint.py def insert_after(list, pred, value): · line 109
Quoted 10 lines
tools/python/xen/xend/XendCheckpoint.py +10
-0+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' ?
tools/python/xen/xend/XendCheckpoint.py def insert_after(list, pred, value): · line 117
Quoted 8 lines
tools/python/xen/xend/XendCheckpoint.py +8
-0+ +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 ?
Quoted 69 lines
tools/python/xen/xend/XendCheckpoint.py +22
-0+ 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
a24dbfcbdf69tools/python/xen/xend/XendDomainInfo.pytools/python/xen/xend/XendDomainInfo.py +26
-0@@ -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.
tools/python/xen/xend/XendDomainInfo.py def create(config): · line 135
Quoted 10 lines
tools/python/xen/xend/XendDomainInfo.py +4
-0@@ -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.
Quoted 86 lines · Konrad Rzeszutek Wilk
Quoted 84 lines · Michal Novotny
Quoted 6 lines · Michal Novotny
Quoted 4 lines · Keir Fraser
Okay, hopefully someone will be able to Ack this patch with better knwoledge
of xend than me.Quoted 10 lines
Quoted 3 lines
-- Keir
Ok, good. If it will be accepted and put into xen-staging/xen-unstable tree please let me know about it.
Thanks,
MichalWell, 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 restoreto 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<[email protected]>Signature · 6 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat diff -r a24dbfcbdf69 tools/python/xen/xend/XendCheckpoint.pytools/python/xen/xend/XendCheckpoint.py +37
-0@@ -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)
Quoted 9 lines
tools/python/xen/xend/XendCheckpoint.py def insert_after(list, pred, value): · line 139
Quoted 7 lines
tools/python/xen/xend/XendCheckpoint.py +6
-0+ +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.
Quoted 11 lines
tools/python/xen/xend/XendCheckpoint.py def insert_after(list, pred, value): · line 139
Quoted 9 lines
tools/python/xen/xend/XendCheckpoint.py +8
-0+ +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'.
Quoted 66 lines
Quoted 66 lines
tools/python/xen/xend/XendCheckpoint.py +21
-0+ 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 XendDomaintools/python/xen/xend/XendCheckpoint.py +1
-0@@ -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
a24dbfcbdf69tools/python/xen/xend/XendDomainInfo.pytools/python/xen/xend/XendDomainInfo.py +26
-0@@ -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
Quoted 16 lines
Quoted 4 lines
tools/python/xen/xend/XendDomainInfo.py +0
-0def create(config): """Creates and start a VM using the supplied configuration.tools/python/xen/xend/XendDomainInfo.py def create(config): · line 135
Quoted 10 lines
tools/python/xen/xend/XendDomainInfo.py +4
-0@@ -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
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Correct me if I am wrong, but I think your patch assumes that the amount of free memory in the system can be computed by assuming each guest memory is fixed size. Due to various features in Xen 4.0, this is no longer a safe assumption. Tmem has a libxc call to freeze and unfreeze its use of memory so dynamic memory use by tmem can be stopped, and another libxc call to determine "freeable" memory, and another to free it. I don't know if the page-sharing functionality added at 4.0 has anything similar.
But in any case, simple algorithms to add up current (or max) guest memory will have many false-positive and false-negative results.
Quoted 42 lines
-----Original Message-----
From: Michal Novotny [mailto:[email protected]]
Sent: Tuesday, June 22, 2010 6:56 AM
To: [email protected]
Subject: Re: [Xen-devel] [PATCH] Fix restore handling checksQuoted 8 lines · Michal Novotny
Quoted 6 lines · Keir Fraser
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
Quoted 5 lines
tree please let me know about it.
Thanks,
MichalWell, 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 restoreto 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 <[email protected]>--
Michal Novotny<[email protected]>, RHCE
Virtualization Team (xen userspace), Red Hat
Quoted 12 lines · Dan Magenheimer
Correct me if I am wrong, but I think your patch assumes that the amount of free memory in the system can be computed by assuming each guest memory is fixed size. Due to various features in Xen 4.0, this is no longer a safe assumption. Tmem has a libxc call to freeze and unfreeze its use of memory so dynamic memory use by tmem can be stopped, and another libxc call to determine "freeable" memory, and another to free it. I don't know if the page-sharing functionality added at 4.0 has anything similar.
Honestly I don't know about how tmem features work since I have them disabled because I'm unable to start any guest with them enabled because it fails on creation.
Michal
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Quoted 7 lines · Dan Magenheimer
Correct me if I am wrong, but I think your patch assumes that the amount of free memory in the system can be computed by assuming each guest memory is fixed size. Due to various features in Xen 4.0, this is no longer a safe assumption. Tmem has a libxc call to freeze and unfreeze its use of memory so dynamic memory use by tmem can be stopped
Maybe it's stopped but in the domain_getinfo() of libxc we should be getting the value of original guest memory although it's being freezed but the only difference is that the memory should not be accessible since it's locked somehow. Is my understanding of tmem freeze correct?
Quoted 10 lines
and another libxc call to
determine "freeable" memory, and another to free it. I don't know if the page-sharing functionality added at 4.0 has anything similar.But in any case, simple algorithms to add up current (or max) guest memory will have many false-positive and false-negative results.
Why should it give too many false-positives/false-negatives since the handling there is to sum the total guest memory and decrease the computed size from the total host memory according to physinfo() output from libxc. Also there should be the minimal memory for dom0 taken in account since. There's the example for my configuration - I'm having 8G of RAM in total, if I start up one guest with 2G of RAM allocated, we should be having 8 - 2 = 6 G available now (no matter what amount of memory is being allocated to the dom0 since the physinfo() is getting the total memory information from hypervisor directly, i.e. you could be having 4G allocated to dom0 but the host machine could be having 8G of RAM in total).
1. total physical memory = 8G
2. dom0_mem = 4G, dom0-min-mem = 1G
3. create the guest A with 2G RAM -> 6G in total are available now
4. create the guest B with 4G RAM -> 4G should be available but guest is still on migration/restore
5. In the middle of the guest restore/migrate from step 4 (guest B) we start another migration/restore of 2G guest (guest C), since the guest B is having already 2G memory, that way "mem_kb" equals to 2G for guest B (instead of 4G) so we have to take "maxmem_kb" instead (i.e. 4G value) to compute we don't have enough memory for guest C creation.
If we used "mem_kb" in all the cases (even for migration/restore case) we would sum up the value to be: 2 + 2 (there should be 4 since the guest is restoring right now) + 2 = 6G which is less than 8G (total memory) - 1G (dom0-min-mem) = 7G so it would allow the guest creation which would result into failure when the migration/restore of guest C fails and therefore the guest C will be destroyed with incomplete memory transfer.
That's why I used the computation of "maxmem_kb" instead, since for this scenario the value is: 2 + 4 + 2 = 8G which is bigger than 7G (total memory - dom0-min-mem) so we disallow the guest restore immediately.
So what should those calculations give many false-positives or false-negatives?
Michal
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Quoted 5 lines
From: Michal Novotny [mailto:[email protected]]
Honestly I don't know about how tmem features work since I have them disabled because I'm unable to start any guest with them enabled because it fails on creation.
If you are booting Xen enabling tmem and NOT setting dom0_mem= there was a bug just reported and fixed (or patch posted at least). If this is another problem with tmem (e.g. you ARE setting both tmem and dom0_mem), please tell me more.
However, note that just enabling tmem in Xen doesn't do anything. Tmem must also be enabled in guests.
Quoted 1 line
Why should it give too many false-positives/false-negatives...
The issue is that in a tmem system (with self-ballooning enabled which will be the default for tmem-enabled guests), all the guests are dynamically changing in size between their maximum memory and some minimum which may be smaller than the mem= they were launched with.
And "free" memory as reported by Xen is also changing dynamically due to guest tmem calls.
The only way to ensure a fixed amount of memory is available is:
1) freeze tmem
2) get free memory and "tmem freeable" memory and if there is enough
3) ALLOCATE the memory NOW
4) check to ensure the allocation worked
5) unfreeze tmem
The code in xends currently does this when launching new domains.
Tmem freeze only stops tmem from allocating more free memory from Xen (by failing all tmem calls from tmem-enabled guests). It doesn't stop guest ballooning activity. So even the "ALLOCATE the memory NOW" may fail... but that is acceptable when launching a new domain.
Hope that helps,
Dan
Quoted 6 lines · Dan Magenheimer
Quoted 6 lines
From: Michal Novotny [mailto:[email protected]]
Honestly I don't know about how tmem features work since I have them disabled because I'm unable to start any guest with them enabled because it fails on creation.
Quoted 9 lines
If you are booting Xen enabling tmem and NOT setting dom0_mem= there was a bug just reported and fixed (or patch posted at least). If this is another problem with tmem (e.g. you ARE setting both tmem and
dom0_mem), please tell me more.However, note that just enabling tmem in Xen doesn't do anything. Tmem must also be enabled in guests.
Well, I double-checked the configuration and I was not using dom0_mem on HV command-line so this is the bug I hit. It's a version about week old or something like that so I guess it was the issue I met.
Quoted 21 lines
Quoted 2 lines
Why should it give too many false-positives/false-negatives...
The issue is that in a tmem system (with self-ballooning enabled which will be the default for tmem-enabled guests), all the guests are dynamically changing in size between their maximum memory and some minimum which may be smaller than the mem= they were launched with.
And "free" memory as reported by Xen is also changing dynamically due to guest tmem calls.
The only way to ensure a fixed amount of memory is available is:
1) freeze tmem
2) get free memory and "tmem freeable" memory and if there is enough
3) ALLOCATE the memory NOW
4) check to ensure the allocation worked
5) unfreeze tmemThe code in xends currently does this when launching new domains.
Quoted 10 lines
Tmem freeze only stops tmem from allocating more free memory from Xen (by failing all tmem calls from tmem-enabled guests). It doesn't stop guest ballooning activity. So even the "ALLOCATE the memory NOW" may fail... but that is acceptable when launching a new domain.
Hope that helps,
Dan
Oh, ok. Then I guess that the patch could be dropped since there are objections against both IDE read-only disk violation and memory check parts, i.e. against both parts.
Michal
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Michal Novotny writes ("Re: [Xen-devel] [PATCH] Fix restore handling checks"):
Quoted 3 lines
Right, there's the exception for CD-ROMs according to the spec. I should implement this as well but read-only IDE disk devices are not supported according to the IDE specs.
I don't think this is a very good argument for not supporting them.
The Xen administrator should be able to present a guest with an IDE disk and nevertheless prevent the guest from writing to it.
If that means that the guest administrator needs to be told out-of-band that writes will not work, rather than having their kernel automatically discover the read-only nature, then that's fine. And if the guest administrator doesn't do that and their kernel tries to write and gets IDE errors then that is the correct consequence.
Ian.
Quoted 7 lines · Ian Jackson
Michal Novotny writes ("Re: [Xen-devel] [PATCH] Fix restore handling checks"):
Quoted 3 lines
Right, there's the exception for CD-ROMs according to the spec. I should implement this as well but read-only IDE disk devices are not supported according to the IDE specs.
I don't think this is a very good argument for not supporting them.
Quoted 8 lines
The Xen administrator should be able to present a guest with an IDE disk and nevertheless prevent the guest from writing to it.
If that means that the guest administrator needs to be told out-of-band that writes will not work, rather than having their kernel automatically discover the read-only nature, then that's fine. And if the guest administrator doesn't do that and their kernel tries to write and gets IDE errors then that is the correct consequence.
We tried to get that to work, and we could not really find a device error that worked reliably in the guests. We often got infinite loops on mount or little after that, on both Linux and Windows.
If you want to use read-only devices, you should really use SCSI or PV devices.
Paolo
Paolo Bonzini writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 3 lines
We tried to get that to work, and we could not really find a device error that worked reliably in the guests. We often got infinite loops on mount or little after that, on both Linux and Windows.
That's fine. If the admins don't want that then they can use SCSI or PV or simply not tell the guest to write to the device.
Disallowing the configuration entirely just because the error handling is suboptimal (but safe!) is not correct.
Ian.
Quoted 6 lines · Ian Jackson
Paolo Bonzini writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 4 lines
We tried to get that to work, and we could not really find a device error that worked reliably in the guests. We often got infinite loops on mount or little after that, on both Linux and Windows.
Quoted 8 lines
That's fine. If the admins don't want that then they can use SCSI or PV or simply not tell the guest to write to the device.
Disallowing the configuration entirely just because the error handling is suboptimal (but safe!) is not correct.
Ian.
Why do you think so, Ian? This follows the IDE specifications to disallow read-only IDE disks since the specs doesn't support IDE read-only disks. The only read-only device being supported by IDE specs is the CD-ROM device.
Michal
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 4 lines
Why do you think so, Ian? This follows the IDE specifications to disallow read-only IDE disks since the specs doesn't support IDE read-only disks. The only read-only device being supported by IDE specs is the CD-ROM device.
The administrator should be free to violate the IDE specification if the IDE specification conflicts with their security goals.
Ian.
Quoted 7 lines · Ian Jackson
Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 5 lines
Why do you think so, Ian? This follows the IDE specifications to disallow read-only IDE disks since the specs doesn't support IDE read-only disks. The only read-only device being supported by IDE specs is the CD-ROM device.
Quoted 5 lines
The administrator should be free to violate the IDE specification if the IDE specification conflicts with their security goals.
Ian.
Are you saying that it's OK for administrators to violate the IDE specs and do it the way that is should never be done since this way it's not working on bare-metal systems ? This is the breach and it shouldn't be done this way so why to allow it? Shouldn't we care the code complies with the specifications to have it done the right way?
Michal
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 5 lines
Are you saying that it's OK for administrators to violate the IDE specs and do it the way that is should never be done since this way it's not working on bare-metal systems ? This is the breach and it shouldn't be done this way so why to allow it? Shouldn't we care the code complies with the specifications to have it done the right way?
The job of the programmer is to give effect to the wishes of the users, not to comply with rules from elsewhere. If the wishes of the users conflict with rules from elsewhere, including specs, then the programmer should do what the user wants.
Ian.
Quoted 11 lines · Ian Jackson
Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 5 lines
Are you saying that it's OK for administrators to violate the IDE specs and do it the way that is should never be done since this way it's not working on bare-metal systems ? This is the breach and it shouldn't be done this way so why to allow it? Shouldn't we care the code complies with the specifications to have it done the right way?
The job of the programmer is to give effect to the wishes of the users, not to comply with rules from elsewhere. If the wishes of the users conflict with rules from elsewhere, including specs, then the programmer should do what the user wants.
Telling him that he wanted something that cannot be _emulated_ accurately is also a possibility. But we can agree to disagree here and Michal can remove this part of the patch.
Paolo
Quoted 8 lines · Paolo Bonzini
Quoted 7 lines · Ian Jackson
Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 5 lines
Are you saying that it's OK for administrators to violate the IDE specs and do it the way that is should never be done since this way it's not working on bare-metal systems ? This is the breach and it shouldn't be done this way so why to allow it? Shouldn't we care the code complies with the specifications to have it done the right way?
Quoted 9 lines
Quoted 4 lines
The job of the programmer is to give effect to the wishes of the users, not to comply with rules from elsewhere. If the wishes of the users conflict with rules from elsewhere, including specs, then the programmer should do what the user wants.
Telling him that he wanted something that cannot be _emulated_ accurately is also a possibility. But we can agree to disagree here and Michal can remove this part of the patch.
Paolo
Well, just one correction to this Paolo. It can be emulated but the emulation doesn't comply to the specifications and since it was never supported at least for HVM guests prior to my patch to fix read-only image handling so it shouldn't break anything so if it was not emulated before the patch I sent 13 days ago why not to emulate/implement it the right way? I don't see any reason to emulate it the wrong way since we know the right way that's been implemented/fixed just 13 days ago - at least for HVM guests. If users want to use read-only drives they can use SCSI drives so what's the problem here?
Surely, I can remove this part of the patch but I'm just trying to tell the readers of this thread that I don't see much point in having the wrong implementation. I don't think users want to do it the wrong way and I think they could understand that this is not supported and that they should use SCSI drive definition for read-only drives instead.
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Quoted 8 lines · Paolo Bonzini
Quoted 7 lines · Ian Jackson
Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 5 lines
Are you saying that it's OK for administrators to violate the IDE specs and do it the way that is should never be done since this way it's not working on bare-metal systems ? This is the breach and it shouldn't be done this way so why to allow it? Shouldn't we care the code complies with the specifications to have it done the right way?
Quoted 9 lines
Quoted 4 lines
The job of the programmer is to give effect to the wishes of the users, not to comply with rules from elsewhere. If the wishes of the users conflict with rules from elsewhere, including specs, then the programmer should do what the user wants.
Telling him that he wanted something that cannot be _emulated_ accurately is also a possibility. But we can agree to disagree here and Michal can remove this part of the patch.
Paolo
Well, just one correction to this Paolo. It can be emulated but the emulation doesn't comply to the specifications and since it was never supported at least for HVM guests prior to my patch to fix read-only image handling so it shouldn't break anything so if it was not emulated before the patch I sent 13 days ago why not to emulate/implement it the right way? I don't see any reason to emulate it the wrong way since we know the right way that's been implemented/fixed just 13 days ago - at least for HVM guests. If users want to use read-only drives they can use SCSI drives so what's the problem here?
Surely, I can remove this part of the patch but I'm just trying to tell the readers of this thread that I don't see much point in having the wrong implementation. I don't think users want to do it the wrong way and I think they could understand that this is not supported and that they should use SCSI drive definition for read-only drives instead.
Michal
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Quoted 8 lines · Ian Jackson
Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 6 lines
Are you saying that it's OK for administrators to violate the IDE specs and do it the way that is should never be done since this way it's not working on bare-metal systems ? This is the breach and it shouldn't be done this way so why to allow it? Shouldn't we care the code complies with the specifications to have it done the right way?
Quoted 7 lines
The job of the programmer is to give effect to the wishes of the users, not to comply with rules from elsewhere. If the wishes of the users conflict with rules from elsewhere, including specs, then the programmer should do what the user wants.
Ian.
Right but I don't think someone uses the read-only IDE drives or read-only drives in general - at least for HVM guests since just few days I posted the patch to disallow write to the read-only devices on ioemu - maybe you remember I did post the patch at [1]. Before this patch applied the read-only disks were treated as read-write always no matter what the state of disk device was present in the xenstore so why not to teach the users the right things according to the specs. This shouldn't break anything if you consider you added my patch to xen-ioemu just 13 days ago and I find it rather confusing to let users define read-only IDE disks which is not right according to the specs, this means that by allowing it we could confuse the users so that they can think that read-only IDE disks does exists even according to the specs and they may try to think that the bare-metal IDE disks does support it as well. Since this shouldn't break anything why shouldn't we do the things the right way? They have SCSI disks and PV drivers if they want read-only disks after all.
Michal
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat
Quoted 11 lines · On Wed, 23 Jun 2010 13:04:26 +0100 Ian Jackson <[email protected]>…
Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 5 lines
Are you saying that it's OK for administrators to violate the IDE specs and do it the way that is should never be done since this way it's not working on bare-metal systems ? This is the breach and it shouldn't be done this way so why to allow it? Shouldn't we care the code complies with the specifications to have it done the right way?
The job of the programmer is to give effect to the wishes of the users, not to comply with rules from elsewhere. If the wishes of the users conflict with rules from elsewhere, including specs, then the programmer should do what the user wants.
So why doesn't Xen automatically produce free ice cream when the server room gets warm ? and why doesn't Xen support storing 1Tb of data on a 40GB disc ? These are what the user wants.
That said there is nothing forbidding read only IDE media. It's unusual and in the real world you normally only encounter it via hardware plugins used for forensics work (so you can mirror a device and stand up in court and say 'Neither I nor my system could have modified the contents of the disc').
I guess the elegant way would be to present a read only IDE volume as a magneto optical (ie ATAPI disc) but simply aborting (0x04) any write commands isn't any different to what some of the drive protection widgets do.
Alan
Quoted 3 lines · Alan Cox
So why doesn't Xen automatically produce free ice cream when the server room gets warm ? and why doesn't Xen support storing 1Tb of data on a 40GB disc ? These are what the user wants.
If there was a ISO server room standard somewhere that specified no automatically generated free ice cream, but a 200-line patch would include this feature in Xen (disabled by default), I'd say the right thing to do is to include the patch, standard be damned. More to the point, if generating free ice cream was a natural byproduct of whatever we were doing already, and someone said, "But the standard says no automatic free ice cream" and submitted a patch to disable it, I'd argue against it too. Same principle applies. :-)
-George
Quoted 8 lines
Quoted 4 lines
The job of the programmer is to give effect to the wishes of the users, not to comply with rules from elsewhere. If the wishes of the users conflict with rules from elsewhere, including specs, then the programmer should do what the user wants.
So why doesn't Xen automatically produce free ice cream when the server room gets warm ? and why doesn't Xen support storing 1Tb of data on a 40GB disc ? These are what the user wants.
Mmm.... free ice cream... mmmm. This user wishes for it. Ian, could you kindly do what this user wants, even though it conflicts with rules from my wife?
(sorry couldn't resist :-)
Dan
Quoted 13 lines · On Wed, 23 Jun 2010 13:29:56 +0200 Michal Novotny <[email protected]> wrote:
Quoted 12 lines · Ian Jackson
Paolo Bonzini writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 4 lines
We tried to get that to work, and we could not really find a device error that worked reliably in the guests. We often got infinite loops on mount or little after that, on both Linux and Windows.
That's fine. If the admins don't want that then they can use SCSI or PV or simply not tell the guest to write to the device.
Disallowing the configuration entirely just because the error handling is suboptimal (but safe!) is not correct.
Quoted 7 lines
Quoted 2 lines
Ian.
Why do you think so, Ian? This follows the IDE specifications to disallow read-only IDE disks since the specs doesn't support IDE read-only disks. The only read-only device being supported by IDE specs is the CD-ROM device.
IDE (or more properly ATA) doesn't support CD-ROM either. ATAPI does, and it also supports other ATAPI devices with write protect including magneto optical and floppy.
Quoted 23 lines · Alan Cox
Quoted 19 lines · On Wed, 23 Jun 2010 13:29:56 +0200 Michal Novotny<[email protected]> wrote:
Quoted 17 lines · Ian Jackson
Paolo Bonzini writes ("Re: [PATCH] Fix restore handling checks"):
Quoted 5 lines
We tried to get that to work, and we could not really find a device error that worked reliably in the guests. We often got infinite loops on mount or little after that, on both Linux and Windows.
That's fine. If the admins don't want that then they can use SCSI or PV or simply not tell the guest to write to the device.
Disallowing the configuration entirely just because the error handling is suboptimal (but safe!) is not correct.
Ian.
Quoted 11 lines
Quoted 5 lines
Why do you think so, Ian? This follows the IDE specifications to disallow read-only IDE disks since the specs doesn't support IDE read-only disks. The only read-only device being supported by IDE specs is the CD-ROM device.
IDE (or more properly ATA) doesn't support CD-ROM either. ATAPI does, and it also supports other ATAPI devices with write protect including magneto optical and floppy.
Agreed Alan. This is right that ATAPI does. Nevertheless in IDE specs it's about ATA and ATAPI. The read-only file systems are supported for IDE drives only for floppy and tape media according to the linux kernel source codes. Also, CD-ROM devices can be read-only but no other IDE media, i.e. no IDE disks.
Michal
Signature · 2 lines
Michal Novotny<[email protected]>, RHCE Virtualization Team (xen userspace), Red Hat