Bug 85887 – Add SVN mirror handling feature to build.webkit.org

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

Nowadays there were too many svn.webkit.org server downtime
which caused "rm -rf" on all buildslave and new checkout.

Intercontinental svn checkout takes hours and many parallel checkout
usually overloads the svn.webkit.org server. That's why I propose to
set up svn mirrors for developers and for buildbots too.

Patch is coming soon.

[Csaba Osztrogonác]

Created attachment 140724 [details]
WIP patch

We finished setting up an SVN mirror 2 weeks ago and use it on http://build.webkit.sed.hu (14 buildslaves) with this patch adapted to our master, and it works very stable. The new wait-for-SVN-server.py ensures that svn update never start before mirror is ready and in sync with svn.webkit.org. It protects buildslaves from "rm -f" because of unavailable SVN server or invalid SVN revision passed with force build button. In my patch this script only runs when SVN mirror is added. But it can be useful for svn.webkit.org SVN server too to avoid "rm -rf" because of unavailable server.

[Csaba Osztrogonác]

I forgot to mention that the origin of "SVN server XML call" part 
of Tools/BuildSlaveSupport/wait-for-SVN-server.py is buildbot code:
https://github.com/buildbot/buildbot/blob/buildbot-0.8.2/master/contrib/svnpoller.py

[Peter Beverloo]

Adding dpranke@ and maruel@ for a Chromium opinion, also given this thread which outlines our GIT mirror:
https://lists.webkit.org/pipermail/webkit-dev/2012-March/019698.html

[Dirk Pranke]

Comment on attachment 140724 [details]
WIP patch

I have no useful knowledge to contribute here. Sounds useful, though :)

The code looks fine to me, but you probably want wms or lforschler to review it.

[Lucas Forschler]

Is there a corresponding change to the config.json which will point your slaves to the mirror?

[Eric Seidel (no email)]

Comment on attachment 140724 [details]
WIP patch

Can we unittest this in master_cfg_unittest.py?

[Csaba Osztrogonác]

Created attachment 140874 [details]
Add SVN mirror for Qt buildslaves

