Allow test slaves to save and upload files somewhere, even from try

RESOLVED FIXED

Status

defect
P4
normal
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: billm, Assigned: mtabara)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [blobs])

Attachments

(30 attachments, 20 obsolete attachments)

4.91 KB, patch
aki
: review+
Details | Diff | Splinter Review
3.00 KB, patch
aki
: review+
Details | Diff | Splinter Review
10.62 KB, patch
aki
: review+
Details | Diff | Splinter Review
4.28 KB, patch
aki
: review+
Details | Diff | Splinter Review
3.09 KB, patch
ted
: review+
Details | Diff | Splinter Review
959 bytes, patch
rail
: review+
Details | Diff | Splinter Review
859 bytes, patch
rail
: review+
Details | Diff | Splinter Review
929 bytes, patch
rail
: review+
Details | Diff | Splinter Review
4.61 KB, patch
aki
: review+
Details | Diff | Splinter Review
2.52 KB, patch
catlee
: review+
Details | Diff | Splinter Review
1.87 KB, patch
aki
: review+
Details | Diff | Splinter Review
936 bytes, patch
rail
: review+
Details | Diff | Splinter Review
1.69 KB, patch
rail
: review+
Details | Diff | Splinter Review
935 bytes, patch
rail
: review+
Details | Diff | Splinter Review
595 bytes, patch
rail
: review+
Details | Diff | Splinter Review
1.09 KB, patch
rail
: review+
Details | Diff | Splinter Review
1.91 KB, patch
rail
: review+
Details | Diff | Splinter Review
766 bytes, patch
rail
: review+
Details | Diff | Splinter Review
5.73 KB, patch
aki
: review+
Details | Diff | Splinter Review
11.22 KB, patch
aki
: review+
Details | Diff | Splinter Review
4.82 KB, patch
aki
: review+
Details | Diff | Splinter Review
1.32 KB, patch
aki
: review+
Details | Diff | Splinter Review
1.68 KB, patch
aki
: review+
Details | Diff | Splinter Review
6.64 KB, patch
aki
: review+
Details | Diff | Splinter Review
7.46 KB, patch
aki
: review+
Details | Diff | Splinter Review
8.36 KB, patch
aki
: review+
Details | Diff | Splinter Review
1.27 KB, patch
aki
: review+
Details | Diff | Splinter Review
5.72 KB, patch
aki
: review+
Details | Diff | Splinter Review
7.30 KB, patch
aki
: review+
Details | Diff | Splinter Review
4.50 KB, patch
aki
: review+
Details | Diff | Splinter Review
We currently have bug 642167 open for uploading minidumps to ftp.mozilla.org. However, there are some security issues with that. Chris Atlee and I talked about allowing files to be uploaded to a scratch machine that would be accessible to developers and to the test slaves. The data would only have to live there for a few days.

The main aspects of this seem to be:

1. Find a machine to use with enough disk space. Chris said that this is harder than it sounds, since the machine will be under a lot of load and the disk would fail quickly.

2. Allow test slaves to access the machine.

Writing code to actually upload files from the test slaves should be a separate bug.

So how hard is #1? Would it be easier to use something like Amazon S3? None of this data is valuable, so we just want something easy that won't require much maintenance. Backups and redundancy shouldn't be necessary.
Component: Release Engineering → Release Engineering: Platform Support
QA Contact: release → coop
This seems like an automation bug to me, rather than anything platform-specific since I assume we'll want this for _all_ the test platforms.
Component: Release Engineering: Platform Support → Release Engineering: Automation (General)
QA Contact: coop → catlee
possibly we could use a yet-to-be-implemented blob store. either that or S3
Priority: -- → P4
Whiteboard: [blobs]
I spoke with Taras about this request. According to him, this change should result in in developer months of saved time. (People currently have to resort to printf-debugging on try.) Taras tells me that Bill says it would've saved him 3 weeks on bug 803376. I would really like to reduce the time to fix GC bugs.

catlee - Can we talk about the feasibility of prioritizing this issue higher than P4?
I spoke with billm about his request. It seems like the minimal request is:

1. Find a machine, as per the description. billm suggested that we can turn on the file upload via a config param to start so as to minimize the disk usage and churn.

2. Modify the firewall or other premissions to allow the test machines to ftp (or somehow otherwise transfer) out the file to the machine from #1.

billm will take care of the code to actually upload the files once the test machines are configured to allow the transfer. 

billm - Do you have a bug on file for uploading the files?

