1131790 - Move clobbers into a runner task so that they can be batched

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Morgan Phillips [:mrrrgn]]

Right now, clobbers happen during a buildbot job. This isn't ideal, since it loses us the ability to batch clobbers. The runner task should, using only the system's hostname, pull down a list of requested clobbers and act on them.

This will likely require adding new RelEngAPI-Clobberer endpoints.

Comment 1

[Morgan Phillips [:mrrrgn]]

Attachment: https://github.com/mozilla/build-tools/pull/4

This allows clobberer to work with a new 'lastclobber/all' endpoint and moves support for the old 'lastclobber' endpoint to a legacy mode.

Comment 2

[Morgan Phillips [:mrrrgn]]

Attachment: clobberer.client.diff

A proper hg diff

Comment 3

[Rail Aliiev [:rail]]

Comment on attachment 8569268 [details] [diff] [review]
clobberer.client.diff

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

::: clobberer/clobberer.py
@@ +55,5 @@
> +    higher than --build.
> +    >>>sanitize_builddir('../../abc')
> +    //abc
> +    """
> +    return builddir.replace('.', '')

Hmm, I think this will hurt us with directories with legit dots. sanitize_builddir("a.directory") would be "adirectory".

Maybe just:
* construct the final absolute path (os.path.join + abspath)
* normalize it (normpath + realpath)
* compare to basedir (commonprefix?) to figure out if it's under it
* fail if it's not

Or just get rid of it and file a separate bug.

::: clobberer/test_clobberer.py
@@ +80,5 @@
> +        fake_data_null = {'result': []}
> +        fake_data_json_str = json.dumps(fake_data_null)
> +        urllib2_mock = mock.Mock()
> +        urllib2_mock.read.side_effect = [fake_data_json_str]
> +        mock_urlopen.return_value = urllib2_mock

I think you can do something like

mock_urlopen.return_value.read.return_value = [fake_data_json_str]

Can you try it to get rid of the boilerplate?

@@ +95,5 @@
>  
> +        fake_data_str = lastclobber_fmt.format(builddir, timestamp, who)
> +        urllib2_mock = mock.Mock()
> +        urllib2_mock.read.side_effect = [fake_data_str]
> +        mock_urlopen.return_value = urllib2_mock

the same here

@@ +124,5 @@
>          # make sure it can handle no data
> +        fake_data_str = ""
> +        urllib2_mock = mock.Mock()
> +        urllib2_mock.read.side_effect = [fake_data_str]
> +        mock_urlopen.return_value = urllib2_mock

and here

Comment 4

[Morgan Phillips [:mrrrgn]]

(In reply to Rail Aliiev [:rail] from comment #3)
> Comment on attachment 8569268 [details] [diff] [review]
> clobberer.client.diff
> 
> Review of attachment 8569268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: clobberer/clobberer.py
> @@ +55,5 @@
> > +    higher than --build.
> > +    >>>sanitize_builddir('../../abc')
> > +    //abc
> > +    """
> > +    return builddir.replace('.', '')
> 
> Hmm, I think this will hurt us with directories with legit dots.
> sanitize_builddir("a.directory") would be "adirectory".
> 
> Maybe just:
> * construct the final absolute path (os.path.join + abspath)
> * normalize it (normpath + realpath)
> * compare to basedir (commonprefix?) to figure out if it's under it
> * fail if it's not
> 
> Or just get rid of it and file a separate bug.
> 
> ::: clobberer/test_clobberer.py
> @@ +80,5 @@
> > +        fake_data_null = {'result': []}
> > +        fake_data_json_str = json.dumps(fake_data_null)
> > +        urllib2_mock = mock.Mock()
> > +        urllib2_mock.read.side_effect = [fake_data_json_str]
> > +        mock_urlopen.return_value = urllib2_mock
> 
> I think you can do something like
> 
> mock_urlopen.return_value.read.return_value = [fake_data_json_str]
> 
> Can you try it to get rid of the boilerplate?
> 
> @@ +95,5 @@
> >  
> > +        fake_data_str = lastclobber_fmt.format(builddir, timestamp, who)
> > +        urllib2_mock = mock.Mock()
> > +        urllib2_mock.read.side_effect = [fake_data_str]
> > +        mock_urlopen.return_value = urllib2_mock
> 
> the same here
> 
> @@ +124,5 @@
> >          # make sure it can handle no data
> > +        fake_data_str = ""
> > +        urllib2_mock = mock.Mock()
> > +        urllib2_mock.read.side_effect = [fake_data_str]
> > +        mock_urlopen.return_value = urllib2_mock
> 
> and here

