Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI/FTL] Oriole to Panther, presubmit false #130912

Merged
merged 6 commits into from
Jul 21, 2023
Merged

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Jul 19, 2023

Alternative to #130868

Oriole is the Pixel 6, Panther is the Pixel 7. Panther is more available in FTL at this point.

There's less value in running this on presubmit, since those can spawn many jobs multiple times over as people push new commits to branches. Let's just run it post submit to avoid overloading the capacity of FTL.

@dnfield dnfield changed the title Oriole to Panther, presubmit false [CI/FTL] Oriole to Panther, presubmit false Jul 19, 2023
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to include these files? If so can you add something to the description that explains why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 19, 2023
.ci.yaml Outdated
@@ -411,8 +411,9 @@ targets:
- bin/**
- .ci.yaml

- name: Linux firebase_oriol33_abstract_method_smoke_test
- name: Linux firebase_panther33_abstract_method_smoke_test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we skip the device model info in the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was originally split out like this to avoid conflict with another similarly named test that runs on a number of devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(by @godofredoc )

Copy link
Contributor

@keyonghan keyonghan Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect we will have other model change in the future, and each change will cut off the historical builds. Does it make sense to use a more general name for these two firebase tasks?
These are what we have (or proposed in this PR)
Linux firebase_abstract_method_smoke_test (based on griffin │ XT1650 | os:24 and redfin │ Pixel 5 | os:30)
Linux firebase_panther33_abstract_method_smoke_test (panther | pixel 7 | os: 33)

Does it make sense to use low_end v.s. high_end, or something similar instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could just merge this back in with the abstract_method_smoke_test because panther shoul dbe more available than oriole, and the problem was that oriole was causing timeouts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on #130499 (comment), the capacity of griffin is marked as low. We may hit similar issues for this model in the future.

Not sure about the main differences between these three devices. Is it the os that we care and want to test against? If yes, I would separate/rename the target based on their os_version instead of the model (assuming the os doesn't change and the model changes).

If we are confident the capacity is no longer an issue any more, it sgtm to merge them. But we need to find an replacement of the griffin with a device model of high/medium capacity. otherwise we will hit the same issue again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to test on some highly available physical devices and make sure we're targetting as many API levels as possible. But there's no automated monitoring right now (beyond flakiness) about whether a device has become lower in availablility or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, can we replace griffin with a high available device model with the same or close API level? Considering panther is high now, it makes to me to consolidate it back to abstract_method_smoke_test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I moved panther to the old one, removed griffin and instead test API 24 on a virtual device, and mark them all as presubmit: false so we don't overload FTL.

.ci.yaml Outdated
@@ -444,8 +427,8 @@ targets:
task_name: abstract_method_smoke_test
physical_devices: >-
[
"--device", "model=panther,version=33",
"--device", "model=redfin,version=30",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No comma is expected for the last element in the property list, and this is causing the test ownership task failure.

@dnfield
Copy link
Contributor Author

dnfield commented Jul 21, 2023

(This PR is still not merging despite having autosubmit and approvals)

@keyonghan keyonghan added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Jul 21, 2023
Copy link
Contributor

@keyonghan keyonghan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@keyonghan
Copy link
Contributor

(This PR is still not merging despite having autosubmit and approvals)

F.Y.I. @ricardoamador Could you help take a look at this PR? seems the bot misses it. Is it related to some cocoon version switch around that time? I just removed and re-added the label.

@ricardoamador
Copy link
Contributor

@keyonghan taking a look now.

@keyonghan
Copy link
Contributor

Wait, this PR is being submitted to main branch, which I don't think is ready. Is that the problem?

@ricardoamador
Copy link
Contributor

Yeah I just saw that. I don't think it should matter as it would be treated as we treat the release branches.
Tree status would not be taken into account.

@ricardoamador ricardoamador added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Jul 21, 2023
@dnfield
Copy link
Contributor Author

dnfield commented Jul 21, 2023

Hah, whups

@dnfield dnfield changed the base branch from main to master July 21, 2023 17:03
@dnfield
Copy link
Contributor Author

dnfield commented Jul 21, 2023

Maybe the bot should have warned me not to try to merge into main?

@ricardoamador
Copy link
Contributor

The bot could provide a warning but we allow this for the purposes of the release team.

It looks like something is providing an ignore temporarily status but I have not been able to find it.

@ricardoamador
Copy link
Contributor

I would be surprised if this merged after changing target to master. But if it does that will help give me a clue to where we were hung.

@ricardoamador
Copy link
Contributor

ricardoamador commented Jul 21, 2023

Hmm, are there any branch protections on main? For some reason when the target branch was changed to master I had the ability to squash and merge but with main I did not. I am wondering if this was the same for the bot?

Okay this might merge then. Which will tell us this must be a branch protection issue.

@dnfield
Copy link
Contributor Author

dnfield commented Jul 21, 2023

Main appears to have branch protections. I could not manually merge it there, but the button is enabled now that I switched it to master.

@auto-submit auto-submit bot merged commit 764095c into flutter:master Jul 21, 2023
11 checks passed
@ricardoamador
Copy link
Contributor

Looks like there were some protections in place with main that did not allow the merge. Though I think the bot should have at least logged this. I am going to open an issue to see if we can detect any of this.

@ricardoamador
Copy link
Contributor

ricardoamador commented Jul 21, 2023

Created two issues as a result of Autosubmit refusing to do its job.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 22, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jul 22, 2023
flutter/flutter@9cfbf6b...e8b397c

2023-07-22 polinach@google.com Setup leak tracking regression for material. (flutter/flutter#130169)
2023-07-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6344b17a2e03 to 481684a6e276 (2 revisions) (flutter/flutter#131118)
2023-07-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from b47cf14fda0e to 6344b17a2e03 (1 revision) (flutter/flutter#131114)
2023-07-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 840bcc3449ff to b47cf14fda0e (3 revisions) (flutter/flutter#131109)
2023-07-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2d8cff44261b to 840bcc3449ff (11 revisions) (flutter/flutter#131101)
2023-07-21 goderbauer@google.com Remove obsolete work around for shadow drawing (flutter/flutter#131066)
2023-07-21 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from acb5d0640b6c to 2d8cff44261b (3 revisions) (flutter/flutter#131092)
2023-07-21 polinach@google.com Upgrade to newer leak_tracker. (flutter/flutter#131085)
2023-07-21 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from f5c1650c7acc to acb5d0640b6c (10 revisions) (flutter/flutter#131070)
2023-07-21 32538273+ValentinVignal@users.noreply.github.com Suggest a potential valid name for the flutter project when using `flutter create` (flutter/flutter#130900)
2023-07-21 dnfield@google.com [CI/FTL] Oriole to Panther, presubmit false (flutter/flutter#130912)
2023-07-21 6655696+guidezpl@users.noreply.github.com Improve handling of certain icons in RTL (flutter/flutter#130979)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
Alternative to flutter#130868

Oriole is the Pixel 6, Panther is the Pixel 7. Panther is more available in FTL at this point.

There's less value in running this on presubmit, since those can spawn many jobs multiple times over as people push new commits to branches. Let's just run it post submit to avoid overloading the capacity of FTL.
vashworth pushed a commit to vashworth/flutter that referenced this pull request Aug 2, 2023
Alternative to flutter#130868

Oriole is the Pixel 6, Panther is the Pixel 7. Panther is more available in FTL at this point.

There's less value in running this on presubmit, since those can spawn many jobs multiple times over as people push new commits to branches. Let's just run it post submit to avoid overloading the capacity of FTL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants