Re: [Xen-devel] [XTF PATCH v2] xtf-runner: use xl create -Fc directly

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/08/16 12:01, Wei Liu wrote:
> On Tue, Aug 09, 2016 at 04:37:40PM +0100, Wei Liu wrote:
>> On Tue, Aug 09, 2016 at 02:11:27PM +0100, Andrew Cooper wrote:
>>> On 08/08/16 14:13, Wei Liu wrote:
>>>> On Mon, Aug 08, 2016 at 02:06:37PM +0100, Andrew Cooper wrote:
>>>>> On 08/08/16 12:24, Wei Liu wrote:
>>>>>> Now that xl create -c is fixed in xen-unstable, there is no need to keep
>>>>>> the hack to get guest console output anymore.
>>>>>>
>>>>>> Use xl create -Fc directly, then wait for the xl process to exit.  Print
>>>>>> any error as it occurs.
>>>>>>
>>>>>> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>>>>> Sadly, now I think about this further, it does re-introduce the
>>>>> serialisation problem I was trying specifically trying to avoid.
>>>>>
>>>> Can you give an example of the race you wanted to avoid?
>>>>
>>>> I thought with the xenconsole work in place I had solved all races I was
>>>> aware of, but maybe I missed something obvious.
>>> It isn't a race.  I do not want synchronously wait for the taredown of
>>> domains to complete, as it adds unnecessary latency when running more
>>> than a single test.
>>>
>>>>> You need to run `xl create -F` so you can sensibly wait on the create
>>>>> list to avoid tripping up the leak detection.
>>>>>
>>>>> However, the guest.communicate() call will wait for the guest process to
>>>>> terminate, which includes all output.
>>>>>
>>>> Is there a problem with that?
>>> It makes your "for child in child_list" useless, as all processes have
>>> guareteed to exit already.
>>>
>>>>> Therefore, I think we still need the `xl create -Fp`, `xl console`, `xl
>>>>> unpause` dance, where the create process gets put on the create_list,
>> There are several issues with this.
>>
>> Let's go back to the original `xl create -p`, `xl console` and `xl
>> unpause` first.  This sequence is not race-free. 
>>
>> xl create -p can race with xl console. That, however, is mitigated by
>> xl console waiting for 5s for tty node to show up and the use of
>> subproc_call in runner.
>>
>> xl unpause can race with xl console (Popen doesn't wait for process to
>> finish). The domain can be unpaused and exit before xl console gets a
>> chance to attach. This would cause random failure in osstest.
>>
>> See
>>
>> http://osstest.xs.citrite.net/~osstest/testlogs/logs/66926/test-xtf-amd64-amd64-2/info.html
>> http://osstest.xs.citrite.net/~osstest/testlogs/logs/66926/test-xtf-amd64-amd64-2/gall-mite---var-log-xen-console-guest-test-hvm32-fep.log
>>
>> My theory is that the failure in fep test is due to the second race.
>> Unfortunately I can't reproduce that locally at the moment.
>>
>> Then let's look at `xl create -Fp`, `xl console` and `xl unpause`
>> sequence. `xl create -Fp` doesn't return until the guest vanishes, so
>> that means we need to use Popen to run it. The leaves us even less clue
>> to tell when to execute `xl console`. The first race can only become
>> worse. The second race still exists.
>>
>> If you want to retain this trick. I'm afraid what you need is more or
>> less my very first patch.
>>
>>>>> and it is the console process which gets communicated with.
>>>>>
>>>>> This also has the advantage that it doesn't cause ./xtf-runner to break
>>>>> against all non-staging trees.
>>>>>
>>>> I thought we decided to grep log file for that?
>>> Right, but until that happens, this patch constitutes a functional
>>> regression.
>>>
>> Hmm... I think we're talking past each other. Let me clarify.
>>
>> There are two distinct use cases: used by a human vs used by an
>> automated test system like osstest.
>>
>> For osstest this is not a functional regression because that function is
>> not reliable in the first place. We would need grepping log file to fix
>> the issue.
>>
>> For a human, because he or she doesn't care about 1 in 50 random
>> failure, this is a functional regression. It's still something that
>> should be fixed (and can be fixed) by grepping log file, but obviously
>> less important.
>>
>> I don't want to mix these two use cases. This patch is indeed biased
>> towards the automated test system. I think I can retain the original
>> function and test the version of Xen it runs on to decide which version
>> to use.
>>
>> FAOD my impression is that you have strong opinions on how log grepping
>> should be implemented. That's why I didn't do it.
>>
> And if I were to do it, I would use the simplest approach that I can
> think of. IIRC you mentioned config file to tell which mode the runner
> to operate. I think that's a bit overkilled, but I'm not sure I can
> second-guess what you want to achieve.

This is broadly in the right direction.

>
>
> From 51039110233cabb490c978e7da7dc70d0d30bdcb Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@xxxxxxxxxx>
> Date: Wed, 10 Aug 2016 11:40:07 +0100
> Subject: [XTF PATCH] xtf-runner: skeleton for different modes to get output
>
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  xtf-runner | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/xtf-runner b/xtf-runner
> index 3bb98e0..694b591 100755
> --- a/xtf-runner
> +++ b/xtf-runner
> @@ -536,6 +536,19 @@ def main():
>                    "  all tests in the selection, printing a summary of 
> their\n"
>                    "  results at the end.\n"
>                    "\n"
> +                  "  To determine how runner should get output from Xen, 
> use\n"
> +                  "  --mode option. The default value is \"auto\", which 
> will\n"
> +                  "  choose the mode based on Xen version. Other supported 
> values\n"
> +                  "  are \"console\" and \"logfile\", which respectively 
> means\n"
> +                  "  to get log from xenconsole and to get log from 
> xencosonsoled\n"
> +                  "  output.\n"
> +                  "\n"
> +                  "  The \"console\" mode requires users to configure 
> xenconsoled\n"
> +                  "  to log guest console output. This mode is useful for 
> Xen\n"
> +                  "  version < 4.8. Note that runner will append a custom 
> string to\n"
> +                  "  guest log file before running each test. Also see 
> --log-dir\n"
> +                  "  and --log-pattern options.\n"
> +                  "\n"
>                    "Selections:\n"
>                    "  A selection is zero or more of any of the following\n"
>                    "  parameters: Categories, Environments and Tests.\n"
> @@ -613,6 +626,17 @@ def main():
>                        dest = "host", help = "Restrict selection to 
> applicable"
>                        " tests for the current host",
>                        )
> +    parser.add_option("-m", "--mode", action = "store",
> +                      dest = "mode", default = "auto", type = "string",
> +                      help = "Instruct how runner gets its output (see 
> below)")
> +    parser.add_option("--log-dir", action = "store",
> +                      dest = "log_dir", default = "/var/log/xen/console/",
> +                      type = "string",
> +                      help = "Specify the directory to look for console 
> logs, defaults to \"/var/log/xen/console/\"")
> +    parser.add_option("--log-pattern", action = "store",
> +                      dest = "log_pattern", default = "guest-%s",
> +                      type = "string",
> +                      help = "Sepcify the log file name pattern, defaults to 
> \"guest-%s\"")

"specify"

In python, ' and " are interchangeable for strings (so long as they are
matched pairs), and the primary use of interchanging them is to avoid
needing to escape so many things.

Each of these options should probably have a console prefix, to avoid
confusing them with xtf logs, but this was the general idea I was going
for.  I also wanted to see about being able to set them in the
environment, or a config file, to avoid passing them to each invocation.

~Andrew

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