This repository has been archived by the owner. It is now read-only.

Implement postbuild actions to upload artifacts to S3 and submit reports to Treeherder #671

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@whimboo
Copy link
Contributor

whimboo commented Oct 9, 2015

This PR will fix issue #661. Please keep in mind that this PR is not ready yet. I have to test it across platforms.

@whimboo whimboo self-assigned this Oct 9, 2015

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Oct 9, 2015

@mjzffr and @sydvicious, I would let you know that this is the PR for the treeherder submission changes including the postbuild step for the completed submission. It changes quiet a lot, so I tried to create some smaller commits even I wasn't able to do that from the beginning given that a bit of experimentation was necessary. It was not that straight forward. I will add some inline comments to explain some things better.

@@ -89,24 +89,33 @@
<selector class="hudson.plugins.copyartifact.StatusBuildSelector"/>
</hudson.plugins.copyartifact.CopyArtifact>
<hudson.plugins.xshell.XShellBuilder plugin="xshell@0.9-SNAPSHOT">
<commandLine>python runtests.py --type=functional --branch=mozilla-aurora --build-revision=$REVISION --build-locale=$LOCALE --installer-url=$INSTALLER_URL</commandLine>
<commandLine>python -u submission.py --test-type=functional --state=running --repository=mozilla-aurora --revision=$REVISION --locale=$LOCALE treeherder_venv</commandLine>

This comment has been minimized.

@whimboo

whimboo Oct 9, 2015

Contributor

As you will see in the script we dynamically build-up the necessary treeherder data, also by specifying settings for each kind of test. For media tests we most likely will use --type=media later.

This comment has been minimized.

@whimboo

whimboo Oct 9, 2015

Contributor

I wonder what we should do here if the script is failing to submit data. Probably we should abort the run?

This comment has been minimized.

@mjzffr

mjzffr Oct 13, 2015

Yeah, aborting the entire test run makes sense, as long as we notice right away.

On pf-jenkins, there were a couple of times when Treeherder submission broke (due to a quiet API change, for example) and we only noticed after a couple of days. All our tests still ran as usual, despite the Treeherder issues, which was good -- at least we knew the tests were still passing.

This comment has been minimized.

@whimboo

whimboo Oct 13, 2015

Contributor

Great. So I will leave the code as is. Thanks!

<executeFromWorkingDir>false</executeFromWorkingDir>
</hudson.plugins.xshell.XShellBuilder>
<hudson.plugins.xshell.XShellBuilder plugin="xshell@0.9-SNAPSHOT">
<commandLine>python runtests.py --type=functional --installer-url=$INSTALLER_URL</commandLine>

This comment has been minimized.

@whimboo

whimboo Oct 9, 2015

Contributor

This wrapper script will also survive the mozharness flip later given that it will store the exit code of the script, which we need in the postbuild step to determine the result of the run.

This comment has been minimized.

@mjzffr

mjzffr Oct 13, 2015

To capture the result accurately, we'll need to keep track of both the mozharness exit code and the marionette exit code (or parse the test logs). For example, I think mozharness exits successfully as long as all the required steps complete; the exit code doesn't capture the test result.

If the run-tests step in mozharness hits an output-timeout, does the mozharness exit code reflect that? I'm not sure it does.

This comment has been minimized.

@whimboo

whimboo Oct 13, 2015

Contributor

For this PR I cannot test this because its not mozharness yet. But the mozharness script should not exit with 0 if marionette fails but should reflect its exit value. I will check this with the next big PR which will include the mozharness inclusion.

