New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----

Blocked on:
issue 407399
issue 768388

Blocking:
issue 707995


Participants' hotlists:
Tiff-List


Sign in to add a comment

Decouple LKGR Storage from Chromium_Status

Project Member Reported by zhangtiff@chromium.org, Aug 29 2017

Issue description

V8-status is currently the only status app that actively uses the LKGR storage that Chromium_Status provides. 

I am working towards migrating Chromium_Status' functionality into Sheriff-o-Matic, so we'll need to come up with a new solution for this LKGR storage. We could reimplement the LKGR storage as a one-off in Sheriff-o-Matic, but since V8 is currently the only user, it probably would make more sense for the LKGR storage to live somewhere that V8 owns. 

If the V8 team has any specific preferences on how LKGR should be stored, I'd be happy to do the implementation work since this is part of the Chromium Status migration. I want to make sure the team's needs are still being supported and that we work towards a solution that keeps any future goals in mind. 
 
Blocking: 707995
Chromium is a user too. It still uses and probably continues to support lkcr. It is right now not stored in the app, but in a way worse way in a file on master1. With LUCI, this will get deprecated and needs a new solution as well.

How about we discontinue storing the lkgr/lkcr/etc in any app and just push to the git ref directly from lkgr finder script? It just needs the right acls. Also the separate lkgr tag pusher scripts would become obsolete. Having the acls on the machine where this runs should not be a problem if it's a slave. V8's lkgr finder already runs on its own slave - and the others could easily migrate, too.
I'm extremely in favor of moving towards removing LKGR/CR storage systems and consolidating the LKGR Tag Pushing function into the main LKGR Finder script. 

That sounds like a good solution to me. I am willing to look into what needs to be done to consolidate these things. Though, if I recall correctly, V8 team has their own version of LKGR Tag Pusher? 
Yes we have... but I'd love to get rid of it too. It should be pretty simple to add pushing to the lkgr_finder script. Or better, just let the script return the new lkgr and let the caller do the pushing. We could much easier implement this in the calling recipe, like:
https://cs.chromium.org/chromium/infra/recipes/recipes/lkgr_finder.py
Another feature the app has and that the ref wouldn't have is the list of lkgrs. But I assume nobody needs that?

In V8, right now we use it to determine an lkgr's age in the master's UI: https://cs.chromium.org/chromium/build/masters/master.client.v8/templates/announce.html?dr&q=lkgr+file:%5Ebuild/masters/master%5C.client%5C.v8/+package:%5Echromium$&l=31

But I think we could just replace that with the commit's age instead.
Cc: -serg...@chromium.org -machenb...@chromium.org
Components: Infra>Client>V8
Blockedon: 768388
Components: Infra>Client>WebRTC
FYI: I rewrote lkgr_finder recipe and it now can push into the ref directly. You can re-use it to move away from pushing to status app for WebRTC as well: https://cs.chromium.org/chromium/infra/recipes/recipes/lkgr_finder.py
Awesome, thanks for doing that, sergiyb! 
Components: -Infra>Sheriffing>SheriffOMatic
Blockedon: 407399
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 21

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/7aec1f9d47040cad97c018f80cff75876a9aa6b8

commit 7aec1f9d47040cad97c018f80cff75876a9aa6b8
Author: Michael Achenbach <machenbach@chromium.org>
Date: Wed Mar 21 08:05:38 2018

[lkgr] Deprecate lkgr status-app post

The only users of lkgr-finder now use the read-from/write-to file option
and afterwards directly update a ref in their git repo.

Hence, we deprecate using the status-app for storing the lkgr. This
unblocks removing corresponding code from the status-app.

Step 7 of:
 https://crbug.com/407399#c31 

Bug:  704080 ,760305
Change-Id: Ibc65265efb632ad76e5b0d619dca6265d8c455b8
Reviewed-on: https://chromium-review.googlesource.com/968422
Reviewed-by: Sergiy Byelozyorov <sergiyb@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>