(In reply to comment #5)> Is there a corresponding change to the config.json which will point your slaves to the mirror?Sure. After the general patch landed, I'm going to add this change to config.json

[Csaba Osztrogonác]

(In reply to comment #6)
> (From update of attachment 140724 [details])
> Can we unittest this in master_cfg_unittest.py?

There isn't any checked in unittest for CheckOutSource and *Factory classes.
But let's give it a try.

[Csaba Osztrogonác]

Created attachment 149537 [details]
Patch

updated to ToT, I'm going to add unit test too.

[Csaba Osztrogonác]

Created attachment 149753 [details]
proposed patch

I updated it to ToT and addded a unit test. 
( It won't apply until https://bugs.webkit.org/show_bug.cgi?id=90072 landed )

[Peter Gal]

View in context: https://bugs.webkit.org/attachment.cgi?id=149753&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/config.json:182
> +                      "SVNMirror": "svn://rain.inf.u-szeged.hu:3389/",

Why is this different from the rest?

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:104
> +        self.addFactoryArguments(platform=platform, configuration=configuration, architecture=architecture, buildOnly=buildOnly, SVNMirror = SVNMirror)

No need for the spaces around the last equal sign (just like the others).

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:685
> +        self.addStep(WaitForSVNServer, doStepIf = bool(SVNMirror), hideStepIf = bool(SVNMirror))

Won't this hide the step if there is an SVNMirror specified? Is this what you wanted? (Also no need for the spaces around the equal signs)

> Tools/BuildSlaveSupport/wait-for-SVN-server.py:31
> +        response = commands.getoutput("svn log --non-interactive --verbose --xml --limit=1 " + SVNServer)

Use % formatter if you use it already elsewhere, just to be consistent.

> Tools/BuildSlaveSupport/wait-for-SVN-server.py:52
> +    except exceptions.ValueError:

'ValueError' should be enough, no need for the exceptions module.

> Tools/BuildSlaveSupport/wait-for-SVN-server.py:69
> +parser = OptionParser()
> +parser.add_option("-r", "--revision", dest="revision", help="SVN revision number")
> +parser.add_option("-s", "--svn-server", dest="SVNServer", help="SVN server")
> +(options, args) = parser.parse_args()
> +waitForSVNRevision(options.SVNServer, options.revision)

This should be in an if __name__ == "__main__": block.
Also add a type argument for the revision like: type="int", so you won't need to check if the revision is a number or not. The OptionParser will do it for you.

[Csaba Osztrogonác]

(In reply to comment #11)
> View in context: https://bugs.webkit.org/attachment.cgi?id=149753&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/config.json:182
> > +                      "SVNMirror": "svn://rain.inf.u-szeged.hu:3389/",
> 
> Why is this different from the rest?

Because the machine run this bot on is out of our firewall and it can 
see only 3389 TCP port. Which is restricded for this machine only. :)

 
> > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:104
> > +        self.addFactoryArguments(platform=platform, configuration=configuration, architecture=architecture, buildOnly=buildOnly, SVNMirror = SVNMirror)
> 
> No need for the spaces around the last equal sign (just like the others).

Fixed.
 
> > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:685
> > +        self.addStep(WaitForSVNServer, doStepIf = bool(SVNMirror), hideStepIf = bool(SVNMirror))
> 
> Won't this hide the step if there is an SVNMirror specified? Is this what you wanted? (Also no need for the spaces around the equal signs)

Good point. But we shouldn't add this step at all if bool(SVNMirror) is false. 
 
> > Tools/BuildSlaveSupport/wait-for-SVN-server.py:31
> > +        response = commands.getoutput("svn log --non-interactive --verbose --xml --limit=1 " + SVNServer)
> 
> Use % formatter if you use it already elsewhere, just to be consistent.
> 
> > Tools/BuildSlaveSupport/wait-for-SVN-server.py:52
> > +    except exceptions.ValueError:
> 
> 'ValueError' should be enough, no need for the exceptions module.

Removed, because type="int" argument check works for us with empty revision too. (force build without revision)
 
> > Tools/BuildSlaveSupport/wait-for-SVN-server.py:69
> > +parser = OptionParser()
> > +parser.add_option("-r", "--revision", dest="revision", help="SVN revision number")
> > +parser.add_option("-s", "--svn-server", dest="SVNServer", help="SVN server")
> > +(options, args) = parser.parse_args()
> > +waitForSVNRevision(options.SVNServer, options.revision)
> 
> This should be in an if __name__ == "__main__": block.
> Also add a type argument for the revision like: type="int", so you won't need to check if the revision is a number or not. The OptionParser will do it for you.

Fixed.

[Csaba Osztrogonác]

Created attachment 149942 [details]
proposed patch

Updated patch based on Peter's review.

[Csaba Osztrogonác]

Created attachment 158304 [details]
proposed patch

[Csaba Osztrogonác]

Comment on attachment 158304 [details]
proposed patch

Updated to ToT, use subprocess.Popen instead of commands.getoutput to make wait-for-SVN-server.py work on Windows too.

[Csaba Osztrogonác]

Comment on attachment 158304 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158304&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:350
> +        # SVN mirror feature isn't unittestable now with source.oldsource.SVN(==source.SVN) , only with source.svn.SVN(==SVN)
> +        # https://bugs.webkit.org/show_bug.cgi?id=85887
> +        if issubclass(CheckOutSource, source.SVN):
> +            return

Unfortunately it isn't unittestable now with source.oldsource.SVN, because
SVN.baseURL is set to _ComputeRepositoryURL(baseURL) in SVN.__init__
and _ComputeRepositoryURL needs build object to work, so we got the
following error if we tried to run unittest with source.oldsource.SVN:

FAIL: test_CheckOutSource (__main__.SVNMirrorTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./mastercfg_unittest.py", line 358, in test_CheckOutSource
    self.assertEquals(CheckOutSourceInstance.baseURL, self.get_SVNMirrorFromConfig(builder['name']))
AssertionError: <buildbot.steps.source.oldsource._ComputeRepositoryURL object at 0x26aa110> != 'http://svn.webkit.org/repository/webkit/'

You can try this unittest with the newer source.svn.SVN if you apply the following patch:
https://bugs.webkit.org/attachment.cgi?id=158307 (https://bugs.webkit.org/show_bug.cgi?id=90072)
But unfortunately we can't migrate to source.svn.SVN for some reason now.

[Csaba Osztrogonác]

Created attachment 163563 [details]
Patch

[Peter Gal]

View in context: https://bugs.webkit.org/attachment.cgi?id=163563&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:710
> +        if bool(SVNMirror):

No need for the bool() call.

[Csaba Osztrogonác]

(In reply to comment #17)
> Created an attachment (id=163563) [details]
> Patch

Updated to ToT.

When the connection is going broken during svn update, buildbot does "rm -rf"
and then a new checkout. Transatlantic checkout is _impossible_, because it
would take half a day, but it is absolutely _impossible_ without connection
lost. In this case now I have to stop the slave, checkout from our local mirror,
svn switch to svn.webkit.org and restart the slave. I got fed up with it. I
won't do this sisyphus work anymore.

Please review this patch as soon as possible. We really need it.

[Csaba Osztrogonác]

Created attachment 163574 [details]
Patch

[Dirk Pranke]

Comment on attachment 163574 [details]
Patch

looks plausible. good luck.

[Csaba Osztrogonác]

(In reply to comment #21)
> (From update of attachment 163574 [details])
> looks plausible. good luck.

Thanks, I'm going to land it in an IDLE time, maybe 
tomorrow morning in CET timezone (night in PDT)

[Csaba Osztrogonác]

Comment on attachment 163574 [details]
Patch

Clearing flags on attachment: 163574

Committed r128399: <http://trac.webkit.org/changeset/128399>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

It seems automatic master restart didn't work.
Bill or Lucas, could you restart the master, please?

[William Siegrist]

Master has been restarted. There was a stale lockfile from the last time I was working on the restart process.

[Lauro Moura Maranhao Neto]

Created attachment 163934 [details]
Patch

The script was added to a different folder than the one specified in master.cfg. I'm moving it to BuildSlaveSupport/ to keep together with the other scripts and avoid changing master.cfg.

[WebKit Review Bot]

Comment on attachment 163934 [details]
Patch

Rejecting attachment 163934 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:

which does not exist!  Applying it anyway.
patching file Tools/BuildSlaveSupport/build.webkit.org-config/wait-for-SVN-server.py
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file Tools/BuildSlaveSupport/build.webkit.org-config/wait-for-SVN-server.py.rej
patching file Tools/BuildSlaveSupport/wait-for-SVN-server.py

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Dirk Pranke']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13835704

[Lauro Moura Maranhao Neto]

It seems Ossy already fixed it :)

http://trac.webkit.org/changeset/128493

[Csaba Osztrogonác]

Thanks for everyone.

After master restarting I noticed some annoying bug:
- first run is broken, because master.cfg wanted to run wait-for-SVN-sever.py
before svn update ... at that time wait-for-SVN-sever.py doesn't exist. :-/
  - problem for a new slave 
  - problem after "rm -rf" after an svn fail or mirror changes in config.json
  - possible solution: check the existance of wait-for-SVN-sever.py from master.cfg
    - true: run it
    - false: sleep for X seconds to let SVN mirror have time for sync
- popen in wait-for-SVN-sever.py is buggy on windows
- type="int" check for revision in wait-for-SVN-sever.py is incorrect, because it can be empty string if force build button is pushed without adding revision.

I leave this bug open now and will file bug reports about these 
bugs tomorrow morning. Of course, I'll fix them as soon as possible.

[Csaba Osztrogonác]

(In reply to comment #30)
> Thanks for everyone.
> 
> After master restarting I noticed some annoying bug:
> - first run is broken, because master.cfg wanted to run wait-for-SVN-sever.py
> before svn update ... at that time wait-for-SVN-sever.py doesn't exist. :-/
>   - problem for a new slave 
>   - problem after "rm -rf" after an svn fail or mirror changes in config.json
>   - possible solution: check the existance of wait-for-SVN-sever.py from master.cfg
>     - true: run it
>     - false: sleep for X seconds to let SVN mirror have time for sync

new bug report: https://bugs.webkit.org/show_bug.cgi?id=98013

> - popen in wait-for-SVN-sever.py is buggy on windows

false positive alarm :) We had a sophisticated svn.bat SVN wrapper 
script on our Windows bot, and popen couldn't find the SVN binary.

> - type="int" check for revision in wait-for-SVN-sever.py is incorrect, because it can be empty string if force build button is pushed without adding revision.

new bug report: https://bugs.webkit.org/show_bug.cgi?id=98014

I close this bug, because it is solved and it works fine. The 
mentioned minor bugs will be fixed in separated bug reports.

[Aakash Jain]

After removing Qt bots in https://trac.webkit.org/changeset/156771/webkit , SVNMirror code is completely unused. It seems to be unused for last 5 years. 

Should we just remove this dead code?

[Aakash Jain]

(In reply to Aakash Jain from comment #32)
> After removing Qt bots in https://trac.webkit.org/changeset/156771/webkit ,
> SVNMirror code is completely unused. It seems to be unused for last 5 years. 
> 
> Should we just remove this dead code?
Deleting this dead code in https://bugs.webkit.org/show_bug.cgi?id=217823