New issue
Advanced search Search tips

Issue 793073 link

Starred by 1 user
Status: Assigned
Owner:
jclinton@chromium.org
Cc:
akes...@chromium.org
ejcaruso@chromium.org
jclinton@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

CQ should not use Gerrit merge conflict calculation to filter CLs from eligible set

Project Member Reported by ejcaruso@chromium.org, Dec 7 2017 
As seen in this CL: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/769298

"The Commit Queue failed to apply your change in https://luci-milo.appspot.com/buildbot/chromeos/master-paladin/16920 . CL:769298 depends on CL:769299, which was not eligible (wrong manifest branch, wrong labels, or otherwise filtered from eligible set)."

CL:769299 is marked as a merge conflict by Gerrit because it conflicts with ToT, but is based on CL:769298 and applies cleanly when that CL is applied first. However, since this merge conflict marker filters it from the eligible set, the CQ refuses to test the change. The merge conflict marker can be resolved by switching the submit type but if we're not doing that then we should just remove the check on the Gerrit merge conflict status and let the CQ handle it correctly by itself.

Comment 1 by ejcaruso@chromium.org, Dec 18 2017 

Ok, update on this: the complicated set of dependencies was really holding this thing together, and removing the dependencies so that we don't depend on merge conflicting CLs actually breaks compilation in the intermediate states. For the 3.14/3.18/4.4 kernels, if the device_jail CL goes in first, it breaks after the revert because the usb device state struct no longer has the member it uses. If it goes in afterwards, then it also breaks after the revert since the device_jail implementation is still using the USBDEVFS_DROP_PRIVILEGES ioctl number which no longer exists.

I'm going to have to get these chumped if I have any hope of getting these CLs in before a month from now. The device_jail changes can go in first but the revert/reimplementation CLs do actually need to either go into the CQ atomically or get chumped to avoid breaking things.

Comment 2 by ejcaruso@chromium.org, Dec 19 2017

Labels: -Pri-3 Pri-2
One more snag:

The revert changes went in on 3.8 and 3.10. This is bad, because the reimplementation changes rely on the linux-header change, and that means the reimplementation changes have to go in across every kernel at the same time. Of course, the 3.14/3.18/4.4 changes are blocked on the fact that the revert/reimplementation have to go in atomically, but currently can't -- since I removed the CQ-DEPENDs as a suggested workaround, the pre-CQ kicks out the revert changes since they cause a compilation failure without the reimplementation change applied concurrently.

At this point I really do think we should be looking at changing the submit type to unblock this series. It was created specifically because it was what Chrome OS wanted and matched up with Chrome OS expected behavior as supplied by the CQ, and this situation seems to be resistant to all other workarounds except chumping. Chumping changes across several kernel versions at once is high-risk so I think it's worth it to try something else before that.

Please advise since we have an inconsistent kernel interface in the meantime and permission_broker can't do proper sandboxing on all of our devices as long as this series remains in flight.

Comment 3 by akes...@chromium.org, Feb 23 2018

Cc: jclinton@chromium.org akes...@chromium.org
Labels: -Pri-2 Pri-3
Owner: jclinton@chromium.org
Status: Available (was: Assigned)
Not currently working on this. Disowning. May be of interest to jclinton@ team.

Comment 4 by benhenry@chromium.org, Aug 1

Status: Assigned (was: Available)
►

Sign in to add a comment