[Feature flag] Enable Geo Merge Request Diff replication

Setting label(s) devopsenablement sectionenablement based on groupgeo.

added devopssystems sectioncore platform labels

Assigning myself, mostly to keep it closely on my radar. But I'm not committing to complete this.

assigned to @toon

Before enabling the FF, I wanted to check how many records it would sync:

MergeRequestDiff.replicables_for_geo_node(GeoNode.find(100109))

That results in the following SQL:

SELECT "merge_request_diffs".*
FROM "merge_request_diffs"
WHERE ("merge_request_diffs"."state" NOT IN ('without_files',
                                             'empty'))
  AND "merge_request_diffs"."stored_externally" = TRUE
  AND "merge_request_diffs"."merge_request_id" IN
    (SELECT "merge_requests"."id"
     FROM "merge_requests"
     WHERE "merge_requests"."target_project_id" IN
         (SELECT "projects"."id"
          FROM "projects"
          WHERE "projects"."namespace_id" IN
              (WITH RECURSIVE "base_and_descendants" AS (
                                                           (SELECT "geo_node_namespace_links"."namespace_id" AS id
                                                            FROM "geo_node_namespace_links"
                                                            WHERE "geo_node_namespace_links"."geo_node_id" = 100109)
                                                         UNION
                                                           (SELECT "namespaces"."id"
                                                            FROM "namespaces",
                                                                 "base_and_descendants"
                                                            WHERE "namespaces"."parent_id" = "base_and_descendants"."id")) SELECT "id"
               FROM "base_and_descendants" AS "namespaces")))
  AND "merge_request_diffs"."external_diff_store" = 1;

That times out in 15s, and I ran it again without statement_timeout, returning 0 rows.

I ran a query plan for this (warm cache): https://explain.depesz.com/s/vkos

My understanding is this scope will only be run with a range or specific ID. Actually I want to "enforce" this a bit in !43475 (merged) since I don't see a sufficient reason to allow running a query like this. I think this may avoid this problem, WDYT?

Also I though, maybe this impacts counts-- but since on secondaries, registries are SSOT, we only count the registry table there. And on the primary, it's fair not to use selective sync to count all records since all secondaries can differ in their selective sync settings. So counts generated by the primary can reasonably use only scopes like has_external_diffs, which apply in all cases.

My understanding is this scope will only be run with a range or specific ID

@mkozono You are right (of course you are ). I like the refactor you're doing in !43475 (merged), that will make it more obvious.

I reran the query for MergeRequestDiff.secondary_replicables(1..1000) (from !44938 (merged) to be more exact), resulting:

SELECT "merge_request_diffs".*
FROM "merge_request_diffs"
WHERE ("merge_request_diffs"."state" NOT IN ('without_files',
                                             'empty'))
  AND "merge_request_diffs"."stored_externally" = TRUE
  AND "merge_request_diffs"."merge_request_id" IN
    (SELECT "merge_requests"."id"
     FROM "merge_requests"
     WHERE "merge_requests"."target_project_id" IN
         (SELECT "projects"."id"
          FROM "projects"
          WHERE "projects"."namespace_id" IN
              (WITH RECURSIVE "base_and_descendants" AS (
                                                           (SELECT "geo_node_namespace_links"."namespace_id" AS id
                                                            FROM "geo_node_namespace_links"
                                                            WHERE "geo_node_namespace_links"."geo_node_id" = 100109)
                                                         UNION
                                                           (SELECT "namespaces"."id"
                                                            FROM "namespaces",
                                                                 "base_and_descendants"
                                                            WHERE "namespaces"."parent_id" = "base_and_descendants"."id")) SELECT "id"
               FROM "base_and_descendants" AS "namespaces")))
  AND "merge_request_diffs"."external_diff_store" = 1
  AND "merge_request_diffs"."id" BETWEEN 1 AND 1000;

Plan: https://explain.depesz.com/s/qeUr

It completed in 220ms, but returned 0 rows, because on prod all records are on object storage.

So I tried again without "merge_request_diffs"."external_diff_store" = 1, because that part gets added by MergeRequestDiff.object_storage_scope:

Plan: https://explain.depesz.com/s/PqjP

It completes in 650ms, which isn't great, but could have been way worse. Most time is spent on searching for MRs in selective sync project (Index Cond: (projects.id = merge_requests.target_project_id)) and searching MR diff linked to those (Index Cond: (merge_requests.id = merge_request_diffs.merge_request_id)). At the moment I don't think there's a lot we can do about that, but considering the number of rows in those tables, it's acceptable.

If we would enable Object Storage sync on Staging, we would sync 217115 rows. But we cannot just simply enable it, because this setting works on all replicables, so also container registry and others will start syncing with it enabled.

So @mkozono suggested, it's probably only possible to properly test this in a separate test setup on GCP. But that's pretty time-consuming to set up.

@nhxnguyen Is there anyone on the team with a setup ready on GCP to test this feature more thoroughly? I have no experience with the orchestrator and generating test data might be tedious. So if possible I would like to hand this off to someone else on the Geo team.

Is there anyone on the team with a setup ready on GCP to test this feature more thoroughly?

Thanks for the update @toon. We'll find someone to test the feature.

@alexives Has volunteered to test and work on the feature flag MR

Just adding a note that there was a recent addition to the framework doc which we'll need to follow here, regarding enabling the feature + the GraphQL field: !43249 (merged)

changed the description

added workflowready for development + 1 deleted label

assigned to @alexives

mentioned in commit 0602c176

added workflowin dev label and removed workflowready for development label

mentioned in commit 0a077505

mentioned in commit 31d4d8ca

mentioned in merge request !45224 (merged)

marked the checklist item Test in GCP (e.g. with instances spun up with ansible-gitlab-geo) as completed

[Feature flag] Enable Geo Merge Request Diff replication

What

Owners

Expectations

What are we expecting to happen?

What might happen if this goes wrong?

What can we monitor to detect problems with this?

Beta groups/projects

Roll Out Steps

Designs

Child items ...

Activity

[Feature flag] Enable Geo Merge Request Diff replication

What

Owners

Expectations

What are we expecting to happen?

What might happen if this goes wrong?

What can we monitor to detect problems with this?

Beta groups/projects

Roll Out Steps

Relates to

Activity