[Kimchi-devel] [PATCH 7/8] Check QEMU stream DNS capability when attaching new disk to guest

[Aline Manera]

On 10/29/2014 06:26 PM, Paulo Ricardo Paz Vital wrote:
> On Tue, 2014-10-28 at 10:43 -0200, Aline Manera wrote:
>> When QEMU is not able to solve hostname for a remote CDROM path, we need
>> to convert the hostname to IP to avoid problems.
>> To make xmlutils/ independent of model instances, that verification must
>> be done prior to the XML generation.
>>
>> Signed-off-by: Aline Manera <alinefm at linux.vnet.ibm.com>
>> ---
>>   src/kimchi/model/vmstorages.py | 16 ++++++++++++++++
>>   tests/test_model.py            | 11 ++++++++++-
>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py
>> index 70240b0..576d66d 100644
>> --- a/src/kimchi/model/vmstorages.py
>> +++ b/src/kimchi/model/vmstorages.py
>> @@ -17,12 +17,15 @@
>>   # License along with this library; if not, write to the Free Software
>>   # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301 USA
>>
>> +import socket
>>   import string
>> +import urlparse
>>
>>   from lxml import etree
>>
>>   from kimchi.exception import InvalidOperation, InvalidParameter, NotFoundError
>>   from kimchi.exception import OperationFailed
>> +from kimchi.model.config import CapabilitiesModel
>>   from kimchi.model.vms import DOM_STATE_MAP, VMModel
>>   from kimchi.model.storagevolumes import StorageVolumeModel
>>   from kimchi.model.utils import get_vm_config_flag
>> @@ -122,6 +125,13 @@ class VMStoragesModel(object):
>>               params['path'] = vol_info['path']
>>
>>           params.update(self._get_available_bus_address(params['bus'], vm_name))
>> +
>> +        # Check QEMU can resolve hostname
>> +        hostname = urlparse.urlparse(params['path']).hostname
>> +        if hostname is not None and not CapabilitiesModel().qemu_stream_dns:
>> +            ip = socket.gethostbyname(hostname)
>> +            params['path'] = params['path'].replace(hostname, ip)
>> +
>>           # Add device to VM
>>           dev, xml = get_disk_xml(params)
>>           try:
>> @@ -178,6 +188,12 @@ class VMStorageModel(object):
>>           params['path'] = params.get('path', '')
>>           dev_info.update(params)
>>
>> +        # Check QEMU can resolve hostname
>> +        hostname = urlparse.urlparse(params['path']).hostname
>> +        if hostname is not None and not CapabilitiesModel().qemu_stream_dns:
>> +            ip = socket.gethostbyname(hostname)
>> +            params['path'] = params['path'].replace(hostname, ip)
>> +
> The same block of code in two different parts? Why not create a new
> function on utils a function to do this check?

Sure. I will do that on V3.

>
> One more question? Shouldn't the "dev_info.update(params)" line moved to
> be after the "Check QEMU can resolve hostname" block? Looks like the
> params['path'] will contain the wrong value if the "if" statement is
> true.

Yeap. I will fix it on V3 too.

Thanks.

>
>>           dev, xml = get_disk_xml(dev_info)
>>           try:
>>               dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all'))
>> diff --git a/tests/test_model.py b/tests/test_model.py
>> index e407fe5..f094ed7 100644
>> --- a/tests/test_model.py
>> +++ b/tests/test_model.py
>> @@ -25,9 +25,11 @@ import psutil
>>   import pwd
>>   import re
>>   import shutil
>> +import socket
>>   import tempfile
>>   import threading
>>   import time
>> +import urlparse
>>   import unittest
>>   import uuid
>>
>> @@ -433,7 +435,14 @@ class ModelTests(unittest.TestCase):
>>                                     {'path': valid_remote_iso_path})
>>               cdrom_info = inst.vmstorage_lookup(vm_name, cdrom_dev)
>>               cur_cdrom_path = re.sub(":80/", '/', cdrom_info['path'])
>> -            self.assertEquals(valid_remote_iso_path, cur_cdrom_path)
>> +
>> +            # As Kimchi server is not running during this test case
>> +            # CapabilitiesModel.qemu_stream_dns will be always False
>> +            # so we need to convert the hostname to IP
>> +            output = urlparse.urlparse(valid_remote_iso_path)
>> +            hostname = socket.gethostbyname(output.hostname)
>> +            url = valid_remote_iso_path.replace(output.hostname, hostname)
>> +            self.assertEquals(url, cur_cdrom_path)
>>
>>       @unittest.skipUnless(utils.running_as_root(), 'Must be run as root')
>>       def test_vm_storage_provisioning(self):