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

[Zhou Zheng Sheng]

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:".

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.

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):
> 


-- 
Thanks and best regards!

Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397