778177 - add a watcher.ini file to the tegras while we roll out sutagent 1.11

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Here is some code that will add a watcher.ini to the tegras.  This will allow it to ping bm-remote instead of mozilla.org:

import devicemanagerSUT
destname = '/data/data/com.mozilla.watcher/files/watcher.ini'
tmpname = '/mnt/sdcard/watcher.ini'
data = '\r\n[watcher]\r\nPingTarget = bm-remote.build.mozilla.org\r\n'
dm = devicemanagerSUT.DeviceManagerSUT('192.168.1.69', 20701)
dm.verifySendCMD(['push %s %s\r\n' % (tmpname, len(data)), data], newline = False)
dm.sendCMD(['exec su -c "dd if=%s of=%s"' % (tmpname, destname)])

Comment 1

[Justin Wood (:Callek)]

Attachment: [tools] v1

Based on Joel's patch.

The idea is I can do:

for i in tegra-*; do PYTHONPATH=/builds/sut_tools python sut_tools/update_watcher_ini.py $i; done

and be done a whole foopy.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 646617 [details] [diff] [review]
[tools] v1

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

just remember this is pegged to the devicemanager that we use in sut_tools :)

Comment 3

[Chris Cooper [:coop] (he/him)]

Comment on attachment 646617 [details] [diff] [review]
[tools] v1

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

::: sut_tools/update_watcher_ini.py
@@ +1,5 @@
> +#!/usr/bin/env python
> +
> +def main(tegra):
> +    import devicemanagerSUT
> +    destname = '/data/data/com.mozilla.watcher/files/watcher.ini'

/data/data is the correct dir here, and not an accidental duplication?

Comment 4

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

yes, /data/data is legit on android.  Thanks for asking.

Comment 5

[Justin Wood (:Callek)]

Ok this is now done for all tegras on linux-foopy-test, foopy05 and foopy06; deploying is annoying given the normal reboots, so we'll get this tied to verify somehow for real deploy.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

so we want strikes=0 to be added as well.

this line:
+    data = '\r\n[watcher]\r\nPingTarget = bm-remote.build.mozilla.org\r\n'

should be:
+    data = '\r\n[watcher]\r\nPingTarget = bm-remote.build.mozilla.org\r\nstrikes = 0\r\n'

Comment 7

[Justin Wood (:Callek)]

Attachment: Part 2

This obsoletes the v1 patch here, and should make it deployed into prod easily.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 647773 [details] [diff] [review]
Part 2

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

::: sut_tools/verify.py
@@ +212,5 @@
> +    if not dmAlive(dm):
> +       return False
> +    
> +    realLoc = "/data/data/com.mozilla.watcher/files/watcher.ini"
> +    if dm.fileExists(realLoc):

this doesn't check the value of the file, just that it exists.

@@ +219,5 @@
> +    try:
> +        tmpname = '/mnt/sdcard/watcher.ini'
> +        # Need to install it
> +        dm.verifySendCMD(['push %s %s\r\n' % (tmpname, len(watcherINI)), watcherINI], newline = False)
> +        dm.sendCMD(['exec su -c "dd if=%s of=%s"' % (tmpname, destname)])

s/destname/realLoc/

@@ +222,5 @@
> +        dm.verifySendCMD(['push %s %s\r\n' % (tmpname, len(watcherINI)), watcherINI], newline = False)
> +        dm.sendCMD(['exec su -c "dd if=%s of=%s"' % (tmpname, destname)])
> +    except:
> +        setFlag(errorFile, "Unable to properly upload the watcher.ini")
> +        return False

you don't return True on success.

Comment 9

[Justin Wood (:Callek)]

(In reply to Joel Maher (:jmaher) from comment #8)
> Comment on attachment 647773 [details] [diff] [review]
> Part 2
> 
> Review of attachment 647773 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: sut_tools/verify.py
> @@ +212,5 @@
> > +    if not dmAlive(dm):
> > +       return False
> > +    
> > +    realLoc = "/data/data/com.mozilla.watcher/files/watcher.ini"
> > +    if dm.fileExists(realLoc):
> 
> this doesn't check the value of the file, just that it exists.

O right :( I lost your pastebin on that check, can you remind me the magic?

> s/destname/realLoc/

Good catch -- also a good sign that my mind is mushing now, and that I should not do deploy tonight, as sanity

> you don't return True on success.

Fixed

Comment 10

[Justin Wood (:Callek)]

Attachment: Part 2.1

This actually works on staging, not sure what was different between line endings before but this solved it.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 648223 [details] [diff] [review]
Part 2.1

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

the \r\n is tricky, a relic from the windows mobile days.  When we had both agents in the works, our scripts had to account for \r\n and so we made the android version hack it in as well.   

The two comments below are nothing of concern, just nits or general questions.

::: sut_tools/verify.py
@@ +224,5 @@
> +               return True
> +    except:
> +        setFlag(errorFile, "Unable to identify if watcher.ini is current")
> +        raise
> +        return False

I think the return False will never get hit

@@ +276,5 @@
>      if not cleanupTegra(dm):
>          return False
> +    
> +    if not setWatcherINI(dm):
> +        return False

you don't catch the exception that is raised here.  Do you want a big old traceback printed to the tinderbox log or catch it and handle it with a known error?

Comment 12

[Justin Wood (:Callek)]

Attachment: [tools] Part 2 -- Final

So, this removes the superfluous raise that I had in here (which was for testing an earlier issue) and makes the final failure more meaningful and useful in all failures there.

Comment 13

[Armen [:armenzg]]

Comment on attachment 648374 [details] [diff] [review]
[tools] Part 2 -- Final

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

r+ with addressing my comment one way (put it in a function) or the other (there is a slight difference that you missed).

Good job Callek!

::: sut_tools/verify.py
@@ +241,5 @@
> +               return True
> +    except:
> +        pass
> +    setFlag(errorFile, "Unable to verify the updated watcher.ini")
> +    return False

This try/except block is pretty much the same as the previous one (unless I'm missing something). This could be a call to a function to make it cleaner and reduce the chances to modify one and not the other in the future.

Unless I got it wrong, would you please put it in a function?

Comment 14

[Justin Wood (:Callek)]

(In reply to [:armenzg] - gone from Aug. 3rd to Aug. 27th from comment #13)
> Comment on attachment 648374 [details] [diff] [review]
> [tools] Part 2 -- Final
> 
> Review of attachment 648374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with addressing my comment one way (put it in a function) or the other
> (there is a slight difference that you missed).
> 
> Good job Callek!

Allow me to give at least half the "Good Job" credit to joel, for he was the one that came up with the dm.* commands to do this, and the .ini options. Even though I fully support the change.

That said, me and armen spoke in person about this, and determined that as-is is better than trying to abstract any more of it into a function.

http://hg.mozilla.org/build/tools/rev/bcc3a7523513

(reso/fixed, but not yet deployed)

Comment 15

[Justin Wood (:Callek)]

Attachment: [tools] Part 2's bustage fix

Apparently Part 2 tested fine in staging, but busted in prod -- this was tested in prod and worked fine.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 648850 [details] [diff] [review]
[tools] Part 2's bustage fix

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

logically this looks just fine.  I am really confused with staging/production are so different.