Bug 222601 – [JSC] detect infrastructure failure for remote stress tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Angelos Oikonomopoulos]

[JSC] detect infrastructure failure for remote stress tests

[Angelos Oikonomopoulos]

Created attachment 421943 [details]
Patch

[Angelos Oikonomopoulos]

The current patch only changes jsc-stress-test-writer-default.rb. Uploading this for early comments, will update the other 2 writers if things look good.

[Angelos Oikonomopoulos]

Created attachment 421947 [details]
Patch

[Angelos Oikonomopoulos]

Created attachment 422072 [details]
Patch

[Angelos Oikonomopoulos]

Created attachment 422073 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75218695>

[Angelos Oikonomopoulos]

Any objections in principle?

[Yusuke Suzuki]

Comment on attachment 422073 [details]
Patch

r=me

[EWS]

Committed r275801 (236372@main): <https://commits.webkit.org/236372@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422073 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 224423

[Yusuke Suzuki]

@Mark found that it seems that run-jsc-stress-tests output includes a lot of texts now. So I've reverted it.

    0/75193 .............................Could not open file: /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
Could not open file: /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
Could not open file: /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
Could not open file: /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
...
Could not open file: /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
    2/75193 ............................Could not open file: /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
Could not open file: /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
    3/75193 .............................Could not open file: /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
Could not open file: /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
...

[Yusuke Suzuki]

@Angelos can you take a look?

[Angelos Oikonomopoulos]

