[PATCH] Experimental NFS export support V2

[Tony Asleson]

V2 - use variable instead of positional

Signed-off-by: Tony Asleson <tasleson(a)redhat.com&gt;
---
 targetd/fs.py   |  75 ++++++++++++++++--
 targetd/main.py |  18 +++--
 targetd/nfs.py  | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 320 insertions(+), 13 deletions(-)
 create mode 100644 targetd/nfs.py

diff --git a/targetd/fs.py b/targetd/fs.py
index a6fd9b9..63080d3 100644
--- a/targetd/fs.py
+++ b/targetd/fs.py
@@ -20,6 +20,7 @@ import os
 import time
 from subprocess import Popen, PIPE
 from main import TargetdError
+from nfs import Nfs, Export
 
 
 # Notes:
@@ -46,6 +47,7 @@ fs_cmd = 'btrfs'
 
 pools = []
 
+
 def initialize(config_dict):
 
     global pools
@@ -67,8 +69,11 @@ def initialize(config_dict):
         fs_clone=fs_clone,
         ss_list=ss,
         fs_snapshot=fs_snapshot,
-        fs_snapshot_delete=fs_snapshot_delete
-        )
+        fs_snapshot_delete=fs_snapshot_delete,
+        nfs_export_auth_list=nfs_export_auth_list,
+        nfs_export_list=nfs_export_list,
+        nfs_export_add=nfs_export_add,
+        nfs_export_remove=nfs_export_remove)
 
 
 def invoke(cmd, raise_exception=True):
@@ -194,8 +199,8 @@ def fs_pools(req):
     return results
 
 
-def fs(req):
-    fs_list = []
+def _fs_hash():
+    fs_list = {}
 
     for pool in pools:
         full_path = os.path.join(pool, fs_path)
@@ -210,9 +215,11 @@ def fs(req):
                 if len(data):
                     (total, free) = fs_space_values(full_path)
                     for e in data:
-                        fs_list.append(dict(name=e[10], uuid=e[8],
-                                       total_space=total, free_space=free,
-                                       pool=pool))
+                        sub_vol = e[10]
+                        key = full_path + '/' + sub_vol
+                        fs_list[key] = dict(name=sub_vol, uuid=e[8],
+                                            total_space=total, free_space=free,
+                                            pool=pool, full_path=key)
                 break
             elif result == 19:
                 time.sleep(1)
@@ -223,6 +230,11 @@ def fs(req):
     return fs_list
 
 
+def fs(req):
+    fs_hash = _fs_hash()
+    return fs_hash.values()
+
+
 def ss(req, fs_uuid, fs_cache=None):
     snapshots = []
 
@@ -293,3 +305,52 @@ def fs_clone(req, fs_uuid, dest_fs_name, snapshot_id):
         raise TargetdError(-51, "Filesystem with that name exists")
 
     invoke([fs_cmd, 'subvolume', 'snapshot', source, dest])
+
+
+def nfs_export_auth_list(req):
+    return Nfs.security_options()
+
+
+def nfs_export_list(req):
+    rc = []
+    fs_hash = _fs_hash()
+
+    exports = Nfs.exports()
+    for e in exports:
+        #Only report those exports which match our filesystem layout
+        if e.path in fs_hash:
+            rc.append(dict(host=e.host, path=e.path, options=e.options_list(),
+                           fs_uuid=fs_hash[e.path]['uuid']))
+    return rc
+
+
+def nfs_export_add(req, host, path, export_path, options):
+
+    if export_path is not None:
+        raise TargetdError(-401, "separate export path not supported at "
+                                 "this time")
+    bit_opt = 0
+    key_opt = {}
+
+    for o in options:
+        if '=' in o:
+            k, v = o.split('=')
+            key_opt[k] = v
+        else:
+            bit_opt |= Export.bool_option[o]
+
+    print 'Calling: %s - %s - %x %s' % (host, path, bit_opt, str(key_opt))
+
+    Nfs.export_add(host, path, bit_opt, key_opt)
+
+
+def nfs_export_remove(req, host, path):
+    exports = Nfs.exports()
+
+    for e in exports:
+        if e.host == host and e.path == path:
+            Nfs.export_remove(e)
+            return None
+
+    raise TargetdError(-400, "NFS export to remove not found %s:%s",
+                       (host, path))
\ No newline at end of file
diff --git a/targetd/main.py b/targetd/main.py
index 1082f5f..16173df 100644
--- a/targetd/main.py
+++ b/targetd/main.py
@@ -27,12 +27,14 @@ import yaml
 import itertools
 import socket
 import ssl
