2PC on CI branch
What does this MR do?
Proof of concept. This MR tries to detect cross-database writes within a transaction during CI test run and raise errors.
Example: INSERT
, UPDATE
or DELETE
query is triggered for both ci_*
and non-ci tables.
Issue.transaction do
Issue.create(...)
Ci::Pipeline.create(...)
end
How does it work?
In the spec_helper.rb
class, we have the following option: use_transactional_fixtures = true
. This makes each test case run its own transaction. During the test run, a test case can be wrapped in multiple transactions (using savepoints):
- Transaction created by
use_transactional_fixtures
- Transaction created by
let_it_be
- Test case is running
- Application code may create transactions (savepoints), this is what we're looking for
- Rollback
Note: this works for schemas (single DB), for multiple databases, we'll need a slightly different logic (track savepoints per DB)
The TransactionTracker
functionality does the following for each test case:
- Detect the point where the savepoints created by the test frameworks are finished.
- Start collecting the created savepoints.
- Collect
UPDATE
,INSERT
,DELETE
DB queries and extract the referenced DB tables. - Check for offenses.
The tracking is disabled for the following cases:
InternalId
- FactoryBot record creation
Example from the log:
TRANSACTION (0.1ms) SAVEPOINT active_record_3 /*application:test,correlation_id:662b936c6988c202435bce3ee6f2d79e*/
↳ config/initializers/00_rails_disable_joins.rb:17:in `scope'
-- Savepoint created, name: active_record_3
ProjectMember Destroy (0.1ms) DELETE FROM "members" WHERE "members"."type" = 'ProjectMember' AND "members"."source_id" = 2 AND "members"."source_type" = 'Project' AND "members"."requested_at" IS NOT NULL /*application:test,correlation_id:662b936c6988c202435bce3ee6f2d79e*/
↳ spec/models/project_spec.rb:3265:in `block (4 levels) in <top (required)>'
DB accessed with savepoint(active_record_3): main
NotificationSetting Destroy (0.1ms) DELETE FROM "notification_settings" WHERE "notification_settings"."source_id" = 2 AND "notification_settings"."source_type" = 'Project' /*application:test,correlation_id:662b936c6988c202435bce3ee6f2d79e*/
↳ spec/models/project_spec.rb:3265:in `block (4 levels) in <top (required)>'
ContainerRepository Load (0.2ms) SELECT "container_repositories".* FROM "container_repositories" WHERE "container_repositories"."project_id" = 2 /*application:test,correlation_id:662b936c6988c202435bce3ee6f2d79e*/
↳ spec/models/project_spec.rb:3265:in `block (4 levels) in <top (required)>'
DB accessed with savepoint(active_record_3): ci
Ci::Pipeline Destroy (0.7ms) DELETE FROM "ci_pipelines" WHERE "ci_pipelines"."project_id" = 2 AND ("ci_pipelines"."source" IN (1, 2, 3, 4, 5, 6, 7, 8, 10, 11) OR "ci_pipelines"."source" IS NULL) /*application:test,correlation_id:662b936c6988c202435bce3ee6f2d79e*/
↳ spec/models/project_spec.rb:3265:in `block (4 levels) in <top (required)>'
-- CI table(s) and Main table(s) were referenced within a transaction: ci_pipelines
Screenshots or Screencasts (strongly suggested)
Does this MR meet the acceptance criteria?
Conformity
-
I have included changelog trailers, or none are needed. (Does this MR need a changelog?) -
I have added/updated documentation, or it's not needed. (Is documentation required?) -
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) -
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) -
I have self-reviewed this MR per code review guidelines. -
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) -
I have followed the style guides. -
This change is backwards compatible across updates, or this does not apply.
Availability and Testing
-
I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) -
I have tested this MR in all supported browsers, or it's not needed. -
I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.
Security
Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team