Page MenuHomePhabricator

Core tests failing due to Flow HTTP requests and ServiceContainer access
Open, HighPublic

Description

Recently, core tests started failing due to flow issues (ContainerDisabledException when accessing services, and HTTP requests to ... blocked. Use MockHttpTrait.)

Not sure what changed, but this is resulting in V-1 on unrelated core patches.

Example: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/705477 is an empty patch and the wmf-quibble-core-vendor-mysql-php72-docker job failed.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 moved this task from Unsorted to Reports on the User-DannyS712 board.
DannyS712 updated the task description. (Show Details)

Could this have been caused by T286187: Release Quibble 1.0.0? Its only failing for the wmf-quibble-core-vendor-mysql-php72-docker job, so maybe something that changed between the prior version of quibble and the new one caused it

Nikerabbit raised the priority of this task from High to Unbreak Now!.Tue, Jul 20, 11:47 AM
Nikerabbit added a subscriber: Nikerabbit.

Merge blocker -> UBN!

Came here because it also affects Translate and probably many other extensions too.

Extensions like GrowthExperiments are also failing.

Might have been caused by https://gerrit.wikimedia.org/r/c/integration/quibble/+/703182 which could have changed the way Parsoid requests made by Flow go, possibly?

Can these tests be skipped on the short term? I have some time sensitive changes relating to desktop improvements that need to go through today that are being blocked by this issue.

I have seen this issue in Flow, Vector and core.

Might have been caused by https://gerrit.wikimedia.org/r/c/integration/quibble/+/703182 which could have changed the way Parsoid requests made by Flow go, possibly?

Can this be reverted?

Change 705746 had a related patch set uploaded (by Zabe; author: Zabe):

[integration/quibble@master] Revert "Load Parsoid from vendor as fallback, and configure"

https://gerrit.wikimedia.org/r/705746

Change 705747 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] Revert "jjb: update jobs to Quibble 1.0.0"

https://gerrit.wikimedia.org/r/705747

Mentioned in SAL (#wikimedia-releng) [2021-07-20T18:39:43Z] <hashar> Rolling back Jenkins jobs from Quibble 1.0.0 to 0.0.47 # T287001

Change 705747 merged by jenkins-bot:

[integration/config@master] Revert "jjb: update jobs to Quibble 1.0.0"

https://gerrit.wikimedia.org/r/705747

Change 705746 abandoned by Zabe:

[integration/quibble@master] Revert "Load Parsoid from vendor as fallback, and configure"

Reason:

Per hashar's comment

https://gerrit.wikimedia.org/r/705746

Quibble 1.0.0 went with a tweak to LocalSettings.php to load Parsoid: https://gerrit.wikimedia.org/r/c/integration/quibble/+/703182/5/quibble/mediawiki/local_settings.php.tpl and I have deployed the Jenkins jobs a few hours ago which caused the issue. I have rolled back the jobs to 0.0.47 so the issue is fixed.

A patch related to the config change was: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Flow/+/703186 Prevent network requests. Which sounds related?

hashar lowered the priority of this task from Unbreak Now! to High.Tue, Jul 20, 6:51 PM

I have marked the build https://integration.wikimedia.org/ci/job/wmf-quibble-core-vendor-mysql-php72-docker/58287/ to be kept forever back referencing to this bug.

All Jenkins jobs are now back to 0.0.47 so lowering the priority.

I marked the line in https://gerrit.wikimedia.org/r/c/integration/quibble/+/703182 which I suspect to be the culprit, but fundamentally I don't think that patch should have been necessary in the first place. It seems to be confusing VE "autoconfig" with loading parsoid as an extension. VE "autoconfig" in release builds is only needed because Parsoid is *not* loaded as an extension in release builds, due to limitations in how the installer handles extension dependencies, and VE "autoconfig" is actually rather misnamed because what it consists of is some code copy-and-pasted from Parsoid so that it is loaded as part of the VE extension instead of as part of the Parsoid extension (since the extension installer doesn't know enough to load the Parsoid extension). The functionality is mostly completely unrelated to "configuration". If you load Parsoid as an extension (as quibble does) you don't need to mess around with that code, and so it doesn't matter if you're running a release build or not.

Parsoid already runs "as a service" when we do API testing with the core API testing framework (poking @Arlolra who worked on this), so Flow ought to be able to do the same thing without messing with the quibble configuration.

That said, I've paged out some of the details of exactly how this works, and maybe it's related to some special magic in quibble for the mediawiki-quibble-apitests-vendor-php72-docker jobs. But I'm pretty sure the "right way" to support flow here is to make flow use the same mechanism that quibble-apitests does, instead of running Parsoid in a new special configuration.

See for example the CI run in https://integration.wikimedia.org/ci/job/mediawiki-quibble-apitests-vendor-php72-docker/11607/

Probably also related: T233736: Testing the REST API in CI

You *will* need to add parsoid as a dependency in integration-config/zuul/parameter_functions.py. It may *seem* that Flow should have pulled in Parsoid due to Flow's dependency on VisualEditor, but VisualEditor takes special care never to actually require Parsoid in its CI. (That's a historical artifact and better tests for VE should probably be written.) So the Flow jobs won't currently install the Parsoid "extension" and so the Parsoid REST API won't be available to Flow as integration-config stands right now. That's easily mended w/o a quibble release, though.

See the patches I attached to T218534.