+import traceback
+import sys
 
 default_config_path = "/etc/target/targetd.yaml"
 
 default_config = dict(
-    block_pools = ['vg-targetd'],
-    fs_pools = [],
+    block_pools=['vg-targetd'],
+    fs_pools=[],
     user="admin",
     # security: no default password
     target_name="iqn.2003-01.org.linux-iscsi.%s:targetd" %
socket.gethostname(),
@@ -43,6 +45,7 @@ default_config = dict(
 
 config = {}
 
+
 class TargetdError(Exception):
     def __init__(self, error_code, message, *args, **kwargs):
         Exception.__init__(self, *args, **kwargs)
@@ -80,17 +83,18 @@ async_id = 100
 class TargetHandler(BaseHTTPRequestHandler):
 
     def _new_async_id(self):
+        global async_id
         with async_id_lock:
             new_id = async_id
             async_id += 1
         return new_id
 
     def mark_async(self):
-        '''
+        """
         Mark a request as finishing after the given HTTP request returns.
 
         Handlers calling this must
-        '''
+        """
         if not self.async_id:
             self.async_id = self._new_async_id()
             rpcdata = json.dumps(
@@ -122,7 +126,6 @@ class TargetHandler(BaseHTTPRequestHandler):
                 if not code:
                     del long_op_status[self.async_id]
 
-
     def log_request(self, code='-', size='-'):
         # override base class - don't log good requests
         pass
@@ -182,9 +185,11 @@ class TargetHandler(BaseHTTPRequestHandler):
                     result = mapping[method](self)
             except KeyError:
                 error = (-32601, "method %s not found" % method)
+                traceback.print_exc(file=sys.stdout)
                 raise
             except TypeError:
-                error = (-32602, "invalid method parameter(s)")
+                error = (-32602, "invalid method parameter(s) %s" %
str(params))
+                traceback.print_exc(file=sys.stdout)
                 raise
             except TargetdError, td:
                 error = (td.error, td.msg)
@@ -207,6 +212,7 @@ class TargetHandler(BaseHTTPRequestHandler):
 class ThreadedHTTPServer(ThreadingMixIn, HTTPServer, object):
     """Handle requests in a separate thread."""
 
+
 class TLSThreadedHTTPServer(ThreadedHTTPServer):
     """Also use TLS to encrypt the connection"""
 
diff --git a/targetd/nfs.py b/targetd/nfs.py
new file mode 100644
index 0000000..25ee8f0
--- /dev/null
+++ b/targetd/nfs.py
@@ -0,0 +1,240 @@
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 3 of the
+# License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>;.
+
+from subprocess import Popen, PIPE
+import re
+
+
+def invoke(cmd, raise_exception=True):
+    """
+    Exec a command returning a tuple (exit code, stdout, stderr) and optionally
+    throwing an exception on non-zero exit code.
+    """
+    c = Popen(cmd, stdout=PIPE, stderr=PIPE)
+    out = c.communicate()
+
+    if raise_exception:
+        if c.returncode != 0:
+            cmd_str = str(cmd)
+            raise RuntimeError('Unexpected exit code "%s" %s, out= %s'
%
+                               (cmd_str, str(c.returncode),
+                                str(out[0] + out[1])))
+
+    return c.returncode, out[0], out[1]
+
+
+def make_line_array(out):
+    """
+    Split the text out as an array of text strings
+    """
+    rc = []
+    for line in out.split('\n'):
+        if len(line) > 1:
+            rc.append(line)
+    return rc
+
+
+class Export(object):
+
+    SECURE = 0x00000001
+    RW = 0x00000002
+    RO = 0x00000004
+    SYNC = 0x00000008
+    ASYNC = 0x00000010
+    NO_WDELAY = 0x00000020
+    NOHIDE = 0x00000040
+    CROSS_MNT = 0x00000080
+    NO_SUBTREE_CHECK = 0x00000100
+    INSECURE_LOCKS = 0x00000200
+    ROOT_SQUASH = 0x00000400
+    NO_ROOT_SQUASH = 0x00000800
+    ALL_SQUASH = 0x00001000
+    WDELAY = 0x00002000
+    HIDE = 0x00004000
+    INSECURE = 0x00008000
+
+    bool_option = dict(secure=SECURE, rw=RW, ro=RO, sync=SYNC, async=ASYNC,
+                       no_wdelay=NO_WDELAY, nohide=NOHIDE,
+                       cross_mnt=CROSS_MNT, no_subtree_check=NO_SUBTREE_CHECK,
+                       insecure_locks=INSECURE_LOCKS, root_squash=ROOT_SQUASH,
+                       all_squash=ALL_SQUASH, wdelay=WDELAY, hide=HIDE,
+                       insecure=INSECURE, no_root_squash=NO_ROOT_SQUASH)
+
+    key_pair = dict(mountpoint=str, mp=str, fsid=None, refer=str, replicas=str,
+                    anonuid=int, anongid=int)
+
+    export_regex = '([\/a-zA-Z0-9\.-_]+)[\s]+(.+)\((.+)\)'
+
+    @staticmethod
+    def _bitCount(int_type):
+        count = 0
+        while int_type:
+            int_type &= int_type - 1
+            count += 1
+        return count
+
+    @staticmethod
+    def _validate_options(options):
+
+        if Export._bitCount(((Export.RW | Export.RO) & options)) == 2:
+            raise ValueError("Both RO & RW set")
+
+        if Export._bitCount(((Export.INSECURE | Export.SECURE) & options)) == 2:
+            raise ValueError("Both INSECURE & SECURE set")
+
+        if Export._bitCount(((Export.SYNC | Export.ASYNC) & options)) == 2:
+            raise ValueError("Both SYNC & ASYNC set")
+
+        if Export._bitCount(((Export.HIDE | Export.NOHIDE) & options)) == 2:
+            raise ValueError("Both HIDE & NOHIDE set")
+
+        if Export._bitCount(((Export.WDELAY | Export.NO_WDELAY) & options)) \
+                == 2:
+            raise ValueError("Both WDELAY & NO_WDELAY set")
+
+        if Export._bitCount(((Export.ROOT_SQUASH | Export.NO_ROOT_SQUASH)
+                            & options)) > 1:
+            raise ValueError("Only one option of ROOT_SQUASH, NO_ROOT_SQUASH,
"
+                             "can be specified")
+
+        return options
+
+    @staticmethod
+    def _validate_key_pairs(kp):
+        if kp:
+            if isinstance(kp, dict):
+                for k, v in kp.items():
+                    if k not in Export.key_pair:
+                        raise ValueError('option %s not valid' % k)
+
+                return kp
+            else:
+                raise ValueError('key_value_options domain is None or dict')
+        else:
+            return {}
+
+    def __init__(self, host, path, bit_wise_options=0, key_value_options=None):
+
+        if host == '<world>':
+            self.host = '*'
+        else:
+            self.host = host
+        self.path = path
+        self.options = Export._validate_options(bit_wise_options)
+        self.key_value_options = Export._validate_key_pairs(key_value_options)
+
+    @staticmethod
+    def parse_opt(options_string):
+        bits = 0
+        pairs = {}
+
+        options = options_string.split(',')
+        for o in options:
+            if '=' in o:
+                #We have a key=value
+                key, value = o.split('=')
+                pairs[key] = value
+            else:
+                bits |= Export.bool_option[o]
+
+        return bits, pairs
+
+    @staticmethod
+    def parse(export_text):
+        rc = []
+        pattern = re.compile(Export.export_regex)
+
+        for m in re.finditer(pattern, export_text):
+            rc.append(Export(m.group(2), m.group(1),
+                             *Export.parse_opt(m.group(3))))
+        return rc
+
+    @staticmethod
+    def _append(s, a):
+        if len(s):
+            s = s + "," + a
+        else:
+            s = a
+        return s
+
+    def options_list(self):
+        rc = []
+        for k, v in self.bool_option.items():
+            if self.options & v:
+                rc.append(k)
+
+        for k, v in self.key_value_options.items():
+            rc.append('%s=%s' % (k, v))
+
+        return rc
+
+    def options_string(self):
+        return ','.join(self.options_list())
+
+    def __repr__(self):
+        return "%s%s(%s)" % (self.path.ljust(50), self.host,
+                             self.options_string())
+
+
+class Nfs(object):
+    """
+    Python module for configuring NFS exports
+    """
+    cmd = 'exportfs'
+
+    def __init__(self):
+        pass
+
+    @staticmethod
+    def security_options():
+        return "sys", "krb5", "krb5i", "krb5p"
+
+    @staticmethod
+    def exports():
+        """
+        Return list of exports
+        """
+        rc = []
+        ec, out, error = invoke([Nfs.cmd,  '-v'])
+        rc = Export.parse(out)
+        return rc
+
+    @staticmethod
+    def export_add(host, path, bit_wise_options, key_value_options):
+        """
+        Adds a path as an NFS export
+        """
+        export = Export(host, path, bit_wise_options, key_value_options)
+        options = export.options_string()
+
+        cmd = [Nfs.cmd]
+
+        if len(options):
+            cmd.extend(['-o', options])
+
+        cmd.extend(['%s:%s' % (host, path)])
+
+        ec, out, err = invoke(cmd, False)
+        if ec == 0:
+            return None
+        elif ec == 22:
+            raise ValueError("Invalid option: %s" % err)
+        else:
+            raise RuntimeError('Unexpected exit code "%s" %s, out= %s'
%
+                               (str(cmd), str(ec),
+                                str(out + ":" + err)))
+
+    @staticmethod
+    def export_remove(export):
+        ec, out, err = invoke([Nfs.cmd, '-u', '%s:%s' %
+                                              (export.host, export.path)])
\ No newline at end of file
-- 
1.8.2.1

[Andy Grover]

Hi Tony, wow a lot of work here. Comments inline.

On 05/21/2013 04:05 PM, Tony Asleson wrote:
>  V2 - use variable instead of positional 
Can you be a little more verbose? :)

>  @@ -67,8 +69,11 @@ def initialize(config_dict):
>            fs_clone=fs_clone,
>            ss_list=ss,
>            fs_snapshot=fs_snapshot,
>  -        fs_snapshot_delete=fs_snapshot_delete
>  -        )
>  +        fs_snapshot_delete=fs_snapshot_delete,
>  +        nfs_export_auth_list=nfs_export_auth_list,
>  +        nfs_export_list=nfs_export_list,
>  +        nfs_export_add=nfs_export_add,
>  +        nfs_export_remove=nfs_export_remove) 
If you leave the ) on its own line then when we add more, the previous 
line doesn't have to be changed. Also we can put a , after every entry, 
even the last one, for the same reason.

>  +def fs(req):
>  +    fs_hash = _fs_hash()
>  +    return fs_hash.values() 
just return _fs_hash().values()?

>  +    print 'Calling: %s - %s - %x %s' % (host, path, bit_opt,
> str(key_opt)) 
debug left in?

>  +def nfs_export_remove(req, host, path):
>  +    exports = Nfs.exports()
>  +
>  +    for e in exports: 
a little shorter to just say 'for e in Nfs.exports():'

>  +        if e.host == host and e.path == path:
>  +            Nfs.export_remove(e)
>  +            return None
>  +
>  +    raise TargetdError(-400, "NFS export to remove not found %s:%s",
>  +                       (host, path))
>  \ No newline at end of file 
better to have the conditional, if true, raise an exception. Then you 
just fall off the end if everything's good, which implicitly returns None.

>  diff --git a/targetd/main.py b/targetd/main.py
>  index 1082f5f..16173df 100644
>  --- a/targetd/main.py
>  +++ b/targetd/main.py
>  @@ -27,12 +27,14 @@ import yaml
>    import itertools
>    import socket
>    import ssl
>  +import traceback
>  +import sys
>
>    default_config_path = "/etc/target/targetd.yaml"
>
>    default_config = dict(
>  -    block_pools = ['vg-targetd'],
>  -    fs_pools = [],
>  +    block_pools=['vg-targetd'],
>  +    fs_pools=[], 
Break whitespace changes out into a separate changeset

>  +
>    class TargetdError(Exception):
>        def __init__(self, error_code, message, *args, **kwargs):
>            Exception.__init__(self, *args, **kwargs)
>  @@ -80,17 +83,18 @@ async_id = 100
>    class TargetHandler(BaseHTTPRequestHandler):
>
>        def _new_async_id(self):
>  +        global async_id
>            with async_id_lock:
>                new_id = async_id
>                async_id += 1
>            return new_id 
???

>  -        '''
>  +        """
>            Mark a request as finishing after the given HTTP request returns.
>
>            Handlers calling this must
>  -        '''
>  +        """ 
Separate changeset

>            if not self.async_id:
>                self.async_id = self._new_async_id()
>                rpcdata = json.dumps(
>  @@ -122,7 +126,6 @@ class TargetHandler(BaseHTTPRequestHandler):
>                    if not code:
>                        del long_op_status[self.async_id]
>
>  -
>        def log_request(self, code='-', size='-'):
>            # override base class - don't log good requests
>            pass
>  @@ -182,9 +185,11 @@ class TargetHandler(BaseHTTPRequestHandler):
>                        result = mapping[method](self)
>                except KeyError:
>                    error = (-32601, "method %s not found" % method)
>  +                traceback.print_exc(file=sys.stdout)
>                    raise
>                except TypeError:
>  -                error = (-32602, "invalid method parameter(s)")
>  +                error = (-32602, "invalid method parameter(s) %s" %
> str(params))
>  +                traceback.print_exc(file=sys.stdout)
>                    raise 
Separate changeset

>  diff --git a/targetd/nfs.py b/targetd/nfs.py
>  new file mode 100644
>  index 0000000..25ee8f0
>  --- /dev/null
>  +++ b/targetd/nfs.py
>  \ No newline at end of file
>  
All looks good. Just plz add a newline :)

Regards -- Andy

[Tony Asleson]

On 05/22/2013 02:55 PM, Andy Grover wrote:
>  Hi Tony, wow a lot of work here. Comments inline.
>  
>  On 05/21/2013 04:05 PM, Tony Asleson wrote:
> > V2 - use variable instead of positional
>  
>  Can you be a little more verbose? :) 
Sorry, if you diff the diffs it's apparent, I will be more wordy in the
future :-)

> > @@ -67,8 +69,11 @@ def initialize(config_dict):
> >           fs_clone=fs_clone,
> >           ss_list=ss,
> >           fs_snapshot=fs_snapshot,
> > -        fs_snapshot_delete=fs_snapshot_delete
> > -        )
> > +        fs_snapshot_delete=fs_snapshot_delete,
> > +        nfs_export_auth_list=nfs_export_auth_list,
> > +        nfs_export_list=nfs_export_list,
> > +        nfs_export_add=nfs_export_add,
> > +        nfs_export_remove=nfs_export_remove)
>  
>  If you leave the ) on its own line then when we add more, the previous
>  line doesn't have to be changed. Also we can put a , after every entry,
>  even the last one, for the same reason. 
Good idea

>  
> > +def fs(req):
> > +    fs_hash = _fs_hash()
> > +    return fs_hash.values()
>  
>  just return _fs_hash().values()?
>  
> > +    print 'Calling: %s - %s - %x %s' % (host, path, bit_opt,
> > str(key_opt))
>  
>  debug left in? 
Yep, I will remove

>  
> > +def nfs_export_remove(req, host, path):
> > +    exports = Nfs.exports()
> > +
> > +    for e in exports:
>  
>  a little shorter to just say 'for e in Nfs.exports():' 
Will fix

>  
> > +        if e.host == host and e.path == path:
> > +            Nfs.export_remove(e)
> > +            return None
> > +
> > +    raise TargetdError(-400, "NFS export to remove not found %s:%s",
> > +                       (host, path))
> > \ No newline at end of file
>  
>  better to have the conditional, if true, raise an exception. Then you
>  just fall off the end if everything's good, which implicitly returns None. 
Will fix

>  
> > diff --git a/targetd/main.py b/targetd/main.py
> > index 1082f5f..16173df 100644
> > --- a/targetd/main.py
> > +++ b/targetd/main.py
> > @@ -27,12 +27,14 @@ import yaml
> >   import itertools
> >   import socket
> >   import ssl
> > +import traceback
> > +import sys
> >
> >   default_config_path = "/etc/target/targetd.yaml"
> >
> >   default_config = dict(
> > -    block_pools = ['vg-targetd'],
> > -    fs_pools = [],
> > +    block_pools=['vg-targetd'],
> > +    fs_pools=[],
>  
>  Break whitespace changes out into a separate changeset 
Sorry, editor does this automatically.  Will place in separate.

>  
> > +
> >   class TargetdError(Exception):
> >       def __init__(self, error_code, message, *args, **kwargs):
> >           Exception.__init__(self, *args, **kwargs)
> > @@ -80,17 +83,18 @@ async_id = 100
> >   class TargetHandler(BaseHTTPRequestHandler):
> >
> >       def _new_async_id(self):
> > +        global async_id
> >           with async_id_lock:
> >               new_id = async_id
> >               async_id += 1
> >           return new_id
>  
>  ??? 
My editor was flagging this as an error with its static analysis feature.

Run this snippet below and see what you get.  Only works if I use the
global flag which is currently commented out.  Otherwise I get an
exception:

UnboundLocalError: local variable 'g_var' referenced before assignment

g_var = 100

class foo(object):
    def testing(self):
        #global g_var
        l_var = g_var
        g_var += 1
        return l_var

f = foo()

print 'f.testing() = ', f.testing()
print 'g_var = ', g_var

> > -        '''
> > +        """
> >           Mark a request as finishing after the given HTTP request
> > returns.
> >
> >           Handlers calling this must
> > -        '''
> > +        """
>  
>  Separate changeset 
Sorry, editor does this automatically.  Will place in separate.

>  
> >           if not self.async_id:
> >               self.async_id = self._new_async_id()
> >               rpcdata = json.dumps(
> > @@ -122,7 +126,6 @@ class TargetHandler(BaseHTTPRequestHandler):
> >                   if not code:
> >                       del long_op_status[self.async_id]
> >
> > -
> >       def log_request(self, code='-', size='-'):
> >           # override base class - don't log good requests
> >           pass
> > @@ -182,9 +185,11 @@ class TargetHandler(BaseHTTPRequestHandler):
> >                       result = mapping[method](self)
> >               except KeyError:
> >                   error = (-32601, "method %s not found" % method)
> > +                traceback.print_exc(file=sys.stdout)
> >                   raise
> >               except TypeError:
> > -                error = (-32602, "invalid method parameter(s)")
> > +                error = (-32602, "invalid method parameter(s) %s" %
> > str(params))
> > +                traceback.print_exc(file=sys.stdout)
> >                   raise
>  
>  Separate changeset 
Actually this was additional debug.  Would you like to leave it in?
Unfortunately if the method throws an exception of these types you
incorrectly look at the wire API call instead of the method implementation.

> > diff --git a/targetd/nfs.py b/targetd/nfs.py
> > new file mode 100644
> > index 0000000..25ee8f0
> > --- /dev/null
> > +++ b/targetd/nfs.py
> > \ No newline at end of file
> >
>  
>  All looks good. Just plz add a newline :) 
:-)

Thanks for the review!  I will send updated patch set.

-Tony

[Andy Grover]

On 05/22/2013 01:21 PM, Tony Asleson wrote:
> > ???
>
>  My editor was flagging this as an error with its static analysis feature.
>
>  Run this snippet below and see what you get.  Only works if I use the
>  global flag which is currently commented out.  Otherwise I get an
>  exception:
>
>  UnboundLocalError: local variable 'g_var' referenced before assignment
>
>  g_var = 100
>
>  class foo(object):
>       def testing(self):
>           #global g_var
>           l_var = g_var
>           g_var += 1
>           return l_var
>
>  f = foo()
>
>  print 'f.testing() = ', f.testing()
>  print 'g_var = ', g_var 
Yup.

Sorry for being so stubborn and also wrong the whole time.

-- Andy