Re: Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)
Great, thanks Daniel! -- Johan On Fri, Oct 21, 2016 at 1:45 AM, Daniel Shahaf wrote: > Daniel Shahaf wrote on Thu, Oct 13, 2016 at 14:57:16 +: >> [[[ >> backport.py: Fix a race condition involving concurrent commits to STATUS >> (see r1764633). >> >> The fix is to run 'update' at the top of the outermost loop, rather than >> immediately before calling 'svn merge'. >> >> * tools/dist/backport/merger.py >> (merge): Don't run revert+update; instead, expect the caller to have done >> so. >> >> * tools/dist/merge-approved-backports.py: >> Run 'update' and re-parse STATUS before each merge. >> >> * tools/dist/detect-conflicting-backports.py: >> Track API change of merge(). >> ]]] > > Committed in r1765903.
Re: Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)
Daniel Shahaf wrote on Thu, Oct 13, 2016 at 14:57:16 +: > [[[ > backport.py: Fix a race condition involving concurrent commits to STATUS > (see r1764633). > > The fix is to run 'update' at the top of the outermost loop, rather than > immediately before calling 'svn merge'. > > * tools/dist/backport/merger.py > (merge): Don't run revert+update; instead, expect the caller to have done > so. > > * tools/dist/merge-approved-backports.py: > Run 'update' and re-parse STATUS before each merge. > > * tools/dist/detect-conflicting-backports.py: > Track API change of merge(). > ]]] Committed in r1765903.
Re: Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)
Johan Corveleyn wrote on Thu, Oct 13, 2016 at 13:14:30 +0200: > On Thu, Oct 13, 2016 at 1:05 PM, Branko Čibej wrote: > > On 13.10.2016 13:01, Branko Čibej wrote: > >> On 13.10.2016 11:39, Ivan Zhakov wrote: > >>> On 13 October 2016 at 10:59, wrote: > Author: jcorvel > Date: Thu Oct 13 08:59:07 2016 > New Revision: 1764631 > > URL: http://svn.apache.org/viewvc?rev=1764631&view=rev > Log: > Resurrect the '1.9.x-r1721488' branch, to give backport.pl another > chance to execute the correct backport commands, after backport mess. > > >>> I'm wondering if we really need backport.pl running as cronjob to > >>> merge backports automatically to the stable branches? It's not a first > >>> time when this automatic job performs invalid merges. And as far I > >>> understand we still spend some time to babysit this tool, fix bugs > >>> etc. What is wrong with old proven process by merging revisions > >>> manually? > >> It doesn't happen all that often that backport.pl makes a mistake. I bet > >> manual merging would be just as error-prone. > >> > >> Backporting is a well-defined process. The best possible way to document > >> a process is to automate it. Errors will happen but that's no reason to > >> revert to PEBKAC; bugs can be fixed. > > > > > > In fact, now that I've read Johan's mail ... it /was/ a PEBKAC and > > nothing was wrong with backport.pl. > > Indeed, what we experienced last night was not a script bug (unless if > you state that it should have protected against that race condition > :-)). I think the issue is different. backport.pl was never expected to run concurrently with itself. However, tonight's bug would also have occurred if a human made a commit that met all the following conditions: - Committed between 04:00:02 and (roughly) 04:00:12 UTC, - on a night when there are 2+ approved groups, - removes a group (other than the last) from the "Approved changes" section. That's a pretty rare combination: we have few commits at around 4am UTC, and we have few cases of vetoing an approved group. That's why this bug has never been encountered in 5+ years of using the cron job (and even tonight, it was encountered due to the migration, not due to a nocturnal committer). Patch attached. Cheers, Daniel [[[ backport.py: Fix a race condition involving concurrent commits to STATUS (see r1764633). The fix is to run 'update' at the top of the outermost loop, rather than immediately before calling 'svn merge'. * tools/dist/backport/merger.py (merge): Don't run revert+update; instead, expect the caller to have done so. * tools/dist/merge-approved-backports.py: Run 'update' and re-parse STATUS before each merge. * tools/dist/detect-conflicting-backports.py: Track API change of merge(). ]]] [[[ Index: backport/merger.py === --- backport/merger.py (revision 1762016) +++ backport/merger.py (working copy) @@ -195,11 +195,8 @@ def merge(entry, expected_stderr=None, *, commit=F mergeargs.extend(['--', sf.trunk_url()]) logmsg += entry.raw - # TODO(interactive mode): exclude STATUS from reverts - # TODO(interactive mode): save local mods to disk, as backport.pl does - run_revert() + no_local_mods('.') - run_svn_quiet(['update']) # TODO: use select() to restore interweaving of stdout/stderr _, stdout, stderr = run_svn_quiet(['merge'] + mergeargs, expected_stderr) sys.stdout.write(stdout) Index: detect-conflicting-backports.py === --- detect-conflicting-backports.py (revision 1762016) +++ detect-conflicting-backports.py (working copy) @@ -77,6 +77,7 @@ ERRORS = collections.defaultdict(list) for entry_para in sf.entries_paras(): entry = entry_para.entry() # SVN_ERR_WC_FOUND_CONFLICT = 155015 +backport.merger.run_svn_quiet(['update']) # TODO: what to do if this pulls in a STATUS mod? backport.merger.merge(entry, 'svn: E155015' if entry.depends else None) _, output, _ = backport.merger.run_svn(['status']) Index: merge-approved-backports.py === --- merge-approved-backports.py (revision 1762016) +++ merge-approved-backports.py (working copy) @@ -40,11 +40,14 @@ if sys.argv[1:]: sys.exit(0) backport.merger.no_local_mods('./STATUS') -sf = backport.status.StatusFile(open('./STATUS', encoding="UTF-8")) -# Duplicate sf.paragraphs, since merge() will be removing elements from it. -entries_paras = list(sf.entries_paras()) -for entry_para in entries_paras: - if entry_para.approved(): -entry = entry_para.entry() -backport.merger.merge(entry, commit=True) +while True: +backport.merger.run_svn_quiet(['update']) +sf = backport.status.StatusFile(open('./STATUS', encoding="UTF-8")) +for entry_para in sf.entries_paras(): +if entry_para.approved(): +entry = entry_para.entry(
Re: Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)
On Thu, Oct 13, 2016 at 1:05 PM, Branko Čibej wrote: > On 13.10.2016 13:01, Branko Čibej wrote: >> On 13.10.2016 11:39, Ivan Zhakov wrote: >>> On 13 October 2016 at 10:59, wrote: Author: jcorvel Date: Thu Oct 13 08:59:07 2016 New Revision: 1764631 URL: http://svn.apache.org/viewvc?rev=1764631&view=rev Log: Resurrect the '1.9.x-r1721488' branch, to give backport.pl another chance to execute the correct backport commands, after backport mess. >>> I'm wondering if we really need backport.pl running as cronjob to >>> merge backports automatically to the stable branches? It's not a first >>> time when this automatic job performs invalid merges. And as far I >>> understand we still spend some time to babysit this tool, fix bugs >>> etc. What is wrong with old proven process by merging revisions >>> manually? >> It doesn't happen all that often that backport.pl makes a mistake. I bet >> manual merging would be just as error-prone. >> >> Backporting is a well-defined process. The best possible way to document >> a process is to automate it. Errors will happen but that's no reason to >> revert to PEBKAC; bugs can be fixed. > > > In fact, now that I've read Johan's mail ... it /was/ a PEBKAC and > nothing was wrong with backport.pl. Indeed, what we experienced last night was not a script bug (unless if you state that it should have protected against that race condition :-)). It was the first time the script was run from qavm3, so I was paying extra attention to potential problems. Anyway, it's not like we will migrate to a new machine every month, so it's quite an exceptional situation ... -- Johan
Re: Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)
On 13.10.2016 13:01, Branko Čibej wrote: > On 13.10.2016 11:39, Ivan Zhakov wrote: >> On 13 October 2016 at 10:59, wrote: >>> Author: jcorvel >>> Date: Thu Oct 13 08:59:07 2016 >>> New Revision: 1764631 >>> >>> URL: http://svn.apache.org/viewvc?rev=1764631&view=rev >>> Log: >>> Resurrect the '1.9.x-r1721488' branch, to give backport.pl another >>> chance to execute the correct backport commands, after backport mess. >>> >> I'm wondering if we really need backport.pl running as cronjob to >> merge backports automatically to the stable branches? It's not a first >> time when this automatic job performs invalid merges. And as far I >> understand we still spend some time to babysit this tool, fix bugs >> etc. What is wrong with old proven process by merging revisions >> manually? > It doesn't happen all that often that backport.pl makes a mistake. I bet > manual merging would be just as error-prone. > > Backporting is a well-defined process. The best possible way to document > a process is to automate it. Errors will happen but that's no reason to > revert to PEBKAC; bugs can be fixed. In fact, now that I've read Johan's mail ... it /was/ a PEBKAC and nothing was wrong with backport.pl. -- Brane
Re: Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)
On 13.10.2016 11:39, Ivan Zhakov wrote: > On 13 October 2016 at 10:59, wrote: >> Author: jcorvel >> Date: Thu Oct 13 08:59:07 2016 >> New Revision: 1764631 >> >> URL: http://svn.apache.org/viewvc?rev=1764631&view=rev >> Log: >> Resurrect the '1.9.x-r1721488' branch, to give backport.pl another >> chance to execute the correct backport commands, after backport mess. >> > I'm wondering if we really need backport.pl running as cronjob to > merge backports automatically to the stable branches? It's not a first > time when this automatic job performs invalid merges. And as far I > understand we still spend some time to babysit this tool, fix bugs > etc. What is wrong with old proven process by merging revisions > manually? It doesn't happen all that often that backport.pl makes a mistake. I bet manual merging would be just as error-prone. Backporting is a well-defined process. The best possible way to document a process is to automate it. Errors will happen but that's no reason to revert to PEBKAC; bugs can be fixed. -- Brane
Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)
On 13 October 2016 at 10:59, wrote: > Author: jcorvel > Date: Thu Oct 13 08:59:07 2016 > New Revision: 1764631 > > URL: http://svn.apache.org/viewvc?rev=1764631&view=rev > Log: > Resurrect the '1.9.x-r1721488' branch, to give backport.pl another > chance to execute the correct backport commands, after backport mess. > I'm wondering if we really need backport.pl running as cronjob to merge backports automatically to the stable branches? It's not a first time when this automatic job performs invalid merges. And as far I understand we still spend some time to babysit this tool, fix bugs etc. What is wrong with old proven process by merging revisions manually? -- Ivan Zhakov