Summary: | [EWS] Layout testers can go in an infinite RETRY loop when there are 30+ failures on trunk | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aakash Jain <aakash_jain> | ||||||
Component: | Tools / Tests | Assignee: | Aakash Jain <aakash_jain> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aakash_jain, ap, ddkilzer, jbedard, ryanhaddad, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=212755 https://bugs.webkit.org/show_bug.cgi?id=216046 https://bugs.webkit.org/show_bug.cgi?id=216487 |
||||||||
Attachments: |
|
Description
Aakash Jain
2019-10-31 14:28:23 PDT
Few more examples from mac-debug-wk1 queue: https://ews-build.webkit.org/#/builders/16/builds/7137 https://ews-build.webkit.org/#/builders/16/builds/7142 https://ews-build.webkit.org/#/builders/16/builds/7143 https://ews-build.webkit.org/#/builders/16/builds/7144 There were 30+ crashes on trunk yesterday due to https://bugs.webkit.org/show_bug.cgi?id=201908 (e.g.: https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK1%20%28Tests%29/builds/11748) Created attachment 384207 [details]
Patch
Comment on attachment 384207 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384207&action=review > Tools/BuildSlaveSupport/ews-build/loadConfig.py:142 > + if not isTriggerUsedByAnyBuilder(config, scheduler['name']) and 'build' not in scheduler['name'].lower(): Is this saying that testers cannot trigger other testers? > Tools/BuildSlaveSupport/ews-build/steps.py:494 > + self.include_revision = include_revision What is 'revision' for EWS? The git hash? > Tools/BuildSlaveSupport/ews-build/steps.py:1412 > + self.finished(SUCCESS) Won't this turn the bubble green for a bit? Also, we still have an infinite loop problem is someone broke the build, right? Comment on attachment 384207 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384207&action=review >> Tools/BuildSlaveSupport/ews-build/loadConfig.py:142 >> + if not isTriggerUsedByAnyBuilder(config, scheduler['name']) and 'build' not in scheduler['name'].lower(): > > Is this saying that testers cannot trigger other testers? This was testing that we do not have any unused triggers in config.json. But now it's possible to have a valid case of triggers which no-one in config.json will use directly (since these triggers would be triggered dynamically). Effectively yes, we don't have such a scenario (testers triggering other testers) yet. >> Tools/BuildSlaveSupport/ews-build/steps.py:494 >> + self.include_revision = include_revision > > What is 'revision' for EWS? The git hash? yeah, it's git hash >> Tools/BuildSlaveSupport/ews-build/steps.py:1412 >> + self.finished(SUCCESS) > > Won't this turn the bubble green for a bit? > > Also, we still have an infinite loop problem is someone broke the build, right? This will turn only the step green. Build would be red/FAILED because of previous layout-test steps failing. Comment on attachment 384207 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384207&action=review > Tools/BuildSlaveSupport/ews-build/config.json:349 > + "triggered_by": ["ios-13-sim-build-ews"], This makes sense to me, but I don't understand why this isn't already here, since ios-simulator-13 has been triggered by ios-13-sim-build-ews for months. Comment on attachment 384207 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384207&action=review >> Tools/BuildSlaveSupport/ews-build/config.json:349 >> + "triggered_by": ["ios-13-sim-build-ews"], > > This makes sense to me, but I don't understand why this isn't already here, since ios-simulator-13 has been triggered by ios-13-sim-build-ews for months. This 'triggered_by' key is used just for passing information. I wasn't able to dynamically extract this information (about who triggered current build) using buildbot. So I am passing this information in a static manner. This information is used by tester build, to trigger back the builder. Yes, iOS-13-Simulator-WK2-Tests-EWS is triggered by iOS-13-Simulator-Build-EWS using the 'triggers' key. Comment on attachment 384207 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384207&action=review >>> Tools/BuildSlaveSupport/ews-build/config.json:349 >>> + "triggered_by": ["ios-13-sim-build-ews"], >> >> This makes sense to me, but I don't understand why this isn't already here, since ios-simulator-13 has been triggered by ios-13-sim-build-ews for months. > > This 'triggered_by' key is used just for passing information. I wasn't able to dynamically extract this information (about who triggered current build) using buildbot. So I am passing this information in a static manner. This information is used by tester build, to trigger back the builder. > > Yes, iOS-13-Simulator-WK2-Tests-EWS is triggered by iOS-13-Simulator-Build-EWS using the 'triggers' key. That's unfortunate, but does answer my question! Created attachment 408716 [details]
Patch for landing
Comment on attachment 408716 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=408716&action=review Did some more testing, and updated patch accordingly. Also re-based it on ToT. > Tools/BuildSlaveSupport/ews-app/ews/views/statusbubble.py:156 > + bubble['state'] = 'provisional-fail' Updated status-bubble code here to ensure that the status-bubble is orange when the build is waiting to be retried. > Tools/BuildSlaveSupport/ews-build/factories_unittest.py:50 > def test_generic_factory(self): Updated factories_unittest.py accordingly. > Tools/BuildSlaveSupport/ews-build/steps.py:2186 > + self.setProperty('build_summary', message) Added this line to ensure that the buildbot build summary is set properly. Sample run: Builder build: https://ews-build.webkit-uat.org/#/builders/14/builds/1594 Corresponding tester build: https://ews-build.webkit-uat.org/#/builders/16/builds/122, it correctly triggered back the builder in step #30. Re-triggered builder build: https://ews-build.webkit-uat.org/#/builders/14/builds/1607 , it correctly updated to ToT and triggered tester with ToT. Corresponding status-bubble for ios-wk2 is correctly orange and hover-over message is also correct in https://ews.webkit-uat.org/status-bubble/407969/ Committed r267022: <https://trac.webkit.org/changeset/267022> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408716 [details]. This change seems to be working fine. e.g.: https://ews-build.webkit.org/#/builders/24/builds/26185 failed with 30+ failures. It correctly re-trigerred the parent builder in: https://ews-build.webkit.org/#/builders/23/builds/27400. This correctly triggered the tester again with ToT in https://ews-build.webkit.org/#/builders/24/builds/26223, which passed. (In reply to Aakash Jain from comment #14) > This change seems to be working fine. > > e.g.: https://ews-build.webkit.org/#/builders/24/builds/26185 failed with > 30+ failures. It correctly re-trigerred the parent builder in: > https://ews-build.webkit.org/#/builders/23/builds/27400. This correctly > triggered the tester again with ToT in > https://ews-build.webkit.org/#/builders/24/builds/26223, which passed. Awesome! Thanks Aakash! |