[scheduler] Migrate rerun logic to Cocoon scheduler #1610
Conversation
CaseyHillers
requested review from
godofredoc
and removed request for
keyonghan and
godofredoc
|
Note to reviewers: This is still WIP. I'm coming back to it now and pushed some WIP changes to my remote branch |
| @@ -119,14 +119,17 @@ class RefreshChromebotStatus extends ApiRequestHandler<Body> { | |||
| final Task update = datastoreTask.task; | |||
| update.status = latestLuciTask.status; | |||
|
|
|||
| final CiYaml ciYaml = await scheduler.getCiYaml(datastoreTask.commit); | |||
There was a problem hiding this comment.
It's expensive to load the CiYaml for every single task. Can we load it on commit level?
There was a problem hiding this comment.
scheduler.getCiYaml is cached making it trivial to pull up. Since this logic is only a for loop against existing tasks, is it ok to leave as is? It's difficult to refactor this code to be a double for loop of commit to tasks
| final Map<String, dynamic> defaultProperties = | ||
| repo == 'engine' ? Config.engineDefaultProperties : const <String, dynamic>{}; |
There was a problem hiding this comment.
Why this is removed? We need to inject this to make sure the engine rerun succeeds.
There was a problem hiding this comment.
It's in luci build service now. See
| final CiYaml ciYaml = await scheduler.getCiYaml(datastoreTask.commit); | ||
| final Target target = | ||
| ciYaml.postsubmitTargets.singleWhere((Target target) => target.value.name == datastoreTask.task.name); | ||
|
|
||
| /// Use `update.attempts - 1` as the `retries` to skip the initial run. |
There was a problem hiding this comment.
Thanks! Removed the extra logic here
| commit: datastoreTask.commit, | ||
| target: target, | ||
| task: update, | ||
| datastore: datastore, |
There was a problem hiding this comment.
With ignoreChecks default false, this rerun will never be triggered.
There was a problem hiding this comment.
ignoreChecks is reserved for the reset-prod-test use case as humans likely have a better idea if we should retry (such as needing a 4th retry). In this case, refresh chromebot will trigger tests as long as it meets the conditions in checkRerunBuilder (< 3 attempts, latest datastore does not indicate it passed).
| commit: commit, | ||
| target: target, | ||
| task: task, | ||
| priority: 29, |
There was a problem hiding this comment.
I moved this into a const variable (along with the other priorities)
| @@ -50,6 +49,10 @@ class LuciBuildService { | |||
|
|
|||
| static const Set<Status> failStatusSet = <Status>{Status.canceled, Status.failure, Status.infraFailure}; | |||
|
|
|||
| static const int kDefaultPriority = 30; | |||
| static const int kRerunPriority = 29; | |||
| static const int kReleasePriority = 25; | |||
There was a problem hiding this comment.
We agreed to use same priority for release builds?
There was a problem hiding this comment.
This is for their presubmits. When we start rolling out the cocoon scheduler on release branches, we'll need to revisit this. I filed flutter/flutter#99876 to follow up
There was a problem hiding this comment.
kReleasePriority is only referenced in post submit function _createPostsubmitScheduleBuild below, but is not injected to ScheduleBuildRequest. I am a bit confused about how this is related to the pre-submit schedule and which logic in this PR is expected to use it.
There was a problem hiding this comment.
Ah, you're right. I removed this logic (I'm not sure why we had it there in the first place)
CaseyHillers
added
the
waiting for tree to go green
This is to fix reruns in cocoon, plugins, packages dashboards, and should help fix the engine issues.
flutter/flutter#92300
flutter/flutter#98674
Fixes flutter/flutter#99370