Closed Bug 802768 Opened 10 years ago Closed 10 years ago

Cleanup code, begin to add official panda support to sut_tools

Categories

(Release Engineering :: General, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Callek, Assigned: Callek)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached patch [tools] v1 (obsolete) — Splinter Review
So this patch does a lot of s/tegra/device/ stuff, and needed attention throughout.

I explicitly tried to steer clear of the check.py//dashboard code for now, just making sure it doesn't break with any of these other changes.

Wanting at laest 2 sets of eyes on this code, as a sanity, over-requesting because I don't know which 2 will look sooner. (Others can feel free to chime in)
Attachment #672462 - Flags: review?(kmoir)
Attachment #672462 - Flags: review?(jmaher)
Attachment #672462 - Flags: review?(coop)
Attachment #672462 - Flags: review?(armenzg)
Comment on attachment 672462 [details] [diff] [review]
[tools] v1

Review of attachment 672462 [details] [diff] [review]:
-----------------------------------------------------------------

I would normally give r- with the EXPECTED_DEVICE_SCREEN but I would rather see this committed and make sure that it works with tegra and then follow up with any adjustments for the pandas.

This patch as-is does not seem like it could give any trouble to the tegras.

::: sut_tools/check.py
@@ +15,5 @@
>  
>  # from multiprocessing import get_logger, log_to_stderr
>  from sut_lib import checkSlaveAlive, checkSlaveActive, getIPAddress, dumpException, loadOptions, \
> +                    checkCPAlive, checkCPActive, getLastLine, stopProcess, runCommand, pingDevice, \
> +                    reboot_tegra, stopDevice, getMaster

reboot_device rather than reboot_tegra

@@ +189,4 @@
>  
>      if options.reset and sTegra == 'INACTIVE' and status['cp'] == 'INACTIVE':
>          log.info('stopping hung clientproxy')
> +        stopDevice(tegra)

stopDevice(device) instead of stopDevice(tegra)?

::: sut_tools/verify.py
@@ +137,5 @@
>      info = dm.getInfo("screen")
> +    if not info["screen"][0] == EXPECTED_DEVICE_SCREEN:
> +        setFlag(errorFile, "Remote Device Error: Unexpected Screen on device, got '%s' expected '%s'" % \
> +                            (info["screen"][0], EXPECTED_DEVICE_SCREEN))
> +        if not dm.adjustResolution(**EXPECTED_DEVICE_SCREEN_ARGS):

Not sure if this code should be re-used for pandas as of now.
Attachment #672462 - Flags: review?(armenzg) → review+
Comment on attachment 672462 [details] [diff] [review]
[tools] v1

Review of attachment 672462 [details] [diff] [review]:
-----------------------------------------------------------------

this looks good.  I really want to make sure that all scripts and tools which call this from a command line are modified.

::: sut_tools/clientproxy.py
@@ +35,5 @@
>  
>      Parameters
>          --bbpath   <path>   Parent directory where the buildslave to control is located.
> +        --device            Device to manage. If not given it will be determined.
> +        --deviceIP          IP address of Device to manage. Will be discovered if not given.

who calls this?  buildbot?  can we make sure we update all upstream consumers of these tools.

@@ +489,3 @@
>  
>      try:
> +        n = int(options.device.split('-')[1])

will this work with panda-0001 ?
Attachment #672462 - Flags: review?(jmaher)
Attachment #672462 - Flags: review?(armenzg)
Attachment #672462 - Flags: review+
Attachment #672462 - Flags: review?(armenzg) → review+
no idea how I asked :armenzg for review on this...quirky bugzilla.
Comment on attachment 672462 [details] [diff] [review]
[tools] v1

Review of attachment 672462 [details] [diff] [review]:
-----------------------------------------------------------------

Just the one nit about changing reboot_tegra() to reboot_device(). r+ with that change made.

::: sut_tools/clientproxy.py
@@ +489,3 @@
>  
>      try:
> +        n = int(options.device.split('-')[1])

Just tested it in a python shell, and yes, n=1 for panda-0001.

::: sut_tools/sut_lib.py
@@ +491,3 @@
>          sys.exit(1)
>  
>  def reboot_tegra(tegra, debug=False):