catlee - Am I missing any required steps to make this happen? Does modifying the firewall/permissions for test machines require sec review?
(In reply to Lawrence Mandel [:lmandel] from comment #4)
> billm - Do you have a bug on file for uploading the files?

There's no bug yet, but I can file something.
catlee - Bump. Can this bug be included in releng Q1 activities.
Ted, gps, catlee and I discussed briefly.

Files to be uploaded should be determined in-tree (and outside of the build configs).  A simple solution is for tests to place files in an agreed directory location that gets uploaded.

Test file uploads must not be able to overwrite or delete files in other builds.  Possible solutions include uploading files via a web service or post-upload script.

Test builds do not have access to any SSH keys at present.  Normally, try and non-try build slaves use different keys but since the test build slaves are run against try and non-try binaries, this is a security aspect we'll have to give some thought.
Summary: Allow test slaves to save files somewhere → Allow test slaves to save and upload files somewhere, even from try
We need to upload files from test jobs in order to be able to use the structured logs that our test jobs will be generating soon, courtesy of chmanchester.  They don't need to go any place in particular, they just need to end up in some publicly-visible, reliable location.
Blocks: 870657
Blocks: 890116
Blocks: 893388
Assignee: nobody → mtabara
The BlobberMixin is to handle the virtualenv dependecies, dir creation and MOZ_UPLOAD_DIR definition. Still to be done is the branch specification which is currently set in the configs file - it should be taken from the buildbot configs along the way.

The blobuploader python script package lies here for the moment - http://people.mozilla.com/~mtabara/python/packages/
Attachment #780517 - Flags: feedback?(aki)
Comment on attachment 780517 [details] [diff] [review]
Mechanism to automatically upload files from MOZ_UPLOAD_DIR after running a mozharness script

Thanks for the patch!

>diff --git a/examples/blobber_usage.sh b/examples/blobber_usage.sh
>new file mode 100755
>--- /dev/null
>+++ b/examples/blobber_usage.sh
>@@ -0,0 +1,3 @@
>+#! /bin/sh
>+
>+echo 'Hello world!' > $MOZ_UPLOAD_DIR/file_one

I'm guessing this is just here for testing / proof of concept right?

If this file (in some non-"Hello World" variation) were to stick around in some form, a better location would either be external_tools
    http://hg.mozilla.org/build/mozharness/file/069dc44ccf51/external_tools
if we're planning to call it from scripts, or test/helper_files
    http://hg.mozilla.org/build/mozharness/file/069dc44ccf51/test/helper_files
if we're planning to call it from the nosetests.

examples/ is for non-functional scripts that were intended to show mozharness developers how to write mozharness scripts.
I'm not sure how useful it is today, since we have many more working examples in scripts/.
If we were to keep blobbertest.py in its current form, it might belong in examples/.

Unless there's a specific reason to use an external script, though, self.write_to_file() should do the trick here.
http://hg.mozilla.org/build/mozharness/file/069dc44ccf51/mozharness/base/script.py#l312

>+class BlobberMixin(VirtualenvMixin):
>+    """Provides mechanism to automatically upload files written in
>+    MOZ_UPLOAD_DIR to the blobber upload server at the end of the
>+    running script.
>+
>+    This is dependent on ScriptMixin.
>+    The testing script inheriting this class is to define in its
>+    configuration file the moz_upload_servers (e.g. URLs to blobber
>+    upload servers)
>+
>+    """

The name's amusing, but I'm not sure about it in an intuitive/predictable sense.
BlobUploadMixin might make more sense.

Also, if we took away the MOZ_UPLOAD_DIR env var hardcode, we could make this a generic non-mozilla-specific mixin that could live in mozharness.base.transfer.  The inheriting classes could then set the appropriate blobber_dir based on MOZ_UPLOAD_DIR or config or commandline option or w/e.

>+    #TODO: documentation about the Blobber Server on wiki
>+    def __init__(self, *args, **kwargs):
>+        requirements = [
>+            'blobuploader==0.1',
>+        ]
>+        super(BlobberMixin, self).__init__(*args, **kwargs)
>+        for req in requirements:
>+            self.register_virtualenv_module(req, method='pip')
>+
>+    @PostScriptAction('create-virtualenv')
>+    def _build_upload_directory(self, action, success=None):
>+        dirs = self.query_abs_dirs()
>+        self.blobber_dir = os.path.join(dirs['abs_work_dir'], 'blobber_upload_dir')
>+        self.mkdir_p(self.blobber_dir)
>+        self.env['MOZ_UPLOAD_DIR'] = self.blobber_dir

We shouldn't hardcode this.  We should either go by self.query_abs_dirs() or self.config, probably.
We probably should have a way to add things to query_abs_dirs() without having to override it completely every time, too.
That's outside the scope of this bug unless you want to take it on.

>+    @PostScriptRun
>+    def _upload_blobber_files(self):
>+    #TODO: the branch should be taken from self.config (buildbot configs)
>+        upload = self.query_python_path("blobberc.py")
>+        self.run_command([upload] +
>+                         self.config['moz_upload_servers'] +
>+                         self.config['moz_upload_branch'] +
>+                         ["-d", self.blobber_dir]
>+        )

This assumes we want to run this upload no matter what, every time we run the script.
That's good for production jobs, but would be very problematic for a developer who wanted to run the same script on their desktop, but without attempts at uploads, etc.

I think we should make this an action, rather than a @PostScriptRun.
I'm going to land a _post_fatal() hook where we can run stuff on every single production run.
Alternately, you could have a check in here to skip the upload if the config says to.

I definitely would like a self.info() before the upload happens, to log/inform what we're trying to do.

>diff --git a/scripts/blobbertest.py b/scripts/blobbertest.py

As above, this could live in examples/ if it's going to stick around.

>+    def run_me(self):
>+        dirs = self.query_abs_dirs()
>+        script = os.path.join(dirs['base_work_dir'], '..', 'examples', 'blobber_usage.sh')
>+        self.run_command([script], env=self.env)

This is where the self.write_to_file() could go.
Attachment #780517 - Flags: feedback?(aki) → feedback+
* Blob upload mechanism to automatically upload fils written in the MOZ_UPLOAD_DIR to the blobber server.
* The code is integrated in the Desktop Unit Test running script.
* the "upload_blobber_files" is an action performed subject to request in the running script
* Still to be done is to determine the branch along the way (buildbot configs most likely). For the moment the branch relies in the config file for the running script
Attachment #781917 - Flags: feedback?(aki)
Comment on attachment 781917 [details] [diff] [review]
Blob upload mechanism sample for Desktop Unit Test integration

This looks good, thanks!

We're going to have to modify each script that we want to use blob uploads.

Also, we can make this method always run via _post_fatal() once bug 898711 lands, or we can make it always run via the decorator, as long as we have a way to turn it off.  Having it be a part of the actions means we'll only run it if we haven't fatal()ed or raised an uncaught exception.  I think that's generally the desktop_unittest.py behavior, though, so this is probably good.
Attachment #781917 - Flags: feedback?(aki) → feedback+
A simple first usage of this would be to fix bug 642167 by setting the MINIDUMP_SAVE_PATH environment variable to be the same as MOZ_UPLOAD_DIR (or a subdir of that, if that works, it doesn't matter much) for our unit test and Talos runs.
* Removed the hardcoded moz_upload_branch from config files into BlobUploadMixin (now taken from buildbot configs)
* Redefined the MINIDUMP_SAVE_PATH with the same path as MOZ_UPLOAD_DIR.

I am currently setting up a personal development master to test the patch within all scripts. 

Questions:
1. will MINIDUMP_STACKWALK be config-provided?
2. should we set MINIDUMP_SAVE_PATH as MOZ_UPLOAD_DIR always as it is now? Or should we do that only when MINIDUMP_SAVE_PATH is not config-provided?
Attachment #783348 - Flags: feedback?(ted)
Attachment #783348 - Flags: feedback?(aki)
Comment on attachment 783348 [details] [diff] [review]
Mechanism to upload from MOZ_UPLOAD_DIR and redefine MINIDUMP_SAVE_PATH

(In reply to Mihai Tabara [:mtabara] from comment #15)
> Created attachment 783348 [details] [diff] [review]
> Mechanism to upload from MOZ_UPLOAD_DIR and redefine MINIDUMP_SAVE_PATH
> 
> * Removed the hardcoded moz_upload_branch from config files into
> BlobUploadMixin (now taken from buildbot configs)

This is true for tests, and I *think* it's true for builds.
Hardcoding it to use the buildbot properties makes it even harder for someone to use this standalone, though.  We might want a way to specify it outside of buildprops.json (commandline option?)

> * Redefined the MINIDUMP_SAVE_PATH with the same path as MOZ_UPLOAD_DIR.

This is test-specific, but since you're specifying it in desktop_unittest.py, that's cool.

> Questions:
> 1. will MINIDUMP_STACKWALK be config-provided?
> 2. should we set MINIDUMP_SAVE_PATH as MOZ_UPLOAD_DIR always as it is now?
> Or should we do that only when MINIDUMP_SAVE_PATH is not config-provided?

So far we have it for every OS.  That might not be true for B2G emulator tests, etc., but that's a different script, so we're fine here.
Attachment #783348 - Flags: feedback?(aki) → feedback+
Attachment #783348 - Flags: feedback?(ted) → feedback+
(In reply to Mihai Tabara [:mtabara] from comment #15)
> Questions:
> 1. will MINIDUMP_STACKWALK be config-provided?

The binary used comes from the build/tools repo and is different per-platform:
http://hg.mozilla.org/build/tools/file/tip/breakpad/

I don't know how to actually answer that question, hopefully that helps.

> 2. should we set MINIDUMP_SAVE_PATH as MOZ_UPLOAD_DIR always as it is now?
> Or should we do that only when MINIDUMP_SAVE_PATH is not config-provided?

Yes, we should do that. Having minidumps get uploaded is way more useful than the existing behavior of just saving them off to a local directory, and should be the default.

(In reply to Aki Sasaki [:aki] (back August 5) from comment #16)
> > * Redefined the MINIDUMP_SAVE_PATH with the same path as MOZ_UPLOAD_DIR.
> 
> This is test-specific, but since you're specifying it in
> desktop_unittest.py, that's cool.

This should be used for Talos as well, FWIW. (It wouldn't be harmful to have it set everywhere, but I don't know if you dislike that for theoretical purity.)
* removed hardcoded branch specs from config file to cmdline option along with the moz_upload_server
* successfully uploads to blob server for Desktop Unittest on dev master with Linux Machine

To be done ASAP:
* test uploads to blob server on Mac & Win machines
* find proper way to change buildbot-configs to call the scripts with the new cmdlines options

Questions:
1. Are we still willing to run 'upload-blobber-files' via the  _post_fatal()?
2. Should we use any try/except with default values in the BlobMixin for getting branch/server?
Attachment #788366 - Flags: feedback?(aki)
Comment on attachment 788366 [details] [diff] [review]
File upload mechanism from MOZ_UPLOAD_DIR and MINIDUMP_SAVE_PATH

Sorry about the delay, 24.0b1 wasn't playing nice.
 
> Questions:
> 1. Are we still willing to run 'upload-blobber-files' via the  _post_fatal()?

Sure, as long as there's a way to turn it off.

> 2. Should we use any try/except with default values in the BlobMixin for
> getting branch/server?

How about, if both moz_upload_branch and moz_upload_server are specified, do the blob upload; otherwise skip?

Otherwise we can specify both, and have a bool flag that we can flip on/off.  --blob-upload or --no-blob-upload maybe.
Attachment #788366 - Flags: feedback?(aki) → feedback+
Product: mozilla.org → Release Engineering
* blob upload server lies in the configuration files as it's less likely to change often
* blob upload branch lies as a command line option (a different patch for tunning the buildbot-configs to do so is coming soon)

* blob upload mechanism works only if both branch and server lie in the self.config, otherwise it's being skipped. Therefore one can run this standalone locally  by skipping branch as cmdline option

* the upload_blobber_files is no longer a manually called action but triggered throughout the PostScriptRun decorator
* in the same time, should anything fail with == FATAL level, the action will also be triggered by _post_fatal()
Attachment #790802 - Flags: feedback?(aki)
Comment on attachment 790802 [details] [diff] [review]
File upload mechanism from MOZ_UPLOAD_DIR

>diff --git a/configs/unittests/linux_unittest.py b/configs/unittests/linux_unittest.py
>--- a/configs/unittests/linux_unittest.py
>+++ b/configs/unittests/linux_unittest.py
>@@ -119,9 +119,13 @@ config = {
>             "halt_on_failure": True,
>             "enabled": ADJUST_MOUSE_AND_SCREEN
>         },
>     ],
>     "repos": [{"repo": "http://hg.mozilla.org/build/tools"}],
>     "minidump_stackwalk_path": MINIDUMP_STACKWALK_PATH,
>     "minidump_save_path": "%(abs_work_dir)s/../minidumps",
>     "buildbot_max_log_size": 52428800,
>+    "moz_upload_server": [
>+         "-u",
>+         "http://10.134.48.49:8080",
>+    ],
> }

I don't want to bog down too much here, since I've made a suggestion and you've implemented it.
Looking closer, I'm thinking:

Are we only going to have one upload server specifiable?
If so, we can

    "moz_upload_server": "http://10.134.48.49:8080",

and you could keep the

    [["--moz-upload-servers"],
    {"dest": "moz_upload_server",
     "help": "Blob servers's location"
    }]

though you'd want it to be singular (--moz-upload-server).

You would need to specify the -u yourself.
If we need to be able to specify a list, we don't have a good way to override, though we could

    "default_blob_upload_servers": ["http://10.134.48.49:8080",],

    [["--blob-upload-server"],
    {"dest": "blob_upload_servers",
     "action": "extend",
     "help": "Blob servers's location",
    }]

And then when you're looking for the upload servers, it's

    blob_upload_servers = self.config.get("blob_upload_servers", self.config.get("default_blob_upload_servers"))

Make sense?

I also like renaming this to blob_upload_server(s) since moz_upload_server could be an ssh/rsync upload as well.

>     def upload_blobber_files(self):
>         upload = self.query_python_path("blobberc.py")
>         dirs = self.query_abs_dirs()
>+        self.info("Get the directory from which to upload the files.")
>         try:
>             blob_dir = dirs['abs_blob_upload_dir']
>         except KeyError:
>             self.fatal("Blob upload folder config is missing.")

You're fatal()ing here... maybe you should be checking to see if the upload branch and server are set first?
You can also do a similar check, either by

    if 'abs_blob_upload_dir' in dirs:

or

    if dirs.get('abs_blob_upload_dir'):

>+            branch = ["-b", self.config['moz_upload_branch']]
>+            server = self.config['moz_upload_server']

If we follow my above suggestion for specifying the servers, we can

    server = []
    for server in self.config['blob_upload_servers']:
        server.extend(['-u', server])

>+            self.info("Files are to be uploaded with %s corresponding branch at"
>+                      " the %s location." % (branch, server))

This is printing lists... is that the desired effect?

>+    def _post_fatal(self):
>+        self.upload_blobber_files()

This *might* not be necessary.
I think the PostScriptRun methods run every time, but I'm not entirely sure.  Can you insert a fatal() in the script and see if it uploads twice?
Attachment #790802 - Flags: feedback?(aki) → feedback+
* Talked to catlee and concluded that ATM we are most likely going to use multiple hosts rather than a pound/load balancer unique-host
* upgraded the blobberc python packge (pypi) to support mutiple hosts (v. 0.2)
* ranamed branch/server cmdline args to be more consistent
* on/off blob mechanism is based on branch specification. Thus one can avoid passing <blob-upload-branch> argument and the mechanism is skipped
* there are default hosts in the config files for blob server which can be overwritten by passing <blob-upload-server> cmdline option
* eventually, the blob server will have a specific IP range from which to accept POST calls, thus we don't have to worry about developers sending files to the server while locally-testing
* Aki was right. Specific _post_fatal() in scripts is unnecessary as @PostScriptRun runs anyway and thus calls the action to handle blob uploading
* refactoring
Attachment #792288 - Flags: feedback?(aki)
Flags: needinfo?(catlee)
Comment on attachment 792288 [details] [diff] [review]
File upload mechanism from MOZ_UPLOAD_DIR

This is looking good!

>+        self.debug("Check <blob-upload-branch> and <blob-upload-server> options.")
>+        if 'blob_upload_branch' in self.config:
>+            self.info("Blob upload gear active.")

Hm, I can see a situation where someone copies our commandline, but creates their own config file that doesn't contain default_blob_upload_servers.
You probably want to check for blob_upload_branch and (blob_upload_servers or default_blob_upload_servers).

Also, what behavior do we expect when the blob upload directory either doesn't exist or is empty?
Is that handled by the python package, since I don't see anything to handle that here?
Attachment #792288 - Flags: feedback?(aki) → feedback+
> Hm, I can see a situation where someone copies our commandline, but creates
> their own config file that doesn't contain default_blob_upload_servers.
> You probably want to check for blob_upload_branch and (blob_upload_servers
> or default_blob_upload_servers).

Good point. I will update this.
 
> Also, what behavior do we expect when the blob upload directory either
> doesn't exist 

So far the creation of the folder is handled by the mozharness script, along with the env var definition (MOZ_UPLOAD_DIR)
http://hg.mozilla.org/users/mtabara_mozilla.com/mozharness/file/2be7e10c59dc/scripts/desktop_unittest.py#l360
In BlobUploadMixin I'm fatal()-ing if the folder is not returned by the query_env(). Should I move the creation of the folder in the BlobUploadMixin?

> or is empty?

Hm .. good observation about this. ATM the client does not do anything in particular. Do you want me to add a `'if not files' then log and return something` statement there?

https://github.com/catlee/blobber/blob/master/scripts/blobberc.py#L88
(In reply to Mihai Tabara [:mtabara] from comment #24)
> > Also, what behavior do we expect when the blob upload directory either
> > doesn't exist 
> 
> So far the creation of the folder is handled by the mozharness script, along
> with the env var definition (MOZ_UPLOAD_DIR)
> http://hg.mozilla.org/users/mtabara_mozilla.com/mozharness/file/2be7e10c59dc/
> scripts/desktop_unittest.py#l360
> In BlobUploadMixin I'm fatal()-ing if the folder is not returned by the
> query_env(). Should I move the creation of the folder in the BlobUploadMixin?

It's possible for us to fatal() before that line, in which case we'll try to call the PostScriptRun before it exists.

I think being able to gracefully handle the directory not existing is a good idea.

> > or is empty?
> 
> Hm .. good observation about this. ATM the client does not do anything in
> particular. Do you want me to add a `'if not files' then log and return
> something` statement there?
> 
> https://github.com/catlee/blobber/blob/master/scripts/blobberc.py#L88

Sure, that sounds good.  Alternately we could check for the existence of the directory and the presence of files inside that directory before calling the blob uploader.  I'm fine with either approach.
* safe check for both branch and server
* skipping blob upload mechanism for blob_dir corner cases
* refactoring
Attachment #792953 - Flags: feedback?(aki)
Comment on attachment 792953 [details] [diff] [review]
File upload mechanism from MOZ_UPLOAD_DIR

I think this is good :)
Thanks for your patience!
Attachment #792953 - Flags: feedback?(aki) → feedback+
* overview of the blob mechanism
* tests run successfully on dev-master (for Linux machine)
* blob server host is URL-hardcoded ATM in the config files. I suggest waiting to see how blob upload mechanism is coming along with staging/production load and depending on that to later on grant him a (DNS) name :)
Attachment #780517 - Attachment is obsolete: true
Attachment #781917 - Attachment is obsolete: true
Attachment #783348 - Attachment is obsolete: true
Attachment #788366 - Attachment is obsolete: true
Attachment #790802 - Attachment is obsolete: true
Attachment #792288 - Attachment is obsolete: true
Attachment #792953 - Attachment is obsolete: true
Attachment #793075 - Flags: review?(aki)
Posted patch Buildbot configs changeset (obsolete) — Splinter Review
* Buildbot-configs modification
Attachment #793077 - Flags: review?(aki)
Posted patch buildbot custom changeset (obsolete) — Splinter Review
* Buildbot-custom modification
Attachment #793080 - Flags: review?(aki)
Comment on attachment 793075 [details] [diff] [review]
overview - File upload mechanism from MOZ_UPLOAD_DIR

This patch is slightly bitrotted, but it's not too hard to merge.
Attachment #793075 - Flags: review?(aki) → review+
Comment on attachment 793080 [details] [diff] [review]
buildbot custom changeset

I don't like this approach because it assumes commandline option order.
If we append something else after --blob-upload-branch, we have a broken commandline.

Instead of

>     ('mochitest-1', {
>         'use_mozharness': True,
>         'script_path': 'scripts/desktop_unittest.py',
>-        'extra_args': ['--mochitest-suite', 'plain1'],
>+        'extra_args': ['--mochitest-suite', 'plain1',
>+                       '--blob-upload-branch'],
>         'script_maxtime': 7200,
>     }),

Maybe

    ('mochitest-1', {
        'use_mozharness': True,
        'script_path': 'scripts/desktop_unittest.py',
        'extra_args': ['--mochitest-suite', 'plain1'],
        'blob_upload': True,
        'script_maxtime': 7200,
    }),

and

if mozharness_suite_config.get('blob_upload'):
    extra_args.extend(["--blob-upload-branch", branch_name])

?
Attachment #793080 - Flags: review?(aki) → review-
Comment on attachment 793077 [details] [diff] [review]
Buildbot configs changeset

As above.
Attachment #793077 - Flags: review?(aki) → review-
* changed to 'blob_upload' flag
Attachment #793077 - Attachment is obsolete: true
Attachment #793716 - Flags: review?(aki)
Posted patch Buildbot custom changeset (obsolete) — Splinter Review
* changed to 'blob_upload' flag used for both branch-level and suite-level
Attachment #793080 - Attachment is obsolete: true
Attachment #793717 - Flags: review?(aki)
Comment on attachment 793717 [details] [diff] [review]
Buildbot custom changeset

You can probably make the conditional easier to read now that we're creating a True, a la

if branch_config.get('blob_upload') and suites.get('blob_upload'):
    test_builder_kwargs['mozharness_suite_config']['blob_upload'] = True

This works too though.
Attachment #793717 - Flags: review?(aki) → review+
Attachment #793716 - Flags: review?(aki) → review+
Attachment #793717 - Attachment is obsolete: true
Cloned buil/mozharness, patched
Attachment #793075 - Attachment is obsolete: true
Attachment #793753 - Flags: review?(aki)
Attachment #793753 - Flags: review?(aki) → review+
Comment on attachment 793753 [details] [diff] [review]
overview - File upload mechanism from MOZ_UPLOAD_DIR

http://hg.mozilla.org/build/mozharness/rev/5adba582be8a
Attachment #793753 - Flags: checked-in+
Live in production.
Merged to production branch of mozharness.
Live as of now.
Flags: needinfo?(catlee)
Posted patch Unit test for BlobUploadMixin (obsolete) — Splinter Review
* unit testing for BlobUploadMixin code
Attachment #796313 - Flags: review?(aki)
* unit testing for BlobUploadMixin 
* small logging typo in BlobUploadMixin
Attachment #796313 - Attachment is obsolete: true
Attachment #796313 - Flags: review?(aki)
Attachment #796330 - Flags: review?(aki)
Attachment #796330 - Flags: review?(aki) → review+
Comment on attachment 796330 [details] [diff] [review]
Unit test for BlobUploadMixin + typo in BlobUploadMixin

https://hg.mozilla.org/build/mozharness/rev/71e9d27e5e26
Attachment #796330 - Flags: checked-in+
Depends on: 910840
In production.
* instead of 64encoding to log, the screenshots go directly as files in MOZ_UPLOAD_DIR from where are to be uploaded within the blob gear
Attachment #800363 - Flags: review?(ted)
Blocks: 913244
Comment on attachment 800363 [details] [diff] [review]
dumpScreen() simplified so that all screenshots go as files in MOZ_UPLOAD_DIR

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

The content of this patch looks okay, but I'd like it to be a bit cleaner for landing. Also, we shouldn't land this on mozilla-{inbound,central} until the blob upload stuff lands there, so you'll have to land this directly on Cedar I suppose.

Please generate a patch for checkin without the whitespace removal. jhammel is working on a large refactoring in bug 746243, and I don't want you to bitrot him any more than necessary.

Also, you can remove a lot more code while you're at it, so you might as well.

::: build/automation.py.in
@@ +639,3 @@
>  
> +    # Always write screenshots to a file - bug 749421 related
> +    imgoutput = 'file'

The comment isn't really necessary, and you can get rid of the imgoutput variable.

@@ +651,3 @@
>      try:
> +        tmpfd, imgfilename = tempfile.mkstemp(prefix='mozilla-test-fail-screenshot_',
> +                                              dir=parent_dir)

Since you're rewriting half of this anyway, you can replace this with:
with mozfile.NamedTemporaryFile(prefix=..., dir=...) as f:
  returncode = subprocess.call(utility + [imgfilename])

(and a bunch of the code below, since you don't need its stdout anymore etc)
Attachment #800363 - Flags: review?(ted)
* simplified the dumpScreen() method to always output to a file and write it down to MOZ_UPLOAD_DIR
Attachment #800363 - Attachment is obsolete: true
Attachment #801002 - Flags: review?(ted)
* simplified dumpScreen() to write screenshots to file in MOZ_UPLOAD_DIR
Attachment #801002 - Attachment is obsolete: true
Attachment #801002 - Flags: review?(ted)
Attachment #801031 - Flags: review?(ted)
Comment on attachment 801031 [details] [diff] [review]
dumpScreen() simplified so that all screenshots go as files in MOZ_UPLOAD_DIR

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

Like I said before, please attach a patch without the whitespace cleanup for final checkin.

::: build/automation.py.in
@@ +624,5 @@
>      if self.haveDumpedScreen:
>        self.log.info("Not taking screenshot here: see the one that was previously logged")
>        return
>      self.haveDumpedScreen = True;
> +    # Need to figure out which OS-dependant tool to use

spelling: dependent

@@ +633,4 @@
>      elif self.IS_WIN32:
>        utility = [os.path.join(utilityPath, "screenshot.exe")]
> +    # Always write screenshots to a file - bug 749421 related
> +    imgoutput = 'file'

Drop the comment and the imgoutput variable.

@@ +636,5 @@
> +    imgoutput = 'file'
> +
> +    # Get dir where to write the file
> +    env = self.environment()
> +    parent_dir = env.get('MOZ_UPLOAD_DIR', '')

nit: I'd probably use None for the default value.

@@ +646,4 @@
>      try:
> +        tmpfd, imgfilename = tempfile.mkstemp(suffix='.png')
> +                                              prefix='mozilla-test-fail-screenshot_',
> +                                              dir=parent_dir)

In my previous review I suggested you should use mozfile.NamedTemporaryFile.

@@ +650,2 @@
>          os.close(tmpfd)
>          dumper = self.Process(utility + [imgfilename])

and also subprocess.call here.
Attachment #801031 - Flags: review?(ted)
(In reply to Mihai Tabara [:mtabara] from comment #52)
> Created attachment 801031 [details] [diff] [review]
> dumpScreen() simplified so that all screenshots go as files in MOZ_UPLOAD_DIR
> 
> * simplified dumpScreen() to write screenshots to file in MOZ_UPLOAD_DIR

I don't understand if/how the linux `screentopng` tool is writing to the file; am I missing something or does it need its output redirected?
(In reply to Jeff Hammel [:jhammel] from comment #54)
> I don't understand if/how the linux `screentopng` tool is writing to the
> file; am I missing something or does it need its output redirected?

I fixed that in bug 910363. That should have been added as a dep.
Depends on: 910363
* simplified dumpScreen() to generate screenshots as files only to MOZ_UPLOAD_DIR
Attachment #801031 - Attachment is obsolete: true
Attachment #801885 - Flags: review?(ted)
Comment on attachment 801885 [details] [diff] [review]
dumpScreen() simplified so that all screenshots go as files in MOZ_UPLOAD_DIR

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

Thanks, this looks good! Are you going to land this on Cedar?
Attachment #801885 - Flags: review?(ted) → review+
Depends on: 920622
Comment on attachment 812393 [details] [diff] [review]
Blobber user/passwd sample in buildbot-configs template

https://hg.mozilla.org/build/buildbot-configs/rev/3c2235479ed4
Attachment #812393 - Flags: review?(rail)
Attachment #812393 - Flags: review+
Attachment #812393 - Flags: checked-in+
Comment on attachment 812397 [details] [diff] [review]
Activate flag to get oauth.txt file with credentials in buildbotcustom

https://hg.mozilla.org/build/buildbotcustom/rev/1487b120649f
Attachment #812397 - Flags: review?(rail)
Attachment #812397 - Flags: review+
Attachment #812397 - Flags: checked-in+
* Blob client is using SSL + basic http authentication to upload files to elastic beanstalk. Thus, when calling the client script a file containing the auth credentials is to be given ( https://github.com/MihaiTabara/blobuploader/blob/master/blobberc.py#L137 )

* should I add the config param as blobupload_config_options as well?
Attachment #812408 - Flags: review?(aki)
Attachment #812408 - Flags: review?(aki) → review+
* point to secure elasticbeanstalk new blob server

! Please do not yet land this
Attachment #812705 - Flags: review?(aki)
Attachment #812705 - Flags: review?(aki) → review+
Attachment #812668 - Flags: review?(dustin) → review+
Attachment #813114 - Flags: review?(rail) → review+
something here is in production
Attachment #812705 - Flags: checked-in+
Attachment #813114 - Flags: checked-in+
Comment on attachment 813167 [details] [diff] [review]
Fix tests in mozharness for new blobuploader version

https://hg.mozilla.org/build/mozharness/rev/e7d96b1fb10a
Attachment #813167 - Flags: review?(rail)
Attachment #813167 - Flags: review+
Attachment #813167 - Flags: checked-in+
in production
Comment on attachment 816028 [details] [diff] [review]
Fix blobuploader in mozharness to 1.0b

https://hg.mozilla.org/build/mozharness/rev/b378d511e8a9
Attachment #816028 - Flags: review?(rail)
Attachment #816028 - Flags: review+
Attachment #816028 - Flags: checked-in+
What is remaining to be done here for this to be used in production?
We are to announce it these days following which we'll be switching it on, gradually, branch by branch.
something here is in mozharness production
Comment on attachment 818068 [details] [diff] [review]
Switch on blobber to try branch

https://hg.mozilla.org/build/buildbot-configs/rev/7e0e163eacb3
Attachment #818068 - Flags: review?(rail)
Attachment #818068 - Flags: review+
Attachment #818068 - Flags: checked-in+
Depends on: 928058
Comment on attachment 818760 [details] [diff] [review]
Fix tests in mozharness for python path fixing

https://hg.mozilla.org/build/mozharness/rev/dc82c4906da1
Attachment #818760 - Flags: review?(rail)
Attachment #818760 - Flags: review+
Attachment #818760 - Flags: checked-in+
please allow file type .etl to be uploaded ( https://github.com/catlee/blobber/blob/master/blobber/config.py#L12 ).  This will be useful for talos xperf.
Comment on attachment 819150 [details] [diff] [review]
Add blobber gear to Andron Panda test

> -            self.warning("We failed to create the shutdown file: str(%s)" % str(e))
> +            self.warning("We failed to create the shutdown file: str(%s)" % str(e))

I can't seem to figure out what's changed about this line?
Attachment #819150 - Flags: review?(aki) → review+
Attachment #819192 - Flags: review?(aki) → review+
something here merged to production mozharness
* completely forgot about configs and buildbot-configs
* works fine on staging master!
Attachment #819150 - Attachment is obsolete: true
Attachment #821978 - Flags: review?(aki)
Attachment #821978 - Flags: review?(aki) → review+
Attachment #821979 - Flags: review?(aki) → review+
Attachment #819192 - Attachment is obsolete: true
Comment on attachment 821979 [details] [diff] [review]
Buildbot-configs patch to enable blobber for all test suites in Android Panda

https://hg.mozilla.org/build/buildbot-configs/rev/4507910edc52
Attachment #821979 - Flags: checked-in+
something here is in production
* checked successfully on staging master
Attachment #822526 - Flags: review?(aki)
Comment on attachment 822526 [details] [diff] [review]
Add blobber gear to Android Panda Talos tests

lgtm.
Attachment #822526 - Flags: review?(aki) → review+
Attachment #822528 - Flags: review?(aki) → review+
Comment on attachment 822526 [details] [diff] [review]
Add blobber gear to Android Panda Talos tests

https://hg.mozilla.org/build/mozharness/rev/46b4768c6f1f
Attachment #822526 - Flags: checked-in+
Comment on attachment 822528 [details] [diff] [review]
Tune buildbotcustom for Talos tests for branch blobber-enabled only

https://hg.mozilla.org/build/buildbotcustom/rev/210636103d4a
Attachment #822528 - Flags: checked-in+
in production
Comment on attachment 822528 [details] [diff] [review]
Tune buildbotcustom for Talos tests for branch blobber-enabled only

Backed out since this may have caused windows talos bustage.
Attachment #822528 - Flags: checked-in+ → checked-in-
Attachment #823640 - Attachment is obsolete: true
Attachment #823640 - Flags: review?(rail)
Comment on attachment 822528 [details] [diff] [review]
Tune buildbotcustom for Talos tests for branch blobber-enabled only

https://hg.mozilla.org/build/buildbotcustom/rev/ee9161ac33bb
back to default
Attachment #822528 - Attachment is obsolete: false
Attachment #822528 - Flags: checked-in- → checked-in+
in production
Attachment #824297 - Attachment is obsolete: true
Attachment #824297 - Flags: review?(aki)
Attachment #824300 - Flags: review?(aki)
Attachment #824299 - Flags: review?(aki) → review+
Attachment #824300 - Flags: review?(aki) → review+
Catlee: FYI, if we rename oauth.txt, we'll have to time it so ScriptFactory and these mozharness configs change at the same time.
(In reply to Aki Sasaki [:aki] from comment #112)
> Catlee: FYI, if we rename oauth.txt, we'll have to time it so ScriptFactory
> and these mozharness configs change at the same time.

This might have already sprung to your mind, but an alternative to timing this is to download the file to both places, make the mozharness change, and then stop downloading to oauth.txt in a second change later.
Comment on attachment 824299 [details] [diff] [review]
Tune buildbotcustom for full Talos blobber integration

http://hg.mozilla.org/build/buildbotcustom/rev/a9f6adc7dcbb
Attachment #824299 - Flags: checked-in+
mozharness merged & live in production.
Posted patch fix_blobberSplinter Review
This is mostly whitespace+unused import cleanup.
I also found that we switched back to importing VirtualenvMixin instead of BlobUploadMixin here: http://hg.mozilla.org/build/mozharness/rev/b29095553ca1

This patch should fix all of the above.
Attachment #825662 - Flags: review?(tabara.mihai)
Comment on attachment 825662 [details] [diff] [review]
fix_blobber

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

Thanks for the heads-up Aki.
Comment on attachment 825662 [details] [diff] [review]
fix_blobber

r+ over IRC.
http://hg.mozilla.org/build/mozharness/rev/d29c0525efd7
Attachment #825662 - Flags: review?(tabara.mihai)
Attachment #825662 - Flags: review+
Attachment #825662 - Flags: checked-in+
* not sure I need to patch all those configs files? (I feel like win_config and prod_config would be enough) ?
Attachment #826017 - Flags: review?(aki)
Attachment #826019 - Flags: review?(aki) → review+
Comment on attachment 826017 [details] [diff] [review]
Add blobber gear to Marionette script

You're definitely touching more config files than needed for this pass, but it's probably harmless... when you add 'blob_upload': True in the various marionette test suites in http://hg.mozilla.org/build/buildbot-configs/file/144a6d68d917/mozilla-tests/b2g_config.py , you'll be set.
Attachment #826017 - Flags: review?(aki) → review+
* thanks for the heads-up. Missed that :-)
Attachment #826019 - Attachment is obsolete: true
Attachment #826079 - Flags: review?(aki)
Comment on attachment 826079 [details] [diff] [review]
Tune buildbot-configs to enable blobber for Marionette

I think GAIA_UI can be toggled on as well; r=me with or without that.
Attachment #826079 - Flags: review?(aki) → review+
in production
What is the status of this?  Is it usable yet?  I see the bug is still open, but there are also a bunch of patches that have landed so I'm not sure. :)
Aiui, this is enabled for a number of test suites, on a subset of branches (ash, cedar, pine, try)
The test suites enabled are: android (pandas and talos), desktop unit tests, marionette and Talos.
It's still restricted to (ash, cedar, pine, try) until we fix a bunch of yet-opened-issues.

See bugs: 938543, 938339.
Blocks: 938745
Is there a way to set it up so we don't need to add this to each chunk?
Attachment #832974 - Flags: review?(aki)
Attachment #832964 - Flags: review?(aki) → review+
Attachment #832974 - Flags: review?(aki) → review+
(In reply to Andrew Halberstadt [:ahal] from comment #132)
> Created attachment 832974 [details] [diff] [review]
> Tune buildbot-configs to enable blobber for b2g emulator unittests
> 
> Is there a way to set it up so we don't need to add this to each chunk?

Yes, change this line
http://hg.mozilla.org/build/buildbotcustom/file/8c8a2610f748/misc.py#l1996
to 

if branch_config.get('blob_upload') and suites.get('blob_upload', True)

I think we didn't do that because not every test script was ready for it, and sending the unknown commandline arguments might break things.
Note the previous buildbot-config patch should enable blob_upload for this script as well.
Attachment #833077 - Flags: review?(aki)
Comment on attachment 832964 [details] [diff] [review]
Add blobber gear to b2g emulator unittest script

https://hg.mozilla.org/build/mozharness/rev/0658d7a63efb
Attachment #832964 - Flags: checked-in+
Attachment #833077 - Flags: review?(aki) → review+
Comment on attachment 833077 [details] [diff] [review]
Add blobber gear to b2g desktop unittest script

https://hg.mozilla.org/build/mozharness/rev/a166ab38d937
Attachment #833077 - Flags: checked-in+
Comment on attachment 832974 [details] [diff] [review]
Tune buildbot-configs to enable blobber for b2g emulator unittests

https://hg.mozilla.org/build/buildbot-configs/rev/b89a82d65690
Attachment #832974 - Flags: checked-in+
Missed an import in the b2g desktop changes, here's the follow-up fix: https://hg.mozilla.org/build/mozharness/rev/1f488900b827
something[s] here made it to production
Depends on: 943565
Just making a note here.. when enabling blobber on new branches, please also do so for b2g_config.py which uses a separate (but equivalent) dictionary. Example: http://hg.mozilla.org/build/buildbot-configs/rev/8de5f739eab9
Any reason we shouldn't enable it on all branches?
Can we (finally :-D) close this bug once bug 956035 is confirmed to not have damaged anything?
Woot! We should get attachment 801885 [details] [diff] [review] landed on mozilla-inbound.
Depends on: 957447
Let's call this done. I'm going to move attachment 801885 [details] [diff] [review] to a new bug because I think the patch needs updating.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 961205
Could somebody provide a quick summary of what got done here? This seems like a great feature, but I have no idea how to use it.
Executive summary: test runs will have an environment variable set (MOZ_UPLOAD_DIR) which is a path to a directory where you can write files. Any files in that directory (subject to a file extension whitelist) will be uploaded to a publicly-accessible server at the end of the run.

whitelist: https://github.com/catlee/blobber/blob/master/blobber/config.py#L12
Excellent! What's the server? Or will that appear somewhere in the log at the end of the run or something?
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.