New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 805439 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-04-20
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 829734
issue 837858



Sign in to add a comment

6.5-lkgr is invalid

Project Member Reported by habl...@google.com, Jan 24 2018

Issue description

See https://chromium.googlesource.com/v8/v8/+log/6.5-lkgr.

The ref should never point to a commit that is not created by our infrastructure (like https://chromium.googlesource.com/v8/v8/+/7199784766ca3b3926eefc39ab7d748e954acc24).

If you look at the head of 6.5 (https://chromium.googlesource.com/v8/v8/+log/branch-heads/6.5) you can see that the branch has diverged already).

It seems the last commit to 6.5-lkgr (https://chromium.googlesource.com/v8/v8/+/68d7b7250ffae68c25fbd27b80990444e9dcd8a5) did create a branch and made everything broken. This merge was incorrect and done somehow manually and than pushed through.

I tried to delete the remote branch 6.5-lkgr, but that does not work, because the ref exists. I can't roll back the branch either. I also can't change the ref because there is already a branch.

Would be great if somebody could fix this please as currently Chrome 65 is built from some version and will not receive any merges to 6.5.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 24 2018

Labels: merge-merged-6.5
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/8fa2c8d2189b7847ce46b05531b52d7091487b96

commit 8fa2c8d2189b7847ce46b05531b52d7091487b96
Author: Michael Hablich <hablich@chromium.org>
Date: Wed Jan 24 15:37:48 2018

Merged: [memory] Change OS::Allocate on Windows to not over allocate.

Revision: a9aeaa65e654a1590695f82dc80c1ce9e4c02ef4

BUG=chromium:800511,805439
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=machenbach@chromium.org

Change-Id: Ic048861e0ce04dea0ee7ec2ee71112c29ff11506
Reviewed-on: https://chromium-review.googlesource.com/883803
Reviewed-by: Michael Hablich <hablich@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.5@{#7}
Cr-Branched-From: 73c55f57fe8506011ff854b15026ca765b669700-refs/heads/6.5.254@{#1}
Cr-Branched-From: 594a1a0b6e551397cfdf50870f6230da34db2dc8-refs/heads/master@{#50664}
[modify] https://crrev.com/8fa2c8d2189b7847ce46b05531b52d7091487b96/src/base/platform/platform-cygwin.cc
[modify] https://crrev.com/8fa2c8d2189b7847ce46b05531b52d7091487b96/src/base/platform/platform-win32.cc

C#1 merges the fix unto 6.5 as it is needed. That 'repairs' 6.5. 

6.5-lkgr still broken, I will leave that to our infra people who understand our Git repo better.
Owner: serg...@chromium.org
Status: Started (was: Untriaged)
The correct Git command to delete a reference is

  git push origin --delete refs/heads/6.5-lkgr

However, it appears no-one has the right to do so in this repository. I am going to grant this right to v8-admins in https://crrev.com/c/883526, execute the command and then revert the change back again since it's too dangerous: by typing one wrong command one can delete refs/heads/master.
Cc: -serg...@chromium.org
Status: Fixed (was: Started)
Deleted the refs/heads/6.5-lkgr ref and removed ability to delete refs from v8-admin again https://crrev.com/c/883466.
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/9b5626b446b70a3ade7b97c30083995783c988bc

commit 9b5626b446b70a3ade7b97c30083995783c988bc
Author: Michael Hablich <hablich@chromium.org>
Date: Wed Jan 24 16:36:06 2018

Whitespace commit to test infra

Bug: chromium:805439
Change-Id: I611a33d6b501d59b02a2766563a8d8241f3b68bb
Reviewed-on: https://chromium-review.googlesource.com/883532
Reviewed-by: Michael Hablich <hablich@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.5@{#9}
Cr-Branched-From: 73c55f57fe8506011ff854b15026ca765b669700-refs/heads/6.5.254@{#1}
Cr-Branched-From: 594a1a0b6e551397cfdf50870f6230da34db2dc8-refs/heads/master@{#50664}
[modify] https://crrev.com/9b5626b446b70a3ade7b97c30083995783c988bc/tools/whitespace.txt

How can we prevent that this is going to happen again?
Labels: -Pri-0 Pri-2
Owner: machenb...@chromium.org
Status: Assigned (was: Fixed)
Currently all committers can push to refs/heads/*. We could forbid this and e.g. only allow refs/heads/master and refs/branch-heads/* explicitly. For e.g. repairing rolls and other refs we can allow a small group of people to have the same rights as the auto-roller.
Suggested by Sergiy, we could use refs/heads/lkgr/* for the lkgr refs and explicitly permit v8-autoroll only.

We could do this in parallel to the existing refs for a transition period.
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e

commit 1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e
Author: Michael Achenbach <machenbach@chromium.org>
Date: Tue Jan 30 09:36:26 2018

V8: Add new lkgr ref to V8 tag pusher

This makes the v8 tag pusher also update:
refs/heads/lkgr/<name> additionally to
refs/heads/<name>-lkgr

We'll use the former to refine the permissions to it. The latter,
legacy ref will be deprecated from next release cycle on.

Bug: chromium:805439
Change-Id: I41714d1ecc84f137e35a5544ac775fd5ab043c86
Reviewed-on: https://chromium-review.googlesource.com/890259
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Reviewed-by: Sergiy Byelozyorov <sergiyb@chromium.org>

[modify] https://crrev.com/1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e/scripts/slave/recipes/v8/auto_tag.py
[modify] https://crrev.com/1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e/scripts/slave/recipes/v8/auto_tag.expected/update.json
[modify] https://crrev.com/1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e/scripts/slave/recipes/v8/auto_tag.expected/missing.json
[modify] https://crrev.com/1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e/scripts/slave/README.recipes.md
[modify] https://crrev.com/1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e/scripts/slave/recipes/v8/auto_tag.expected/dry_run.json
[modify] https://crrev.com/1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e/scripts/slave/recipes/v8/auto_tag.expected/update_timeout.json
[modify] https://crrev.com/1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e/scripts/slave/recipes/v8/auto_tag.expected/same_lkgr.json

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/ad952aad3d6ff5dc3028bd3631134f553f8e3c8e

commit ad952aad3d6ff5dc3028bd3631134f553f8e3c8e
Author: Michael Achenbach <machenbach@chromium.org>
Date: Tue Jan 30 10:01:47 2018

Revert "V8: Add new lkgr ref to V8 tag pusher"

This reverts commit 1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e.

Reason for revert: Breaks 6.5  branch.

Original change's description:
> V8: Add new lkgr ref to V8 tag pusher
> 
> This makes the v8 tag pusher also update:
> refs/heads/lkgr/<name> additionally to
> refs/heads/<name>-lkgr
> 
> We'll use the former to refine the permissions to it. The latter,
> legacy ref will be deprecated from next release cycle on.
> 
> Bug: chromium:805439
> Change-Id: I41714d1ecc84f137e35a5544ac775fd5ab043c86
> Reviewed-on: https://chromium-review.googlesource.com/890259
> Commit-Queue: Michael Achenbach <machenbach@chromium.org>
> Reviewed-by: Michael Achenbach <machenbach@chromium.org>
> Reviewed-by: Sergiy Byelozyorov <sergiyb@chromium.org>

TBR=machenbach@chromium.org,hablich@chromium.org,sergiyb@chromium.org

Change-Id: I4a8be64c5ea689a4aab94b07fe9c9ce82c3fc5c6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:805439
Reviewed-on: https://chromium-review.googlesource.com/892979
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>

[modify] https://crrev.com/ad952aad3d6ff5dc3028bd3631134f553f8e3c8e/scripts/slave/recipes/v8/auto_tag.py
[modify] https://crrev.com/ad952aad3d6ff5dc3028bd3631134f553f8e3c8e/scripts/slave/recipes/v8/auto_tag.expected/update.json
[modify] https://crrev.com/ad952aad3d6ff5dc3028bd3631134f553f8e3c8e/scripts/slave/recipes/v8/auto_tag.expected/missing.json
[modify] https://crrev.com/ad952aad3d6ff5dc3028bd3631134f553f8e3c8e/scripts/slave/README.recipes.md
[modify] https://crrev.com/ad952aad3d6ff5dc3028bd3631134f553f8e3c8e/scripts/slave/recipes/v8/auto_tag.expected/dry_run.json
[modify] https://crrev.com/ad952aad3d6ff5dc3028bd3631134f553f8e3c8e/scripts/slave/recipes/v8/auto_tag.expected/update_timeout.json
[modify] https://crrev.com/ad952aad3d6ff5dc3028bd3631134f553f8e3c8e/scripts/slave/recipes/v8/auto_tag.expected/same_lkgr.json

Failure log:

Tag pusher pushed:
https://build.chromium.org/p/client.v8.branches/builders/Auto-tag/builds/1509

Afterwards all syncing failed. E.g.:
https://logs.chromium.org/v/?s=chromium%2Fbb%2Fclient.v8.branches%2FAuto-tag%2F1510%2F%2B%2Frecipes%2Fsteps%2Fgclient_sync%2F0%2Fstdout

[0:00:56] error: cannot lock ref 'refs/heads/lkgr/6.5': 'refs/heads/lkgr' exists; cannot create 'refs/heads/lkgr/6.5'
[0:00:56]  ! [new branch]            lkgr/6.5                    -> lkgr/6.5  (unable to update local ref)

Fixed by reverting tag-pusher CL, by adding machenbach temporarily to v8-autoroll and by granting push access to refs/heads/lkgr/6.5.
Then locally:
git push origin :refs/heads/lkgr/6.5
We need to name our ref folder something different from "lkgr", since we already have a ref called refs/heads/lkgr. It doesn't matter what we name it. It'd also be possible to move it to tags, e.g.:
refs/tags/lkgr/<branch>
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/afcfbfcf3656e38b223021168002c6e11bc1ee52

commit afcfbfcf3656e38b223021168002c6e11bc1ee52
Author: Michael Achenbach <machenbach@chromium.org>
Date: Thu Mar 01 10:23:34 2018

Reland "V8: Add new lkgr ref to V8 tag pusher"

This is a reland of 1fcc4e2f8f78a5a4bbb97fb16b52ea12e618df3e.

Now we use refs/tags/lkgr/* to not collide with
refs/heads/lkgr.

Original change's description:
> V8: Add new lkgr ref to V8 tag pusher
>
> This makes the v8 tag pusher also update:
> refs/heads/lkgr/<name> additionally to
> refs/heads/<name>-lkgr
>
> We'll use the former to refine the permissions to it. The latter,
> legacy ref will be deprecated from next release cycle on.
>
> Bug: chromium:805439
> Change-Id: I41714d1ecc84f137e35a5544ac775fd5ab043c86
> Reviewed-on: https://chromium-review.googlesource.com/890259
> Commit-Queue: Michael Achenbach <machenbach@chromium.org>
> Reviewed-by: Michael Achenbach <machenbach@chromium.org>
> Reviewed-by: Sergiy Byelozyorov <sergiyb@chromium.org>

TBR=sergiyb@chromium.org

Bug: chromium:805439
Change-Id: Ia4c5c35b0239e070b0f975875c358e2795ea00c9
Reviewed-on: https://chromium-review.googlesource.com/941041
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>

[modify] https://crrev.com/afcfbfcf3656e38b223021168002c6e11bc1ee52/scripts/slave/recipes/v8/auto_tag.py
[modify] https://crrev.com/afcfbfcf3656e38b223021168002c6e11bc1ee52/scripts/slave/recipes/v8/auto_tag.expected/update.json
[modify] https://crrev.com/afcfbfcf3656e38b223021168002c6e11bc1ee52/scripts/slave/recipes/v8/auto_tag.expected/missing.json
[modify] https://crrev.com/afcfbfcf3656e38b223021168002c6e11bc1ee52/scripts/slave/README.recipes.md
[modify] https://crrev.com/afcfbfcf3656e38b223021168002c6e11bc1ee52/scripts/slave/recipes/v8/auto_tag.expected/dry_run.json
[modify] https://crrev.com/afcfbfcf3656e38b223021168002c6e11bc1ee52/scripts/slave/recipes/v8/auto_tag.expected/update_timeout.json
[modify] https://crrev.com/afcfbfcf3656e38b223021168002c6e11bc1ee52/scripts/slave/recipes/v8/auto_tag.expected/same_lkgr.json

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/1f219f39077bbb46973b3c122b3989943f78642f

commit 1f219f39077bbb46973b3c122b3989943f78642f
Author: Michael Achenbach <machenbach@chromium.org>
Date: Thu Mar 01 11:11:51 2018

Whitespace change to test new auto-tag bot

Bug: chromium:805439
Change-Id: I8946b878162541906a7592a3a0b6ad7be864b2df
Reviewed-on: https://chromium-review.googlesource.com/942841
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.5@{#57}
Cr-Branched-From: 73c55f57fe8506011ff854b15026ca765b669700-refs/heads/6.5.254@{#1}
Cr-Branched-From: 594a1a0b6e551397cfdf50870f6230da34db2dc8-refs/heads/master@{#50664}
[modify] https://crrev.com/1f219f39077bbb46973b3c122b3989943f78642f/tools/whitespace.txt

Looks like it worked this time. The new ref schema for chrome will be:
refs/tags/lkgr/<branch-number>
where <branch-number> is e.g. 6.6

For now the old and the new schema will co-exist. We could either wait for 2 cycles, or manually update the buildspecs in M65 and M64.

Once all current chrome versions point to the new ref schema, I'd remove the old from the auto-tag bot and disallow editing it further in the ACLs.
NextAction: 2018-04-20
Blockedon: 829734
The NextAction date has arrived: 2018-04-20
Blockedon: 837858

Comment 20 by mmoss@chromium.org, Apr 28 2018

What's wrong with the proposal in #7? Instead of giving everybody access to push all refs by default, and restricting the few special ones, why not allow people to only push a few common refs, and then restrict everything else (including the lkgr ones)? That type of "least privilege" configuration is generally preferable anyhow.
We'd still like to give a larger group of people access to repair our rolls (committers would be most simple). And rolls have a different branch each time. Sadly we can't cover with gerrit a pattern like refs/heads/*.*, therefore we need to give access to refs/heads/* and override access for specific refs, like refs/heads/master. Again we can't override with a pattern like refs/heads/*.*-lkgr. The only way to make these restrictions hierarchical is to use a folder like:
refs/heads/foobar/*.

Since refs/tags/lkgr/* led to problems, and refs/heads/lkgr/* can't be used because of existing refs/heads/lkgr, I'd suggest to simply use a fresh, unique ref in heads like:
refs/heads/release/*

It'd be a rolling ref that points to the same thing as refs/heads/*-lkgr does now. I'll add the changes for this and we'll try with another 2-release cycle grace period to see if something comes up.

Sign in to add a comment