New issue
Advanced search Search tips

Issue 852823 link

Starred by 4 users

Issue metadata

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



Sign in to add a comment

consider using "topic" to chain CLs like CQ-DEPEND when submitting

Project Member Reported by vapier@chromium.org, Jun 14 2018

Issue description

Android uses Gerrit's topic feature to set up relationships across CLs & different git repos.  this way they can submit all CLs simultaneously in a single topic w/out having to annotate the commit message with something like CQ-DEPEND.

Gerrit has to be configured with this behavior:
https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#change.submitWholeTopic

i think that's a Gerrit-instance-wide setting, so we'd need to coordinate all Chromium users.

after that, all Submit operations (either through the API or the web interface) would use the topic.
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#submit-change
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#submitted-together

i don't think this would be too much of a disruption for existing users as topics aren't heavily used, and would help harmonize Gerrit flows between these large projects.
 

Comment 1 by la...@chromium.org, Jun 14 2018

For us this would be a feature of CQ - not Gerrit - right?

Comment 2 by vapier@chromium.org, Jun 26 2018

it's a feature in Gerrit that we'd turn on.  then users, instead of writing manual CQ-DEPEND lines, would set the topic field in their CLs.  our CQ bots would need updating to chain this metadata together like it does with CQ-DEPEND today for testing, and then the submit code paths would need a bit of tweaking to be aware of topic submissions (so it wouldn't have to walk all the CLs by hand).
Cc: tandrii@chromium.org
Owner: tandrii@chromium.org
Status: Assigned (was: Available)
This would need to take a fair amount of thinking before we'd want to enable it for Chromium (src.git and its DEPS), because while this feature might be a relatively desirable thing, it's not immediately obvious to me what this implications would be on a Chromium dev's workflow.

I think -- after thinking about it for ~30 seconds -- that as long as the chromium CQ had done a dryrun of the newest (leaf) CL in the topic, it would probably be safe to commit everything in the CL at once, because I think that's essentially the same configuration that the CQ would've applied and tested. 

@tandrii - does that sound right?
Cc: estaab@chromium.org iannucci@chromium.org
I don't have time to give extended reply, which this deserves (and I also want topics), but basic answer is that we must not yet enable submitWholeTopic host-wide. Given Pri3, i guess CrOS doesn't badly want it, so we have time to discuss this.

Dirk: note that there is diff between CLs stack and topic:
 * CLs in a stack target the same ref&repo. Submitting the whole CL stack is already possible today!
 * Topics can span multiple refs and repos (hence global config).

CLs stack are already supported by Chromium CQ (CQ used by chromium/src, v8/v8 and other chromium-like repos). Submitting CLs stack in CQ is disallowed by default, but can be enabled with cq.cfg and is actively used by a few projects, but this has bisectability implications for chromium/src with today's submit strategy.
Cc: iannu...@google.com
Cc: -iannucci@chromium.org
Cc: vapier@chromium.org
hmm, missed the updates on the bug

i don't think the Chromium side needs to fully support this as a blocker to enabling it.  the Chromium CQ could enforce "don't use topics on Chromium CLs" by refusing to merge any that have the topic field set.  that'd train users pretty quickly into not setting the field until the Chromium CQ is ready.

that would also allow CrOS to move forward with the feature independently.
Teaching browser(chromium) CQ to avoid topics is necessary, but not sufficient.

A submit action can be started on any CL of the topic, potentially on a project without CQ.
This, each repo which doesn't want topics will need to disallow submits by committers, effectivelyandating use of CQ. This is probably a good thing, but it requires communication, user education, and finally changing actual ACLs.
A weaker, but potentially sufficient mitigation might be disabling submit action in UI in chromium Gerrit plugin if topic is set, unless repo opts in for use of topics. It's easier to do, requires no ACL changes or user education.
for repos not using a CQ, it really sounds like a PSA is sufficient to call it a day.  people will figure it out soon enough.
Labels: -Pri-3 Pri-2
wrt priority, from CrOS's side, it's not super important because we already have this CQ-DEPEND framework that mostly works well enough most of the time.  it might be better as a Pri-2 as it'd make our CI more reliable when it comes to submitting CLs.
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/b14af659553ae9b2ccc5befcce39eea62a71a7a0

commit b14af659553ae9b2ccc5befcce39eea62a71a7a0
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Oct 19 22:07:51 2018

cbuildbot: reject CLs using topics for now

Since Gerrit has specific meanings for the Topic field (which we have
disabled currently), reject CLs trying to use it until it works.  They
can use hashtags to organize/group their own CLs in the meantime.

BUG=chromium:852823
TEST=precq passes

Change-Id: I9cd32081e898fd3d1ab9f2ee17e6c6a8d60d64f7
Reviewed-on: https://chromium-review.googlesource.com/1290649
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Lann Martin <lannm@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/b14af659553ae9b2ccc5befcce39eea62a71a7a0/cbuildbot/validation_pool.py
[modify] https://crrev.com/b14af659553ae9b2ccc5befcce39eea62a71a7a0/lib/patch_unittest.py
[modify] https://crrev.com/b14af659553ae9b2ccc5befcce39eea62a71a7a0/lib/patch.py

I've seen CQ-Depend used before as a signal when bisecting the kernel (to identify which range of CLs to skip, since they're likely to fail by themselves), it would be much harder to use the topic (since they're not visible in the git log) to make such decisions.

While this topics feature is kind of neat, I don't think we should completely replace CQ-Depend with it (unless it adds similar tags in the commit message).
people can still use CQ-DEPEND in their commit messages if they want docs.  but to be clear, CQ-DEPEND is an artificial construct with zero guarantees of atomicity or transaction behavior ... that means if you have a lot of CL's in a CQ-DEPEND loop and Gerrit is being slow to submit, it could take minutes before they're all merged, assuming Gerrit doesn't reject/flake some of them in the middle and leave the repos in an inconsistent state (we've seen both problems in the past).

