Re: [XTF PATCH] xtf-runner: fix two synchronisation issues

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 29/07/16 13:07, Wei Liu wrote:
> There were two synchronisation issues for the old code:
>
> 1. There was no guarantee that guest console was ready before "xl
>    console" invocation.
> 2. There was no guarantee that runner wouldn't not exit before all test
>    guests were gone.

Sorry, but I can't parse this.

The runner existing before xl has torn down the guest is very
deliberate, because some part of hvm guests is terribly slow to tear
down; waiting synchronously for teardown tripled the wallclock time to
run a load of tests back-to-back.

> @@ -132,24 +141,53 @@ def list_tests(args):
>  
>          print name
>  
> +# A list of processes that we need to wait until they exits.
> +wait_list = []
>  
>  def run_test(test):
>      """ Run a specific test """
>  
> +    console_path = '/local/domain/0/backend/console'
> +    # Setup watch for console
> +    cmd = ['xenstore-watch', console_path]
> +    print "Executing '%s'" % (" ".join(cmd), )
> +    xs_watch = Popen(cmd, stdout = PIPE, stderr = PIPE)
> +
>      _, _, name = test.split('-', 2)
>  
>      cfg = path.join("tests", name, test + ".cfg")
>  
> -    cmd = ['xl', 'create', '-p', cfg]
> +    cmd = ['xl', 'create', '-Fp', cfg]
>  
>      print "Executing '%s'" % (" ".join(cmd), )
> -    rc = subproc_call(cmd)
> -    if rc:
> -        raise RunnerError("Failed to create VM")
> +    wait = Popen(cmd, stdout = DEVNULL, stderr = DEVNULL)

I would name this "create" rather than "wait"

> +    wait_list.append(wait)
> +
> +    # Wait up to 5 seconds for console to show up
> +    signal.alarm(5)
> +    while True:
> +        l = xs_watch.stdout.readline()
> +        domid = l[len(console_path)+1:].split('/')[0]
> +        if domid == '': continue

Please put continues and breaks on newlines.

if not domid:
    continue

> +
> +        cmd = ['xenstore-read', '/local/domain/'+domid+'/name']
> +        print "Executing '%s'" % (" ".join(cmd), )
> +        gname = check_output(cmd).splitlines()[0]
> +        if gname != test: continue
> +
> +        cmd = ['xenstore-read', '/local/domain/'+domid+'/console/tty']
> +        print "Executing '%s'" % (" ".join(cmd), )
> +        con = check_output(cmd).splitlines()[0]
> +        if con != '': break

Somewhere in this loop, you should poll the call to xl create.  In the
case that there is an xl configuration error, this setup will wait for 5
seconds, then discard all trace of the error.

> +
> +    # Tear down watch and timer
> +    signal.alarm(0)
> +    xs_watch.kill()
>  
>      cmd = ['xl', 'console', test]
>      print "Executing '%s'" % (" ".join(cmd), )
>      console = Popen(cmd, stdout = PIPE)
> +    wait_list.append(console)
>  
>      cmd = ['xl', 'unpause', test]
>      print "Executing '%s'" % (" ".join(cmd), )
> @@ -327,12 +365,17 @@ def main():
>      opts, args = parser.parse_args()
>  
>      if opts.list_tests:
> -        return list_tests(args)
> +        ret = list_tests(args)
>      else:
> -        return run_tests(args)
> +        ret = run_tests(args)
> +
> +    for child in wait_list: child.wait()

On a newline please.

You should use stdout=PIPE, stderr=STDOUT (to link stdout and stderr to
the same fd) and .communicate() to get the results.  In any case that a
child exists non-zero, you mustn't discard the error.  As a result, I
don't think you need DEVNULL any more.

~Andrew

> +
> +    return ret
>  
>  
>  if __name__ == "__main__":
> +    signal.signal(signal.SIGALRM, sigalrm_handler)
>      try:
>          sys.exit(main())
>      except RunnerError, e:


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel