[Kimchi-devel] [PATCHv3 2/2] Integrate nfs path check before create nfs pool

[Royce Lv]

On 2014年01月14日 13:21, Zhou Zheng Sheng wrote:
> on 2014/01/13 18:40, lvroyce at linux.vnet.ibm.com wrote:
>> From: Royce Lv <lvroyce at linux.vnet.ibm.com>
>>
>> Before creating nfs pool, reload the prevalidation for the path
>> and see if it is able to complish a quick mount.
>> If not, deny this storage pool from being created.
>>
>> Signed-off-by: Royce Lv <lvroyce at linux.vnet.ibm.com>
>> ---
>>   src/kimchi/model.py | 37 ++++++++++++++++++++++++++++++++++---
>>   1 file changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/kimchi/model.py b/src/kimchi/model.py
>> index 671af02..189b2d3 100644
>> --- a/src/kimchi/model.py
>> +++ b/src/kimchi/model.py
>> @@ -70,7 +70,7 @@ from kimchi.isoinfo import IsoImage
>>   from kimchi.objectstore import ObjectStore
>>   from kimchi.scan import Scanner
>>   from kimchi.screenshot import VMScreenshot
>> -from kimchi.utils import get_enabled_plugins, is_digit, kimchi_log
>> +from kimchi.utils import get_enabled_plugins, is_digit, kimchi_log, parse_cmd_output
>>   from kimchi.vmtemplate import VMTemplate
>>
>>
>> @@ -1557,8 +1557,39 @@ class NetfsPoolDef(StoragePoolDef):
>>           self.path = '/var/lib/kimchi/nfs_mount/' + self.poolArgs['name']
>>
>>       def prepare(self, conn):
>> -        # TODO: Verify the NFS export can be actually mounted.
>> -        pass
>> +        # FIXME: When run_cmd helper function is ready
>> +        # this needs to be re-write based on context manager and helper.
>> +        res = False
>> +        outputs = None
> I would suggest you rename 'res' to 'mountOK' or 'mounted', and move
> "res = False" to near where you use it, just above "for item in mounts:".
ACK
>
> I also suggest you rename 'outputs' to 'rawMounts' and move "outputs =
> None" to just above "with open(...)". After all, I think you can just
> delete this "outputs = None" because as long as it succeeds reading
> "/proc/mounts", "outputs" get a value. When it fails to open and read
> "/proc/mounts", it raises an exception and the code afterwards playing
> with outputs are not executed. So a default value for 'outputs' is not
> necessary.
ACK
>
> If you want to define a default value for "outputs" anyway, it can be an
> empty string but not "None". The reason is that when "outputs" is
> "None", "parse_cmd_output" raises exception "AttributeError" because
> "None" has no "split" method, on the other hand, when it's an empty
> string, "parse_cmd_output" does nothing and the latter code raises
> "InvalidParameter". I think in this case an "InvalidParameter" is more
> meaningful than "AttributeError".
>
>> +        if not os.path.isdir(self.path):
>> +            os.mkdir(self.path)
>> +        export_path = "%s:%s" % (
>> +            self.poolArgs['source']['host'], self.poolArgs['source']['path'])
>> +        cmd = ["mount", "-o", 'soft,timeo=100,retrans=3,retry=0',
>> +               export_path, self.path]
>> +        proc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
>> +                                stderr=subprocess.PIPE)
>> +        thread = threading.Thread(target = proc.communicate)
>> +        thread.start()
>> +        thread.join(30)
>> +
>> +        if thread.is_alive():
>> +            proc.kill()
>> +            thread.join()
>> +
>> +        with open("/proc/mounts" , "rb") as f:
>> +            outputs = f.read()
>> +        output_items = ['dev_path', 'mnt_point', 'type']
>> +        mounts = parse_cmd_output(outputs, output_items)
>> +        for item in mounts:
>> +            if 'dev_path' in item and item['dev_path'] == export_path:
>> +                res =  True
>> +        cmd = ["umount", "-f", export_path]
>> +        subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>> +
>> +        if not res:
>> +            raise InvalidParameter(
>> +                "Export path %s may block during nfs mount" % export_path)
>>
>>       @property
>>       def xml(self):
>>
>