Bug 85887

Summary: Add SVN mirror handling feature to build.webkit.org
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Csaba Osztrogonác <ossy>
Status: RESOLVED FIXED    
Severity: Blocker CC: aakash_jain, ap, dpranke, eric, galpeter, lauro.neto, lforschler, maruel, ossy, peter, tony, webkit.review.bot, wsiegrist, zherczeg
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=122210
https://bugs.webkit.org/show_bug.cgi?id=217823
Bug Depends on:    
Bug Blocks: 90072, 98013, 98014    
Attachments:
Description Flags
WIP patch
none
Add SVN mirror for Qt buildslaves
none
Patch
none
proposed patch
none
proposed patch
none
proposed patch
none
Patch
none
Patch
none
Patch none

Description Csaba Osztrogonác 2012-05-08 08:42:52 PDT
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.
Comment 1 Csaba Osztrogonác 2012-05-08 08:56:14 PDT
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.
Comment 2 Csaba Osztrogonác 2012-05-08 09:22:41 PDT
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
Comment 3 Peter Beverloo 2012-05-08 11:22:40 PDT
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
Comment 4 Dirk Pranke 2012-05-08 12:35:55 PDT
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.
Comment 5 Lucas Forschler 2012-05-08 13:17:51 PDT
Is there a corresponding change to the config.json which will point your slaves to the mirror?
Comment 6 Eric Seidel (no email) 2012-05-08 15:37:34 PDT
Comment on attachment 140724 [details]
WIP patch

Can we unittest this in master_cfg_unittest.py?
Comment 7 Csaba Osztrogonác 2012-05-09 00:38:39 PDT
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
Comment 8 Csaba Osztrogonác 2012-05-09 00:44:48 PDT
(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.
Comment 9 Csaba Osztrogonác 2012-06-26 08:42:30 PDT
Created attachment 149537 [details]
Patch

updated to ToT, I'm going to add unit test too.
Comment 10 Csaba Osztrogonác 2012-06-27 08:35:09 PDT
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 )
Comment 11 Peter Gal 2012-06-28 01:24:01 PDT
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.
Comment 12 Csaba Osztrogonác 2012-06-28 06:50:21 PDT
(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.
Comment 13 Csaba Osztrogonác 2012-06-28 06:52:41 PDT
Created attachment 149942 [details]
proposed patch

Updated patch based on Peter's review.
Comment 14 Csaba Osztrogonác 2012-08-14 05:47:12 PDT
Created attachment 158304 [details]
proposed patch
Comment 15 Csaba Osztrogonác 2012-08-14 05:47:41 PDT
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.
Comment 16 Csaba Osztrogonác 2012-08-14 05:58:19 PDT
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.
Comment 17 Csaba Osztrogonác 2012-09-12 02:39:30 PDT
Created attachment 163563 [details]
Patch
Comment 18 Peter Gal 2012-09-12 02:47:46 PDT
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.
Comment 19 Csaba Osztrogonác 2012-09-12 02:48:39 PDT
(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.
Comment 20 Csaba Osztrogonác 2012-09-12 03:47:14 PDT
Created attachment 163574 [details]
Patch
Comment 21 Dirk Pranke 2012-09-12 09:19:54 PDT
Comment on attachment 163574 [details]
Patch

looks plausible. good luck.
Comment 22 Csaba Osztrogonác 2012-09-12 09:29:18 PDT
(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)
Comment 23 Csaba Osztrogonác 2012-09-12 21:18:20 PDT
Comment on attachment 163574 [details]
Patch

Clearing flags on attachment: 163574

Committed r128399: <http://trac.webkit.org/changeset/128399>
Comment 24 Csaba Osztrogonác 2012-09-12 21:18:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Csaba Osztrogonác 2012-09-12 21:39:39 PDT
It seems automatic master restart didn't work.
Bill or Lucas, could you restart the master, please?
Comment 26 William Siegrist 2012-09-13 10:13:45 PDT
Master has been restarted. There was a stale lockfile from the last time I was working on the restart process.
Comment 27 Lauro Moura Maranhao Neto 2012-09-13 12:20:56 PDT
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.
Comment 28 WebKit Review Bot 2012-09-13 12:56:42 PDT
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
Comment 29 Lauro Moura Maranhao Neto 2012-09-13 13:17:51 PDT
It seems Ossy already fixed it :)

http://trac.webkit.org/changeset/128493
Comment 30 Csaba Osztrogonác 2012-09-13 14:51:24 PDT
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.
Comment 31 Csaba Osztrogonác 2012-10-01 04:28:19 PDT
(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.
Comment 32 Aakash Jain 2018-06-22 11:11:04 PDT
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?
Comment 33 Aakash Jain 2020-10-16 06:54:38 PDT
(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