Gerrit topics on the other hand guarantee that, across multiple repos in a single GoB instance, CLs with the same topic will be simultaneously submitted altogether or none will be submitted at all.  which is what we need.

so, short of any other technical solution, we need & will recommend people use topics when possible.
Mike, I have been using "topic" just to make it easier to find a collection of CLs that all fix the same bug or are needed to implement a feature. e.g.:
https://chromium-review.googlesource.com/q/topic:%22atlas_1Mhz_I2C%22+(status:open%20OR%20status:merged)

Right now I can't even run a trybot on those due to this recent change. Can you provide documentation or an example of how to properly use hashtags for the same purpose?

Can I use hashtags with gerrit command line?
e.g. what is the replacement for this:
   gerrit search 'is:open branch:chromeos-4.4 topic:atlas_1Mhz_I2C'

I have a list of CLs to operate on and will clear the topic for those. But I won't be able to build another list with gerrit command until I understand the alternatives. TIA!
the `gerrit --help` output links to the Gerrit search syntax:
  https://gerrit-review.googlesource.com/Documentation/user-search.html

which documents the hashtag query syntax:
  https://gerrit-review.googlesource.com/Documentation/user-search.html#hashtag

`gerrit --help` also shows the "sethashtags" helper command if you want to modify the set of hashtags on CLs.  there's also a "topic" helper command.  so with those two, you should be able to look up all the CLs using a specific topic, then set the hashtags on those CLs to whatever you want, and then clear the topic on all the CLs.
How do I make "repo upload" (I believe it's the typical way for uploading Chrome OS CLs) use hashtags? It has a convenient -t switch which uses current branch name as the topic, but it doesn't seem to have anything similar for hashtags.
the latest repo release does not offer a short option for setting hashtags
hashtags also dont seem to work from the GUI. that is bad because now one needs to patch in the CL locally and reupload to set the hashtag? 

Clarification on #c20: it does work for self-uploaded CLs, not for partner's CLs that you are reviewing. I was trying to add a hashtag to https://chromium-review.googlesource.com/c/chromiumos/third_party/linux-firmware/+/1314389/ and failed. 

if you have issues using the hashtag field, please file a new bug, or start a new e-mail thread.  i don't think we need to get into hashtag usability issues here ... this is just about using topics like they're (currently) intended to be used in Gerrit.
Cc: athilenius@chromium.org
Labels: -Pri-2 Pri-1
Let me circle back here: given the desire to share parts of CQ between future CrOS CQ and existing browser CQ, we can raise priority here.

If we can drop CQ-depend completely, sharing CQ parts will be easier.
At the same time, if we teach browser CQ to recognize topics, we can reject CLs with topics for chromium/src and other repos by default. Then PSA will indeed suffice before enabling submit-by-topic on Chromium host.

Btw, I dunno why bug was assigned to me, but I guess now I have good reason to own this.
I believe that we should pursue topics but CQ-DEPEND is often used for cros-host dependencies so we won't be able to get rid of it.
right, CQ-DEPEND is still necessary when commits span chrome-internal & chromium (manifest & ebuild changes come to mind), but i think that's far less common.
We will need to add CQ-DEPEND support to the LUCI CQ Daemon. Sounds like it will forever remain a hack though (non-transactional/atomic submits). Any way to make it less hacky while we are at this?

We should also consider either disabling CQ-DEPEND for CLs in the same project, or translating those into Topics. Might get tricky if later a cross-project depends is added though.
i'm fine with encouraging people to stop using CQ-DEPEND for same project/GoB host, but i don't think we need to be proactive about it on the coding side if it's a pain to cover

wrt atomic/transactions, that's how it's always been and i think people get it, so that's OK

Sign in to add a comment