Change 705907 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[integration/quibble@master] Revert "Load Parsoid from vendor as fallback, and configure"

https://gerrit.wikimedia.org/r/705907

Can we save the logs for one of the failed builds in GrowthExperiments as well? I assume that Flow's CI tests are the root cause here. The quibble patch effectively turned on Parsoid in all repositories at once, in addition to configuring Parsoid in a non-standard way. So if other extensions failed when Parsoid was turned on that would be interesting to the parsing team to look into, even though it shouldn't be a near term blocker.

Can we save the logs for one of the failed builds in GrowthExperiments as well? I assume that Flow's CI tests are the root cause here. The quibble patch effectively turned on Parsoid in all repositories at once, in addition to configuring Parsoid in a non-standard way. So if other extensions failed when Parsoid was turned on that would be interesting to the parsing team to look into, even though it shouldn't be a near term blocker.

https://integration.wikimedia.org/ci/job/wmf-quibble-vendor-mysql-php72-docker/69950/console

Can we save the logs for one of the failed builds in GrowthExperiments as well? I assume that Flow's CI tests are the root cause here. The quibble patch effectively turned on Parsoid in all repositories at once, in addition to configuring Parsoid in a non-standard way. So if other extensions failed when Parsoid was turned on that would be interesting to the parsing team to look into, even though it shouldn't be a near term blocker.

https://integration.wikimedia.org/ci/job/wmf-quibble-vendor-mysql-php72-docker/69950/console

I marked this one as kept forever, too.

I *believe* that the failure mode here is (a) the final integrated test on many repos runs core tests with a set of 'important for production' extensions that includes Flow, (b) Flow/includes/Conversion/Utils.php contains an isParsoidConfigured() method, and the tests in question used Utils::convert() which uses the legacy parser if isParsoidConfigured() returns false, and (c) the quibble patch set $wgVirtualRestConfig['modules']['parsoid'] = []; and isset( $vrs['modules']['parsoid'] ) is one of the tests used by Utils::makeVRSObject() to determine if Parsoid is configured. So isParsoidConfigured() started to return true which caused Utils::convert() to run down a different code path which triggered various failures.

Since these tests cases previously assumed Parsoid was not configured, I'd expect various bits of test output would probably need to be updated if they are run with Parsoid providing conversion output, independent of the fact that invoking Parsoid via the API was failing, although maybe not.

In any case, gated extensions currently run with Parsoid disabled. There's a patch sequence culminating in https://gerrit.wikimedia.org/r/c/integration/config/+/655695 to enable that.

Since set_gated_extensions() *doesn't* process the dependency list that set_mw_dependencies uses, adding parsoid as a dependency of flow (ie https://gerrit.wikimedia.org/r/c/integration/config/+/705932 ) won't break the world, it may just break Flow CI. All the rest of the repositories which run the extension-gate tests will still be running Flow in !isParsoidConfigured() mode, and so should be unaffected.

Change 705966 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[integration/config@master] Run extension-gate jobs experimentally for Parsoid

https://gerrit.wikimedia.org/r/705966

@Zabe @Urbanecm Thanks. Examination shows that's still the same set of Flow tests failing. So hopefully if the flow tests are fixed then those failures won't block https://gerrit.wikimedia.org/r/c/integration/config/+/655695.

Brief to-do, to summarize:

  1. Revert the original parsoid-related patch in quibble ( https://gerrit.wikimedia.org/r/c/integration/quibble/+/705907 ) tag and release quibble 0.10.1 and rebuild images (alas)
  2. Add parsoid as a dependency of Flow ( https://gerrit.wikimedia.org/r/c/integration/config/+/705932 ). This may temporarily break Flow CI, but shouldn't break anyone else using extension-gate tests, since the gatedextension list ignores dependencies and so Parsoid will still be disabled for extension-gate builds. Flow devs can fix their tests if needed, and/or write the new API tests envisioned by T218534. They should probably take care that their tests will pass whether or not Parsoid is enabled, though, at least for an interim period (ie, cleanly skip tests that require parsoid if the parsoid extension is not installed).
  3. Add the extension-gate tasks experimentally to Parsoid ( https://gerrit.wikimedia.org/r/c/integration/config/+/705966 ). This will allow Parsoid and Flow devs to look into these issues by running check experimental on empty commits in the parsoid repo.
  4. Someday: add Parsoid to the gated extension list ( https://gerrit.wikimedia.org/r/c/integration/config/+/655695 ) after Flow (and any other extension) issues are fixed.

Change 705966 merged by jenkins-bot:

[integration/config@master] Run extension-gate jobs experimentally for Parsoid

https://gerrit.wikimedia.org/r/705966

Change 705907 merged by jenkins-bot:

[integration/quibble@master] Revert \"Load Parsoid from vendor as fallback, and configure\"

https://gerrit.wikimedia.org/r/705907

Change 707337 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/quibble@master] Release Quibble 1.0.1

https://gerrit.wikimedia.org/r/707337

Change 707337 merged by jenkins-bot:

[integration/quibble@master] Release Quibble 1.0.1

https://gerrit.wikimedia.org/r/707337

Change 707348 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] dockerfiles: update to Quibble 1.0.1

https://gerrit.wikimedia.org/r/707348

Change 707348 merged by jenkins-bot:

[integration/config@master] dockerfiles: update to Quibble 1.0.1

https://gerrit.wikimedia.org/r/707348