causes:

vagrant@vagrant-ubuntu-trusty-64:/tmp/tools/clobberer$ nosetests
..E......
======================================================================
ERROR: test_get_clobber_times (test_clobberer.TestClobbererClient)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "build/bdist.linux-x86_64/egg/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/tmp/tools/clobberer/test_clobberer.py", line 91, in test_get_clobber_times
    clobber_dates_return = get_clobber_times('clobberer/lastclobber/all')
  File "/tmp/tools/clobberer/clobberer.py", line 169, in get_clobber_times
    data = urllib2.urlopen(clobber_url, timeout=30).read()
AttributeError: 'list' object has no attribute 'read'
-------------------- >> begin captured logging << --------------------
root: INFO: Checking clobber URL: clobberer/lastclobber/all
--------------------- >> end captured logging << ---------------------

Comment 5

[Morgan Phillips [:mrrrgn]]

Attachment: clobberer.client.diff

So, this refactors the tests to remove a good deal of boiler plate; though it still uses the mock "side effect" method.

I also went with catlee's safe_join method, stolen from: https://github.com/mozilla/build-mar/blob/master/mardor/utils.py#L11

Comment 6

[Rail Aliiev [:rail]]

Comment on attachment 8569393 [details] [diff] [review]
clobberer.client.diff

Ship it!


       _~
    _~ )_)_~
    )_))_))_)
    _!__!__!_
    \______t/
  ~~~~~~~~~~~~~

Comment 7

[Morgan Phillips [:mrrrgn]]

Attachment: https://github.com/mozilla/build-tools/pull/5

Comment 8

[Morgan Phillips [:mrrrgn]]

Attachment: clobberer.client.diff

So, the lack of argparse (python2.6) turned into whack-a-mole. I put out a bug to explicitly call clobberer with python2.7 in our build scripts; but for the sake of sanity I reverted this version to argopts. It's been tested on python2.6 and should not cause us problems.

Comment 9

[Rail Aliiev [:rail]]

Comment on attachment 8570595 [details] [diff] [review]
clobberer.client.diff

The interdiff looks good to me

Comment 10

[Morgan Phillips [:mrrrgn]]

Attachment: clobber.runner.diff

Comment 11

[Morgan Phillips [:mrrrgn]]

Attachment: clobber.runner.diff

Tested and working. The new client was checked in very early this morning as well.

Comment 12

[Rail Aliiev [:rail]]

Comment on attachment 8571651 [details] [diff] [review]
clobber.runner.diff

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

::: manifests/moco-config.pp
@@ +236,5 @@
>      $runner_env_hg_share_base_dir = '/builds/hg-shared'
>      $runner_env_git_share_base_dir = '/builds/git-shared'
>  
>      $runner_buildbot_slave_dir = '/builds/slave'
> +    $runner_clobberer_url = 'https://api.pub.build.mozilla.org/clobberer/lastclobber/all'

Looks like we are missing all $runner_* configs in http://hg.mozilla.org/build/puppet/file/3b376bb94123/modules/config/manifests/base.pp

Can you add them, so other consumers of PuppetAgain can use some sane default values? moco-config.pp overrides the values from modules/config/manifests/base.pp.

Otherwise the patch looks good to me.

Comment 13

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8571651 [details] [diff] [review]
clobber.runner.diff

On it, thanks! :)

Comment 14

[Morgan Phillips [:mrrrgn]]

Attachment: clobber.runner.diff