Merge of qemu-datapath into REQ477/master branch #3522

gaborigloi · 2018-03-19T11:00:19Z

Signed-off-by: Gabor Igloi gabor.igloi@citrix.com

mseri · 2018-03-19T11:16:12Z

ocaml/xapi/xapi_vbd.ml

-         let path = String.sub params prefix_len (String.length params - prefix_len) in
-         Db.VBD.set_device ~__context ~self ~value:path;
+         let device_path =
+           let nbd_prefix = "hack|nbd:unix:" in


We will come up with a proper design, but for now, the strategy is to make sure the new code is isolated, and this "hack|..." method is hopefully good enough for that.

mseri · 2018-03-19T11:16:56Z

ocaml/xapi/xapi_vbd.ml

+           let is_nbd = String.startswith nbd_prefix params in
+           if is_nbd then begin
+             let export_name = "qemu_node" in
+             let unix_socket_path = params |> Astring.String.cuts ~empty:false ~sep:nbd_prefix |> List.hd |> String.split_on_char '|' |> List.hd in


This will raise an horrible looking and hard to debug error if there are empty lists on the way

Yes, but it's isolated to the qemu-datapath branch

Maybe I could add a debug message above this with the content of params, so that we now what we got in case one of the List.hds fail?
Or is there a concise way of doing something like List.hd but with nice error messages?

I think adding a preliminary debug with the params could be enough.
The only thing with List.hd is probably to define your own hd_log as something like

let hd_log logline = function | h :: _ -> h | [] -> debug logline; raise Not_found (* or whatever else List.hd raises *)

Thanks, I've added a debug line.

mseri · 2018-03-19T11:18:32Z

scripts/nbd_client_manager.py

+        self._lock_file = open(self._path, 'w+')
+        fcntl.flock(self._lock_file, flags)
+
+    def unlock(self):


if you add __enter__ and __exit__ as alias of lock and unlock you can use with FILE_LOCK: below

Thanks, that's a great idea, I'll add those :)

mseri · 2018-03-19T11:22:37Z

scripts/nbd_client_manager.py

+import fcntl
+
+
+LOGGER = logging.getLogger("nbd_client_manager")


I think this logs at warning level by default. Maybe adding a --verbose or --level to set the level to debug on demand could be useful if you want to read the debug logs below as well

I think so, good spot, I'll increase the logging level to debug.

👍 There is a LOGGER.setLogLevel() call for that

I think we need to do more configuration to redirect log messages into syslog using SysLogHandler.

mseri · 2018-03-19T11:38:51Z

scripts/nbd_client_manager.py


+    def __enter__(self):
+        self._lock()
+        return self


You can remove the return if you don't need to do anything to the returned object. Returning None is the default for functions and for context managers, and it is what happens if you omit the return

Thanks, done

gaborigloi · 2018-03-19T13:04:40Z

Should I also add this nbd_client_manager.py to xapi_globs as a nonessential executable?
xcp-idl does close to nothing for nonessential resources btw (only checks for duplicate keys), so it's essentially a no-op, the only advantage is that we declare the path to this script in the traditional place.

mseri · 2018-03-19T13:07:57Z

I don't know. Ping @jonludlam

gaborigloi · 2018-03-19T14:03:18Z

Now logging should work fine:

[root@host ~]# python nbd_client_manager.py connect --path asdf --exportname fjdksl     
Traceback (most recent call last):          
  File "nbd_client_manager.py", line 199, in <module>                                   
    args.func(args)   
  File "nbd_client_manager.py", line 154, in _connect_cli                               
    device = connect_nbd(path=args.path, exportname=args.exportname)                    
  File "nbd_client_manager.py", line 133, in connect_nbd                                
    nbd_device = _find_unused_nbd_device()  
  File "nbd_client_manager.py", line 116, in _find_unused_nbd_device                    
    if not _is_nbd_device_connected(nbd_device=nbd_device):                             
  File "nbd_client_manager.py", line 101, in _is_nbd_device_connected                   
    returncode = _call(cmd, error=False)    
  File "nbd_client_manager.py", line 71, in _call                                       
    close_fds=True    
  File "/usr/lib64/python2.7/subprocess.py", line 711, in __init__                      
    errread, errwrite)                      
  File "/usr/lib64/python2.7/subprocess.py", line 1327, in _execute_child               
    raise child_exception                   
OSError: [Errno 2] No such file or directory                                            
[root@host ~]#

