You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2016/10/13 09:39:16 UTC
Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)
On 13 October 2016 at 10:59, <jc...@apache.org> 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
Re: Stop backport.pl running as cronjob? (was Re: svn commit:
r1764631 - /subversion/branches/1.9.x-r1721488/)
Posted by Johan Corveleyn <jc...@gmail.com>.
Great, thanks Daniel!
--
Johan
On Fri, Oct 21, 2016 at 1:45 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Daniel Shahaf wrote on Thu, Oct 13, 2016 at 14:57:16 +0000:
>> [[[
>> 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/)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Thu, Oct 13, 2016 at 14:57:16 +0000:
> [[[
> 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/)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Johan Corveleyn wrote on Thu, Oct 13, 2016 at 13:14:30 +0200:
> On Thu, Oct 13, 2016 at 1:05 PM, Branko \u010cibej <br...@apache.org> wrote:
> > On 13.10.2016 13:01, Branko \u010cibej wrote:
> >> On 13.10.2016 11:39, Ivan Zhakov wrote:
> >>> On 13 October 2016 at 10:59, <jc...@apache.org> 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()
+ backport.merger.merge(entry, commit=True)
+ break # 'continue' the outer loop
+ else:
+ break
]]]
Re: Stop backport.pl running as cronjob? (was Re: svn commit:
r1764631 - /subversion/branches/1.9.x-r1721488/)
Posted by Johan Corveleyn <jc...@gmail.com>.
On Thu, Oct 13, 2016 at 1:05 PM, Branko Čibej <br...@apache.org> 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, <jc...@apache.org> 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/)
Posted by Branko Čibej <br...@apache.org>.
On 13.10.2016 13:01, Branko \u010cibej wrote:
> On 13.10.2016 11:39, Ivan Zhakov wrote:
>> On 13 October 2016 at 10:59, <jc...@apache.org> 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/)
Posted by Branko Čibej <br...@apache.org>.
On 13.10.2016 11:39, Ivan Zhakov wrote:
> On 13 October 2016 at 10:59, <jc...@apache.org> 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