<executeFromWorkingDir>false</executeFromWorkingDir>
</hudson.plugins.xshell.XShellBuilder>
</builders>
<publishers>
<hudson.tasks.ArtifactArchiver>
<artifacts>*.xml,*.html,*.log</artifacts>
<artifacts>build/upload/*.*</artifacts>

This comment has been minimized.

@whimboo

whimboo Oct 9, 2015

Contributor

Everything to upload is located in the upload dir now. Both junit and html we no longer need given that we have treeherder.

@@ -1,3 +1,3 @@
boto==2.38.0
mozdownload==1.18
mozinfo==0.8

This comment has been minimized.

@whimboo

whimboo Oct 9, 2015

Contributor

This file is only used for the treeherder venv now. So mozdownload is not needed, but mozinfo.


class JenkinsDefaultValueAction(argparse.Action):
"""Jenkins cannot handle optional arguments to a CLI script so we pass in
a 'None' value by default.

This comment has been minimized.

@whimboo

whimboo Oct 9, 2015

Contributor

I don't know how to circumvent this in Jenkins yet in case of optional arguments.

This comment has been minimized.

@mjzffr

mjzffr Oct 13, 2015

I've never encountered this limitation. Without the above fix, what happens when you use a command that omits an optional argument? Can you give an example?

This comment has been minimized.

@whimboo

whimboo Oct 13, 2015

Contributor

We are using the XShell plugin for executing scripts. That was necessary for Mozmill given that the entry script was a bash script or a batch file. Maybe it is a bug in that plugin. Given that we call the Python scripts directly, we may be able to get rid of it. I will test that when I'm back on Friday.

This comment has been minimized.

@whimboo

whimboo Oct 16, 2015

Contributor

So the situation here is a combination of parameters from the parametrized plugin and executing a command via the shell in a build step. The parameters for the job contain a value and not a name + value. That means when we call the shell command we always have to pass the name and value and therefore need the name hard-coded in the command line. Best would be if the default of a job parameter is set, that this option is not passed to the shell script. But I don't see how this can be done in Jenkins.

setattr(namespace, self.dest, values)


class FirefoxUIResultParser(object):

This comment has been minimized.

@whimboo

whimboo Oct 9, 2015

Contributor

Please keep in mind that this is not the version we will use once we migrated to mozharness. I will update all that code similar to what Maja is using in her media tests. This is mostly a 1-1 copy beside the new status property which I already took from Maja's code.

@@ -0,0 +1,10 @@
virtualenv.egg-info

This comment has been minimized.

@whimboo

whimboo Oct 9, 2015

Contributor

All the following files are a full copy of the virtualenv distribution.

This comment has been minimized.

@whimboo

whimboo Oct 9, 2015

Contributor

Maybe I should add it as zip and let the environment.py script unzip it. What do you think?

This comment has been minimized.

@sydvicious

sydvicious Oct 12, 2015

Contributor

Mirroring this is not ideal. A .zip file is better. Is there a reason it's not just an environment requirement?

This comment has been minimized.

@sydvicious

sydvicious Oct 12, 2015

Contributor

I just read your justification in email. Still bothers me, but you have your reasons. A .zip file would be better.

This comment has been minimized.

@whimboo

whimboo Oct 16, 2015

Contributor

As discussed earlier this week and in our meeting we will completely remove the dependency on that external tool and get it installed globally instead. This work gets being done in bug 1212922. So I will completely get rid of all of those files in that PR.

"""
def __init__(self, option_strings, dest, nargs=None, **kwargs):
if nargs is not None:
raise ValueError("nargs not allowed")

This comment has been minimized.

@sydvicious

sydvicious Oct 12, 2015

Contributor

This error message should be better. Not a big deal, but this tells me nothing if I set something up incorrectly.

This comment has been minimized.

@whimboo

whimboo Oct 16, 2015

Contributor

Actually this is not necessary at all given that we do not have lists at all. Also I'm handling lists in the __call__ method. :) So I will remove this check.

required=True,
help='The revision of the build.')
parser.add_argument('--state',
choices=BUILD_STATES,

This comment has been minimized.

@sydvicious

sydvicious Oct 12, 2015

Contributor

Could this be "--build-state"? Just "--state" is kind of ambiguous.

This comment has been minimized.

@whimboo

whimboo Oct 16, 2015

Contributor

Sure. I will get this updated.

@sydvicious

This comment has been minimized.

Copy link
Contributor

sydvicious commented Oct 12, 2015

In general, this looks good. Some concerns have been outlined.

# Create the test environment and activate it
# TODO remove once we make use of the mozharness script
import environment
command = ['python', os.path.join(here, 'firefox-ui-tests', 'create_venv.py'),

This comment has been minimized.

@mjzffr

mjzffr Oct 13, 2015

Why not use environment.py here? It seems the create_venv.py can be removed from firefox-ui-tests in favour of environment.py.

This comment has been minimized.

@whimboo

whimboo Oct 13, 2015

Contributor

This is only temporarily. With the next PR (switch to mozharness) this code is not necessary anymore because mozharness takes care of the environment creation. Because of that I don't want to pollute environment.py with this call.

here = os.path.dirname(os.path.abspath(__file__))

LOOKUP_FRAGMENT = 'api/project/{repository}/resultset/?revision={revision}'
REVISON_FRAGMENT = '/#/jobs?repo={repository}&revision={revision}'

This comment has been minimized.

@mjzffr

mjzffr Oct 13, 2015

[Nit] The names LOOKUP_FRAGMENT and REVISION_FRAGMENT do not distinguish the two strings and it's not clear what they are intended for. Maybe RESULTSET_FRAGMENT and JOB_FRAGMENT would be clearer.

This comment has been minimized.

@whimboo

whimboo Oct 13, 2015

Contributor

Makes total sense. I will update both constants.

failures = {'all_errors': [], 'errors_truncated': True}

for failure in self.failures:
failures['all_errors'].append({'line': failure, 'linenumber': None})

This comment has been minimized.

@mjzffr

mjzffr Oct 13, 2015

'linenumber': None prevent the failure summary from being displayed in the Treeherder log viewer. You can use 1 for all failures instead of None to get around this for now.

This comment has been minimized.

@whimboo

whimboo Oct 13, 2015

Contributor

Oh! If that really works it would be awesome. Thanks for that great hint. I will try it out immediately.

This comment has been minimized.

@whimboo

whimboo Oct 13, 2015

Contributor

It doesn't seem to make it work for the current code. Maybe we should wait for the updated output parser of the mozharness created logs? Means the code you have in your media tests right now. I wouldn't spend to much time on that part now which will be replaced anyway in a couple of days.

This comment has been minimized.

@whimboo

whimboo Oct 13, 2015

Contributor

Oh, here an example link: https://treeherder.allizom.org/logviewer.html#?job_id=2005398&repo=mozilla-central

Strangely this functional test has been submitted as an update test. Maybe I missed to reset the delete workspace option so the json job file was already present.

@mjzffr

This comment has been minimized.

Copy link

mjzffr commented Oct 13, 2015

That's all I can think of. :) Mostly a few clarification questions. Ping me again if you'd like more input on an updated PR.

@whimboo whimboo force-pushed the whimboo:postbuild branch from b6d18b5 to 6bb5e9f Oct 16, 2015

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Oct 16, 2015

I got all the review comments implemented now. I pushed some new commits with the changes. On Monday I will run tests across branches and platforms to verify all is working as expected. If you both still have time to cross-check the current state please do so. Thanks.

@mjzffr

This comment has been minimized.

Copy link

mjzffr commented Oct 16, 2015

Looks good. I especially like that JenkinsDefaultValueAction is in its own module now. 👍

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Oct 19, 2015

I pushed a small update for the virtualenv location. With that mozilla-central_functional works fine across all platforms.

I'm now checking all other jobs if they are passing.

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Oct 19, 2015

Two more commits were necessary to fix remaining issues with other jobs. @sydvicious and @mjzffr if you want to have a look at those again?

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Oct 20, 2015

Maybe wait with the review until I updated this PR for the Hawk credentials.

@whimboo whimboo force-pushed the whimboo:postbuild branch 2 times, most recently from 87fca9b to 83d30ed Oct 20, 2015

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Oct 20, 2015

@mjzffr and @sydvicious the PR is up-to-date for hawk credentials now. Beside that I still found some kinda important bugs for treeherder submission which I fixed now. Sadly we missed them all so far. Also the rebase didn't went that perfect so 2 more commits were necessary. I checked locally against origin/postbuild for missing items and got them fixed all. Please have another look.

command = ['python', 'create_venv.py', self.venv_path]
# Can only be imported after the environment has been activated
import firefox_ui_tests
import firefox_puppeteer

This comment has been minimized.

@sydvicious

sydvicious Oct 20, 2015

Contributor

What happens? Are there namespace collisions? Shouldn't this be fixed by using fully-qualified names?

This comment has been minimized.

@sydvicious

sydvicious Oct 20, 2015

Contributor

Oh, I see. This script is running naked without a virtual env, and environment.create makes sure all of the requirements are there so that code following this will work.

The comment needs to make this clearer.

This comment has been minimized.

@whimboo

whimboo Oct 20, 2015

Contributor

I would leave that for now. As the comment above says it will be removed in a bit with the mozharness changes.

@@ -1,3 +1,3 @@
boto==2.38.0
mozdownload==1.18

This comment has been minimized.

@sydvicious

sydvicious Oct 20, 2015

Contributor

You have moved this requirement, but see below...

This comment has been minimized.

@whimboo

whimboo Oct 20, 2015

Contributor

Not sure what you mean here? mozdownload is not needed for the treeherder virtualenv but mozinfo is. That's what I wrote below.

This comment has been minimized.

@sydvicious

sydvicious Oct 21, 2015

Contributor

The remaining import from mozdownload has since been removed. Ignore this comment.


print('Calling command to create virtual environment: %s' % command)
subprocess.check_call(command)
from mozdownload import FactoryScraper

This comment has been minimized.

@sydvicious

sydvicious Oct 20, 2015

Contributor

...didn't you remove the mozdownload requirement?

This comment has been minimized.

@whimboo

whimboo Oct 20, 2015

Contributor

This is in runtests which creates the environment via https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/create_venv.py. All this will be removed with mozharness.

except OSError as e:
print('Failed to save return value: {}'.format(e))

# Delete http.log if tests were passing

This comment has been minimized.

@sydvicious

sydvicious Oct 20, 2015

Contributor

Really? Even it is "passing", if there are silent failures or non-deterministic behaviors, it might be good to have this sitting around.

This comment has been minimized.

@whimboo

whimboo Oct 20, 2015

Contributor

Those files can be large so I didn't wanted to upload them in case everything passes. So far we never had a need to check the HTTP log in case of passing tests.

This comment has been minimized.

@whimboo

whimboo Oct 20, 2015

Contributor

@mjzffr what would you say?

else:
environment.create(kwargs['venv_path'], os.path.join(here, 'requirements.txt'))

# Can only be imported after the environment has been activated

This comment has been minimized.

@sydvicious

sydvicious Oct 20, 2015

Contributor

Oh, I see. This script is running naked without a virtual env, and environment.create makes sure all of the requirements are there so that code following this will work.

The comment needs to make this clearer.

@sydvicious

This comment has been minimized.

Copy link
Contributor

sydvicious commented Oct 20, 2015

Just a few more clarifications, @whimboo . This is good work.

if settings['logs'].get('gecko.log'):
os.makedirs(os.path.dirname(settings['logs']['gecko.log']))
except OSError:
pass

This comment has been minimized.

@mjzffr

mjzffr Oct 20, 2015

Let's log the error here.

This comment has been minimized.

@whimboo

whimboo Oct 20, 2015

Contributor
  •        if settings['logs'].get('gecko.log'):
    
  •            os.makedirs(os.path.dirname(settings['logs']['gecko.log']))
    
  •    except OSError:
    
  •        pass
    

Let's log the error here.

Done with the commit as uploaded a minute ago.

@whimboo whimboo force-pushed the whimboo:postbuild branch 2 times, most recently from fcb7a01 to e445cce Oct 21, 2015

@whimboo whimboo closed this Oct 27, 2015

@whimboo whimboo force-pushed the whimboo:postbuild branch from e445cce to 958946b Oct 27, 2015

@whimboo whimboo deleted the whimboo:postbuild branch Oct 27, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.