(In reply to Yusuke Suzuki from comment #12)
> @Angelos can you take a look?

Absolutely. Note that, as stated in an earlier comment, I'll need to update the other two test writers before this can go in. Sorry that wasn't clear!

I'll submit a new version when I figure out the issues that cropped up. Thanks for taking a look!

[Angelos Oikonomopoulos]

(In reply to Yusuke Suzuki from comment #11)
> @Mark found that it seems that run-jsc-stress-tests output includes a lot of
> texts now. So I've reverted it.
> 
>     0/75193 .............................Could not open file:
> /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> Could not open file:
> /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> Could not open file:
> /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> Could not open file:
> /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> ...
> Could not open file:
> /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
>     2/75193 ............................Could not open file:
> /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> Could not open file:
> /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
>     3/75193 .............................Could not open file:
> /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> Could not open file:
> /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> ...

The above to be messages from some non-public usage of funfuzz? I'm unclear as to how that uses run-jsc-stress-tests or where the regression-tests.list file comes from and what run-jsc-stress-tests has got to do with it.

What additional output does run-jsc-stress-tests produce with this patch?

The above messages don't come from any script generated by it AFAIK. (Does @Mark refer to Mark Lam? If so, please let me know and I can ping them directly.)

[Mark Lam]

(In reply to Angelos Oikonomopoulos from comment #14)
> (In reply to Yusuke Suzuki from comment #11)
> > @Mark found that it seems that run-jsc-stress-tests output includes a lot of
> > texts now. So I've reverted it.
> > 
> >     0/75193 .............................Could not open file:
> > /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> > Could not open file:
> > /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> > Could not open file:
> > /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> > Could not open file:
> > /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> > ...
> > Could not open file:
> > /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> >     2/75193 ............................Could not open file:
> > /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> > Could not open file:
> > /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> >     3/75193 .............................Could not open file:
> > /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> > Could not open file:
> > /Users/cyril/mozilla_funfuzz/wtmp16/regression-tests.list
> > ...
> 
> The above to be messages from some non-public usage of funfuzz? I'm unclear
> as to how that uses run-jsc-stress-tests or where the regression-tests.list
> file comes from and what run-jsc-stress-tests has got to do with it.
> 
> What additional output does run-jsc-stress-tests produce with this patch?
> 
> The above messages don't come from any script generated by it AFAIK. (Does
> @Mark refer to Mark Lam? If so, please let me know and I can ping them
> directly.)

Yes, Yusuke was referring to me.  These messages came from internal tests.  Before your patch, running the script did not produce this output.  After your patch, it does.  I suspect that the tests were always printing these output.  However, your change somehow expose them.  Do you have any idea where your patch enables more verbose stderr/stdout output to be shown?

[Angelos Oikonomopoulos]

(In reply to Mark Lam from comment #15)
> (In reply to Angelos Oikonomopoulos from comment #14)
> > (In reply to Yusuke Suzuki from comment #11)
[...]
> > The above messages don't come from any script generated by it AFAIK. (Does
> > @Mark refer to Mark Lam? If so, please let me know and I can ping them
> > directly.)
> 
> Yes, Yusuke was referring to me.  These messages came from internal tests. 
> Before your patch, running the script did not produce this output.  After
> your patch, it does.  I suspect that the tests were always printing these
> output.  However, your change somehow expose them.  Do you have any idea
> where your patch enables more verbose stderr/stdout output to be shown?

I've taken a second look and I don't see any instances where the patch changes where stdout/stderr go, but it's tricky code. I can't see any extra output in my testing of course (I just checked again both with local and --remote runs).

webkitruby/jsc-stress-test-writer-default.rb defines a number of handlers that the patch modifies. Are you at liberty to share what test kind is producing the extraneous output? I.e. which run* function is invoked in run-jsc-stress-tests? If so, then I could look at a diff of the generated code in detail.

[Angelos Oikonomopoulos]

FWIW I think I've tracked down the issue with the uncaptured stderr that Mark ran into. I've updated the ruby test writer too, which was straightforward.

My remaining concern is this piece of code:

           outp.puts JSON.generate({
                name: @name,
                outputDir: $runnerDir,
                baseDir: baseDir,
                env: $envVars + @additionalEnv,
                outputName: @name.gsub(/(\\|\/)/, '_'),
                checkScript: filename,
                args: @arguments,
                failFile: "#{failFile}"
            })

in the playstation test writer. My patch removes the failFile and replaces it with a statusFile which contains a single line: $runid $exitcode $pf, where $pf is either P(ass) or F(ail).

Stephan, presumably something checks that failFile on your end? Would you be ok with adapting it to the changes in this patch?

[Angelos Oikonomopoulos]

Created attachment 427342 [details]
Patch

[Stephan Szabo]

(In reply to Angelos Oikonomopoulos from comment #17)
> FWIW I think I've tracked down the issue with the uncaptured stderr that
> Mark ran into. I've updated the ruby test writer too, which was
> straightforward.
> 
> My remaining concern is this piece of code:
> 
>            outp.puts JSON.generate({
>                 name: @name,
>                 outputDir: $runnerDir,
>                 baseDir: baseDir,
>                 env: $envVars + @additionalEnv,
>                 outputName: @name.gsub(/(\\|\/)/, '_'),
>                 checkScript: filename,
>                 args: @arguments,
>                 failFile: "#{failFile}"
>             })
> 
> in the playstation test writer. My patch removes the failFile and replaces
> it with a statusFile which contains a single line: $runid $exitcode $pf,
> where $pf is either P(ass) or F(ail).
> 
> Stephan, presumably something checks that failFile on your end? Would you be
> ok with adapting it to the changes in this patch?

Yes, that'd be fine. Actually, because of the weirdness for filesystem access for our case, I believe our driver is actually are checking the status and then making the fail file for the rest of the test harness, so I should be able to easily have it generate the statusFile in that format instead.

[Angelos Oikonomopoulos]

Created attachment 427739 [details]
Patch

[Angelos Oikonomopoulos]

Created attachment 427872 [details]
Patch

[Angelos Oikonomopoulos]

(In reply to Stephan Szabo from comment #19)
> (In reply to Angelos Oikonomopoulos from comment #17)
[...]
> > Stephan, presumably something checks that failFile on your end? Would you be
> > ok with adapting it to the changes in this patch?
> 
> Yes, that'd be fine. Actually, because of the weirdness for filesystem
> access for our case, I believe our driver is actually are checking the
> status and then making the fail file for the rest of the test harness, so I
> should be able to easily have it generate the statusFile in that format
> instead.

Great! As I can't update your driver, at the moment the patch removes the failFile field from the json file. Do you prefer to handle this some other way?

[Stephan Szabo]

(In reply to Angelos Oikonomopoulos from comment #22)
> (In reply to Stephan Szabo from comment #19)
> > (In reply to Angelos Oikonomopoulos from comment #17)
> [...]
> > > Stephan, presumably something checks that failFile on your end? Would you be
> > > ok with adapting it to the changes in this patch?
> > 
> > Yes, that'd be fine. Actually, because of the weirdness for filesystem
> > access for our case, I believe our driver is actually are checking the
> > status and then making the fail file for the rest of the test harness, so I
> > should be able to easily have it generate the statusFile in that format
> > instead.
> 
> Great! As I can't update your driver, at the moment the patch removes the
> failFile field from the json file. Do you prefer to handle this some other
> way?

I'll probably add statusFile to the json file in the end, but I think the above is fine for the current patch, since I may need to do some other related changes in practice.

[Angelos Oikonomopoulos]

(In reply to Stephan Szabo from comment #23)
[...]
> I'll probably add statusFile to the json file in the end, but I think the
> above is fine for the current patch, since I may need to do some other
> related changes in practice.

Alright, thanks!

@Mark, the change to fix the stderr redirection was

-            cmd = "{ { { { #{shellCommand}; echo $? >&3; } | { #{outputHandler.call(@name)} ;} >&4; } 3>&1; } | { read xs; exit $xs; } } 4>&1\nexitCode=$?\n"
+            cmd = "{ { { { #{shellCommand} 2>&1; echo $? >&3; } | { #{outputHandler.call(@name)} ;} >&4; } 3>&1; } | { read xs; exit $xs; } } 4>&1\nexitCode=$?\n"

Do you feel comfortable trying to land the patch now?

[Angelos Oikonomopoulos]

(In reply to Angelos Oikonomopoulos from comment #24)
[...]
> @Mark, the change to fix the stderr redirection was
> 
> -            cmd = "{ { { { #{shellCommand}; echo $? >&3; } | {
> #{outputHandler.call(@name)} ;} >&4; } 3>&1; } | { read xs; exit $xs; } }
> 4>&1\nexitCode=$?\n"
> +            cmd = "{ { { { #{shellCommand} 2>&1; echo $? >&3; } | {
> #{outputHandler.call(@name)} ;} >&4; } 3>&1; } | { read xs; exit $xs; } }
> 4>&1\nexitCode=$?\n"
> 
> Do you feel comfortable trying to land the patch now?

Note, I also took the opportunity to clarify the above pipeline with an ascii diagram in a preceding comment:

# +--------------------------------------------------------------------+                                                                                                                              
# | +-----------------------------------------------+                  |                                                                                                                              
# | | +--------------+     +-------------------+    |                  |                                                                                                                              
# | | | cmd 1 ----> 1|---> |0 --> outH 1 ---> 4|-> 4|---------------> 1|                                                                                                                              
# | | |     2 /      |     +-------------------+    |   +-----------+  |                                                                                                                              
# | | |echo $? 0 -> 3|---------------------------> 1|-> |0 read xs  |  |                                                                                                                              
# | | +--------------+                              |   |  exit $xs |  |                                                                                                                              
# | |                                               |   +-----------+  |                                                                                                                              
# | +-----------------------------------------------+                  |                                                                                                                              
# +--------------------------------------------------------------------+

[Mark Lam]

Comment on attachment 427872 [details]
Patch

Looks reasonable to me.

[EWS]

Committed r277318 (237577@main): <https://commits.webkit.org/237577@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427872 [details].