Change to reboot_device (as I believe Armen suggested elsewhere)?
Comment on attachment 672462 [details] [diff] [review]
[tools] v1

Review of attachment 672462 [details] [diff] [review]:
-----------------------------------------------------------------

::: buildfarm/mobile/stop_cp.sh
@@ +13,3 @@
>    if [ -d $i ] ; then
>      if [ -e $i/clientproxy.pid ] ; then
>        python sut_tools/stop.py -t $i

self-review catch #1:

stop.py --device $i  rather than -t

::: sut_tools/check.py
@@ +189,4 @@
>  
>      if options.reset and sTegra == 'INACTIVE' and status['cp'] == 'INACTIVE':
>          log.info('stopping hung clientproxy')
> +        stopDevice(tegra)

With this patch, check.py still only supports tegras, so this line is correct. Above though, s/reboot_tegra/reboot_device/ was asked for by not just you, so will do.

::: sut_tools/clientproxy.py
@@ +35,5 @@
>  
>      Parameters
>          --bbpath   <path>   Parent directory where the buildslave to control is located.
> +        --device            Device to manage. If not given it will be determined.
> +        --deviceIP          IP address of Device to manage. Will be discovered if not given.

This is actually only ever called by start_cp.sh -- or rather I also see: http://mxr.mozilla.org/build/source/tools/sut_tools/check.py#196 now but I have no idea if/how that has *ever* been supposed to work. I will fix its command arg anyway, for added sanity.

::: sut_tools/sut_lib.py
@@ +491,3 @@
>          sys.exit(1)
>  
>  def reboot_tegra(tegra, debug=False):

done

::: sut_tools/verify.py
@@ +137,5 @@
>      info = dm.getInfo("screen")
> +    if not info["screen"][0] == EXPECTED_DEVICE_SCREEN:
> +        setFlag(errorFile, "Remote Device Error: Unexpected Screen on device, got '%s' expected '%s'" % \
> +                            (info["screen"][0], EXPECTED_DEVICE_SCREEN))
> +        if not dm.adjustResolution(**EXPECTED_DEVICE_SCREEN_ARGS):

It's actually not: 

    # Resolution Check disabled for now; Bug 737427
    if False and not checkAndFixScreen(dm):

But fairly stated
Attached patch [tools] v2Splinter Review
review/feedback just for extra set of eyes. About to throw this into staging.
Attachment #672462 - Attachment is obsolete: true
Attachment #672462 - Flags: review?(kmoir)
Attachment #672462 - Flags: review?(coop)
Attachment #672931 - Flags: review?(kmoir)
Attachment #672931 - Flags: feedback?(coop)
Attached patch interdiffSplinter Review
For anyone curious, this is an interdiff between v1 and v2
Comment on attachment 672931 [details] [diff] [review]
[tools] v2

Looks good, I noticed when testing that in the start_cp.sh the condition to check for the existence of pandas and tegras directory matches even if tegra-* doesn't exist

if [ -n "$pandas" -a -n "$tegras" ] ; then
# Preserve the assertion that a single foopy won't handle both device types
 31    echo ERROR: Found both panda-* and tegra-* in directory.

so this needs to be fixed
Attachment #672931 - Flags: review?(kmoir) → review+
Attachment #672931 - Flags: feedback?(coop) → feedback+
http://hg.mozilla.org/build/tools/rev/e6ebee9b679d with changes to start_cp as kim suggested
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Soo after testing other patches in more detail, I found a few issues with what landed here.

sut_lib.pandas and tegras were failing to actually get populated (s/devices/allDevices/) and doing that sed also caused problems (was an array with no keys, vs a dictionary like code expected) the update here fixes that.

On linux the start_cp was failing due to the bad stderr redir. I added the ls command to stop_cp as well due to that trying to stop a device called |tegra-*| on the panda foopy. Also the stop/start files on mac foopies are not symlinked to the files in /builds/tools so will need manual updating.
Attachment #675915 - Flags: review?(kmoir)
reopen because not yet deployed and re: c#10
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #675915 - Flags: review?(kmoir) → review+
bustage fix landed and deployed: http://hg.mozilla.org/build/tools/rev/0c2c85858515
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.