[modify] https://crrev.com/7aec1f9d47040cad97c018f80cff75876a9aa6b8/infra/services/lkgr_finder/__main__.py
[modify] https://crrev.com/7aec1f9d47040cad97c018f80cff75876a9aa6b8/infra/services/lkgr_finder/lkgr_lib.py

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 21

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/67eebdbb582a429e72634055c9dd39ae3e367b75

commit 67eebdbb582a429e72634055c9dd39ae3e367b75
Author: Michael Achenbach <machenbach@chromium.org>
Date: Wed Mar 21 09:24:29 2018

Revert "[lkgr] Deprecate lkgr status-app post"

This reverts commit 7aec1f9d47040cad97c018f80cff75876a9aa6b8.

Reason for revert: https://crbug.com/824080

Original change's description:
> [lkgr] Deprecate lkgr status-app post
> 
> The only users of lkgr-finder now use the read-from/write-to file option
> and afterwards directly update a ref in their git repo.
> 
> Hence, we deprecate using the status-app for storing the lkgr. This
> unblocks removing corresponding code from the status-app.
> 
> Step 7 of:
>  https://crbug.com/407399#c31 
> 
> Bug:  704080 ,760305
> Change-Id: Ibc65265efb632ad76e5b0d619dca6265d8c455b8
> Reviewed-on: https://chromium-review.googlesource.com/968422
> Reviewed-by: Sergiy Byelozyorov <sergiyb@chromium.org>
> Reviewed-by: Aaron Gable <agable@chromium.org>
> Commit-Queue: Michael Achenbach <machenbach@chromium.org>

TBR=iannucci@chromium.org,agable@chromium.org,machenbach@chromium.org,sergiyb@chromium.org

Change-Id: Iaa58947b75a63ac4d57f7caf7e63a2bf2e2f3df6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  704080 , 760305
Reviewed-on: https://chromium-review.googlesource.com/972382
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>

[modify] https://crrev.com/67eebdbb582a429e72634055c9dd39ae3e367b75/infra/services/lkgr_finder/__main__.py
[modify] https://crrev.com/67eebdbb582a429e72634055c9dd39ae3e367b75/infra/services/lkgr_finder/lkgr_lib.py

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 6

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/47c17bc164bfdfca263c4fec30e50b1cc30c3207

commit 47c17bc164bfdfca263c4fec30e50b1cc30c3207
Author: Michael Achenbach <machenbach@chromium.org>
Date: Fri Apr 06 18:43:41 2018

Reland "[lkgr] Deprecate lkgr status-app post"

This is a reland of 7aec1f9d47040cad97c018f80cff75876a9aa6b8

The status app code is still removed, but if the current lkgr is
not read from a file, it is instead initialized to a dummy value,
which is the status quo.

Original change's description:
> [lkgr] Deprecate lkgr status-app post
>
> The only users of lkgr-finder now use the read-from/write-to file option
> and afterwards directly update a ref in their git repo.
>
> Hence, we deprecate using the status-app for storing the lkgr. This
> unblocks removing corresponding code from the status-app.
>
> Step 7 of:
>  https://crbug.com/407399#c31 
>
> Bug:  704080 ,760305
> Change-Id: Ibc65265efb632ad76e5b0d619dca6265d8c455b8
> Reviewed-on: https://chromium-review.googlesource.com/968422
> Reviewed-by: Sergiy Byelozyorov <sergiyb@chromium.org>
> Reviewed-by: Aaron Gable <agable@chromium.org>
> Commit-Queue: Michael Achenbach <machenbach@chromium.org>

Bug:  704080 , 760305, 824080
Change-Id: Id507bcd82d3c2d3cff00b88764c86c21ab4e1eff
Reviewed-on: https://chromium-review.googlesource.com/998357
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>

[modify] https://crrev.com/47c17bc164bfdfca263c4fec30e50b1cc30c3207/infra/services/lkgr_finder/__main__.py
[modify] https://crrev.com/47c17bc164bfdfca263c4fec30e50b1cc30c3207/infra/services/lkgr_finder/lkgr_lib.py

Sign in to add a comment