Maniphest T202116

LoadBalancer opening extra connections in different connection categories doesn't work with PHPUnit & temporary tables
Closed, ResolvedPublic
Actions

Description

This is a followup to T201137: WikibaseLexeme 'jenkins_u0_mw.unittest_content_models' doesn't exist.

While running phpunit with temporary tables, the temporary tables will only exist on a single DB connection.
LoadBalancer::openLocalConnection can open extra connections if the auto commit flag is passed in and thus a connection category / connKey of "localAutoCommit" is used instead of "local".
In cases where this happens the new connection will not have access to the temporary tables created on the initial connection.

Hopefully now this issue is on phabricator if someone runs into it again they'll spot this and quickly figure out what is happening.

CCing @aaron as he probably has an idea of what is best.

Possible ways forward

In unit tests when a local connection with autocommit is loaded, instead load the same connection that already exists?
In unit tests when a local connection with autocommit is loaded, instead fail HARD, throw an exception that makes sense. (something like https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/450524/ perhaps)
Stop using temporary tables in unit tests & CI in general? (Adding @hashar for this idea specifically)

Details

	Subject	Repo	Branch	Lines +/-
	rdbms: add setTempTablesOnlyMode() to suppress CONN_TRX_AUTOCOMMIT during tests	mediawiki/core	master	+45 -21

Customize query in gerrit

Related Objects
Search...

		Status	Subtype	Assigned	Task
		Open		None	T225730 Reduce runtime of MW shared gate Jenkins jobs to 5 min
		Resolved		aaron	T202116 LoadBalancer opening extra connections in different connection categories doesn't work with PHPUnit & temporary tables

Event Timeline

Addshore created this task.Aug 17 2018, 7:22 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 17 2018, 7:22 AM

Addshore moved this task from Unsorted 💣 to Watching 👀 on the User-Addshore board.Aug 17 2018, 7:23 AM

Addshore updated the task description. (Show Details)Aug 17 2018, 8:03 AM

I was going to open a new task for this problem and noticed this task just before pressing "Create New task". This is the paste of what I was going to say:

While writing unit tests for AbuseFilter, I came to testing global filters, which are stored in the DB of a central wiki and executed in other wikis. The code to test instantiates a LoadBalancer instance to connect to the foreign database (here), but when this happens inside a unit test the prefix 'unittest_' is gone. Although I don't know DB-related classes really well, I can say that this happens due to LoadBalancer::openForeignConnection, which sets up a connection with no prefix. For reference, this is the patch to add the unit tests in question. There may be an error in there, but losing prefixes shouldn't really happen and I'm opening this task to be sure.

So my assumption seems to be almost correct, and this is actually a core problem. If, instead, I'm not right, then I'll open a separate task for the issue.

Daimona mentioned this in T201193: Code coverage is low in AbuseFilter, Aim to increase to 50%.Aug 26 2018, 5:37 PM

In T202116#4533014, @Daimona wrote:

I was going to open a new task for this problem and noticed this task just before pressing "Create New task". This is the paste of what I was going to say:

While writing unit tests for AbuseFilter, I came to testing global filters, which are stored in the DB of a central wiki and executed in other wikis. The code to test instantiates a LoadBalancer instance to connect to the foreign database (here), but when this happens inside a unit test the prefix 'unittest_' is gone. Although I don't know DB-related classes really well, I can say that this happens due to LoadBalancer::openForeignConnection, which sets up a connection with no prefix. For reference, this is the patch to add the unit tests in question. There may be an error in there, but losing prefixes shouldn't really happen and I'm opening this task to be sure.

So my assumption seems to be almost correct, and this is actually a core problem. If, instead, I'm not right, then I'll open a separate task for the issue.

I don't think that is quite the same as this issue.
This issue talks specifically about the connection groups within a single LoadBalancer but all pointing to the same underlying DB.
Your issue sounds like an AbuseFilter issue, perhaps in the abuse filter tests a new connection should not be used, just pass in the connection being used in the tests.

In T202116#4533815, @Addshore wrote:

In T202116#4533014, @Daimona wrote:

I was going to open a new task for this problem and noticed this task just before pressing "Create New task". This is the paste of what I was going to say:

While writing unit tests for AbuseFilter, I came to testing global filters, which are stored in the DB of a central wiki and executed in other wikis. The code to test instantiates a LoadBalancer instance to connect to the foreign database (here), but when this happens inside a unit test the prefix 'unittest_' is gone. Although I don't know DB-related classes really well, I can say that this happens due to LoadBalancer::openForeignConnection, which sets up a connection with no prefix. For reference, this is the patch to add the unit tests in question. There may be an error in there, but losing prefixes shouldn't really happen and I'm opening this task to be sure.

So my assumption seems to be almost correct, and this is actually a core problem. If, instead, I'm not right, then I'll open a separate task for the issue.

I don't think that is quite the same as this issue.
This issue talks specifically about the connection groups within a single LoadBalancer but all pointing to the same underlying DB.
Your issue sounds like an AbuseFilter issue, perhaps in the abuse filter tests a new connection should not be used, just pass in the connection being used in the tests.

In fact, reading the task description again, it seems like a different issue. However, in the AF case, the two connections are still pointing to the same DB (for convenience), so this led me to thinking it actually was the same. While I'm not sure that the test connection can be passed to the actual code, it'd be a partial relief to know that this is not a core problem.

Krinkle moved this task from Inbox to PHPUnit on the MediaWiki-Core-Tests board.Oct 5 2018, 8:20 PM

Krinkle moved this task from Untriaged to Rdbms library on the MediaWiki-libs-Rdbms board.Jun 27 2019, 9:08 PM

This area of wikimedia/rdbms has been significantly refactored by @aaron in recent months. Tagging our workboard for him to triage when he's back. This may've been fixed already.

Restricted Application added a project: Platform Engineering. · View Herald TranscriptJul 23 2019, 6:03 PM

Krinkle triaged this task as Medium priority.Jul 23 2019, 6:04 PM

Krinkle removed a project: Platform Engineering.

Krinkle assigned this task to aaron.Aug 6 2019, 5:56 PM

aaron moved this task from Inbox, needs triage to Backlog: Maintenance, non-prioritized on the Performance-Team board.Aug 12 2019, 7:45 PM

Change 530982 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/core@master] rdbms: add setTempTablesOnlyMode() to suppress CONN_TRX_AUTOCOMMIT during tests

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

gerritbot added a project: Patch-For-Review.Aug 19 2019, 10:50 PM

hashar unsubscribed.Aug 20 2019, 8:09 AM

Change 530982 merged by jenkins-bot:
[mediawiki/core@master] rdbms: add setTempTablesOnlyMode() to suppress CONN_TRX_AUTOCOMMIT during tests

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

Maintenance_bot removed a project: Patch-For-Review.Aug 25 2019, 3:10 PM

ReleaseTaggerBot added a project: MW-1.34-notes (1.34.0-wmf.20; 2019-08-27).Aug 25 2019, 4:00 PM

aaron closed this task as Resolved.Aug 25 2019, 9:37 PM

Krinkle added a parent task: T225730: Reduce runtime of MW shared gate Jenkins jobs to 5 min.Aug 26 2019, 7:52 PM

Krinkle mentioned this in T225730: Reduce runtime of MW shared gate Jenkins jobs to 5 min.

daniel mentioned this in T234031: PageUpdaterTest fails when run in isolation due to setTempTableOnlyMode() being called with wrong domain..Sep 27 2019, 11:27 AM