Mar 19 14:01:47 host Running cmd ['modprobe', 'nbd']                                    
Mar 19 14:01:47 host Running cmd ['nbd-client', '-check', '/dev/nbd0']                  
Mar 19 14:01:47 host [Errno 2] No such file or directory#012Traceback (most recent call last):#012  File "nbd_client_manager.py", line 199, in <module>#012    args.func(args)#012  File "nbd_client_manager.py", line 154, in _connect_cli#012    device = connect_nbd(path=args.path, exportname=args.exportname)#012  File "nbd_client_manager.py", line 133, in connect_nbd#012    nbd_device = _find_unused_nbd_device()#012  File "nbd_client_manager.py", line 116, in _find_unused_nbd_device#012    if not _is_nbd_device_connected(nbd_device=nbd_device):#012  File "nbd_client_manager.py", line 101, in _is_nbd_device_connected#012    returncode = _call(cmd, error=False)#012  File "nbd_client_manager.py", line 71, in _call#012    close_fds=True#012  File "/usr/lib64/python2.7/subprocess.py", line 711, in __init__#012    errread, errwrite)#012  File "/usr/lib64/python2.7/subprocess.py", line 1327, in _execute_child#012    raise child_exception#012OSError: [Errno 2] No such file or directory

gaborigloi · 2018-03-19T14:23:44Z

I've decided to add it to Xapi_globs.

gaborigloi · 2018-03-19T14:28:06Z

Thanks for the review, I think I've addressed all the comments. I'll squash the fixes and merge if this is ok now.

gaborigloi · 2018-03-19T14:28:39Z

ocaml/xapi/xapi_vbd.ml

-         Db.VBD.set_device ~__context ~self ~value:path;
+         let device_path =
+           let nbd_prefix = "hack|nbd:unix:" in
+           let is_nbd = String.startswith nbd_prefix params in


I should use Astring here too :/

As you wish. The main xapi is still not ported to Astring at all, feel free to use Xstringext here. If you use Astrig, you will porbably need to add it to the jbuiild and opam files as well

I've done it now

We invoke the nbd_client_manager.py script for VBD.plug/unplug and in the static-vdis HA script in case it's needed. Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>

edwintorok · 2018-03-22T11:32:13Z

Rebased on latest master

coveralls · 2018-03-22T11:50:56Z

Coverage decreased (-0.009%) to 19.143% when pulling 8d776e8 on feature/qemu-datapath into c1555d6 on feature/REQ477/master.

mseri reviewed Mar 19, 2018

View reviewed changes

gaborigloi force-pushed the feature/qemu-datapath branch from 9dd9217 to 14f9855 Compare March 19, 2018 11:40

mseri approved these changes Mar 19, 2018

View reviewed changes

gaborigloi force-pushed the feature/qemu-datapath branch from 269ab99 to f997e17 Compare March 19, 2018 14:24

gaborigloi commented Mar 19, 2018

View reviewed changes

mseri approved these changes Mar 19, 2018

View reviewed changes

gaborigloi force-pushed the feature/qemu-datapath branch from 8c013f1 to f8e39b3 Compare March 19, 2018 14:56

gaborigloi mentioned this pull request Mar 19, 2018

Merge of qemu-datapath into REQ477/master branch xapi-project/xenopsd#483

Merged

mseri approved these changes Mar 21, 2018

View reviewed changes

edwintorok force-pushed the feature/REQ477/master branch from 4cce019 to c1555d6 Compare March 22, 2018 11:00

Start/stop nbd-client for qemu datapath using a new script

8d776e8

We invoke the nbd_client_manager.py script for VBD.plug/unplug and in the static-vdis HA script in case it's needed. Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>

edwintorok force-pushed the feature/qemu-datapath branch from f8e39b3 to 8d776e8 Compare March 22, 2018 11:31

edwintorok merged commit f372f74 into feature/REQ477/master Mar 22, 2018

		import fcntl


		LOGGER = logging.getLogger("nbd_client_manager")

Merge of qemu-datapath into REQ477/master branch #3522

Merge of qemu-datapath into REQ477/master branch #3522

Uh oh!

Conversation

gaborigloi commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborigloi Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborigloi Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborigloi commented Mar 19, 2018

Uh oh!

mseri commented Mar 19, 2018

Uh oh!

gaborigloi commented Mar 19, 2018

Uh oh!

gaborigloi commented Mar 19, 2018

Uh oh!

gaborigloi commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edwintorok commented Mar 22, 2018

Uh oh!

coveralls commented Mar 22, 2018

Uh oh!

Uh oh!

gaborigloi Mar 19, 2018 •

edited

Loading

gaborigloi Mar 19, 2018 •

edited

Loading

mseri Mar 19, 2018 •

edited

Loading

mseri Mar 19, 2018 •

edited

Loading

mseri Mar 19, 2018 •

edited

Loading

mseri Mar 19, 2018 •

edited

Loading

mseri Mar 19, 2018 •

edited

Loading