Sam Word (de00087f) at 19 Mar 15:33

@timmccarthy this looks good to me as well! ✅

What does this MR do and why?

This MR adds missing belongs_to associations to the Geo Design Management Repository State model to support organization transfer functionality.

Models updated:

• Geo::DesignManagementRepositoryState

This is part of a series of MRs split from !227325 to add missing associations across ~70 models.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Related MRs

This MR is part of a series splitting !227325 into smaller domain-specific changes:

Reviewer note: I took a guess at what the rate limit should be, but here's my thinking: I figure there's not much value in requesting this endpoint in somewhat quick succession for the same source user because it just enqueues a background job to retry reassignment that will either succeed or fail. If it succeeds, then subsequent requests to this request fail as the source user is no longer failed. If it repeatedly fails, I don't want to put too much pressure on our background workers if it keeps failing.

What does this MR do and why?

This adds a GraphQL mutation to allow users to retry failed placeholder reassignments via the API using a new service implemented in !227954.

References

• Placeholder user mapping docs: https://docs.gitlab.com/user/import/mapping/#bypass-confirmation-when-reassigning-placeholder-users
• Related to #589777

How to set up and validate locally

Follow the validation steps in !227954 to create a source user and make it fail, but use the new mutation to retry the failed reassignment instead of the console

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #589777

Hey @rodrigo.tomonari Would you mind doing the initial backend review of this MR? Thanks! 🙇

This is what other source user action services like AcceptReassignmentService and ReassignService do so I'll leave this as-is for consistency

This service inherits from top-level BaseService

If these come from Import::SourceUsers::BaseService, this class should inherit from that class (or include the module) to avoid runtime failures.

???

They're assigned with attr_reader in Import::SourceUsers::BaseService, which is this service inherits from. Additionally, testing locally never raised a NameError at runtime

Sam Word (10f1fe14) at 18 Mar 21:09

... and 5 more commits

Reviewer note: The ability to manage source user reassignments was named admin_import_source_user before this check was implemented and it's out of this MR's scope to change

Sam Word (24d6de49) at 18 Mar 19:24

... and 1 more commit

What does this MR do and why?

When a placeholder user reassignment fails after an import, the Import::SourceUser record gets stuck in a FAILED status with no way to retry or recover. This MR adds a new service that allows top-level group owners to retry failed placeholder reassignments as part of implementing a "retry" action.

A new GraphQL mutation to call this service will be implemented in a subsequent MR and no other components call the new service yet. This MR is part of the implementation of #589777

References

• Placeholder user mapping docs: https://docs.gitlab.com/user/import/mapping/#bypass-confirmation-when-reassigning-placeholder-users
• Related to #589777

How to set up and validate locally

Because nothing calls this service, validate by checking that specs pass and the service performs as expected in the console:

1. Find or create an import source user (placeholder user) by running an import from any of the supported importers. Do not import a project into a personal namespace because it will assign all contributions directly to the personal namespace's user. You can also create an Import::SourceUser in the console, but I recommend performing an import to simulate a real failure with user contributions
2. Once the import finishes, make it so that the initial reassignment attempt fails. Manually raising an error is the most straightforward way to guarantee it. Something like this should simulate the bug that led to this MR's issue:

diff --git a/app/services/import/reassign_placeholder_user_records_service.rb b/app/services/import/reassign_placeholder_user_records_service.rb
index bf7fdf2ca26669..7a2597b045e82c 100644
--- a/app/services/import/reassign_placeholder_user_records_service.rb
+++ b/app/services/import/reassign_placeholder_user_records_service.rb
@@ -31,6 +31,7 @@ def execute
 
         reassign_placeholder_references
 
+        raise StandardError, 'Break before reassigning memberships'
         delete_remaining_references
 
         if placeholder_memberships.any?
diff --git a/app/workers/import/reassign_placeholder_user_records_worker.rb b/app/workers/import/reassign_placeholder_user_records_worker.rb
index b1294989ee8a73..df7bc18678cc2b 100644
--- a/app/workers/import/reassign_placeholder_user_records_worker.rb
+++ b/app/workers/import/reassign_placeholder_user_records_worker.rb
@@ -11,7 +11,7 @@ class ReassignPlaceholderUserRecordsWorker
     data_consistency :sticky
     feature_category :importers
     deduplicate :until_executed
-    sidekiq_options retry: 5, dead: false
+    sidekiq_options retry: 1, dead: false
     sidekiq_options max_retries_after_interruption: 20
     concurrency_limit -> { 4 }

3. Assign a real user to the source user and accept the assignment.
4. Accepting the reassignment begins the backend process to reassign all imported records to the real user. The exception added in step 2 should be thrown and cause the reassignment to fail. Allow the async worker to finish.
5. Set reassignment_error on the failed source user to anything in the console if !227775 hasn't been merged (optional)
6. Remove the manual failure in ReassignPlaceholderUserRecordsWorker and/or ReassignPlaceholderUserRecordsService
7. Initialize and execute Import::SourceUsers::RetryFailedReassignmentService with the failed source user and an owner on the source user's namespace
8. Verify the source user is returned in the response and it's in reassignment_in_progress (2) status
9. Verify the retried source user eventually completes and its user contributions are reassigned

Additional validations on the service:

• Only owners of the top-level namespace can retry failed source users. I.e. current_user must be able to admin_import_source_user, which is enabled when they can admin_namespace on the source user's namespace
• The source user must be failed, no other statuses are allowed to retry

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #589777

Sam Word (24cf019b) at 18 Mar 18:24

... and 2 more commits

• How we calculate strict success vs failure/timeout

FWIW, 3rd party project imports and project file imports don't have the exact same statuses as direct transfer. If I remember correctly, all project imports use ProjectImportState to track import status and last error. That would mean project import statuses don't line up exactly with direct transfer, and might impact whether we can include DT and project imports in the same visualizations:

I added this and the rollout issue for offline_transfer_exports to the beta epic: gitlab-org#21009 👍

Hey @reza-marandi Would you mind doing the initial backend review of this MR? Thanks! 🙇

Development update: I implemented nearly everything locally before realizing it was way too big to put into one MR. I'm breaking it up into smaller MRs instead. That also gives us an excuse to use a WIP/derisk feature flag that we can use in case my guess at rate limits is way off

1. Persist reassignment error to source users on f... (!227775)
2. MR add a backend service to retry failed placeholder reassignments without GraphQL mutation that calls it or internal tracking event - !227954, can be merged independently from !227775
3. MR to add internal tracking event similar to other placeholder state internal events like cancel_placeholder_user_reassignment
4. MR to implement GraphQL mutation: DRAFT: GraphQL mutation to retry failed reassig... (!227978), blocked by service MR

What does this MR do and why?

Previously, when a source user failed reassignment, the failure's exception message was logged to importer.log but not persisted to the reassignment_error column on the source user record, even though the column exists and is already on ImportSourceUser GraphQL objects. This MR updates Import::ReassignPlaceholderUserRecordsWorker to truncate and save the failure exception so that it can be returned when querying for source users in the API. This change will support implementing retrying failed placeholder reassignments.

References

• Placeholder user mapping docs: https://docs.gitlab.com/user/import/mapping/#bypass-confirmation-when-reassigning-placeholder-users
• Related to #589777

Screenshots or screen recordings

The UI was technically already displaying the error message on the placeholder user table, but it was never visible because we never persisted it:

How to set up and validate locally

1. Find or create an import source user (placeholder user) by running an import from any of the supported importers. Do not import a project into a personal namespace because it will assign all contributions directly to the personal namespace's user.
2. Once the import finishes, update Import::ReassignPlaceholderUserRecordsWorker to raise an error. Otherwise, you're unlikely to get the reassignment to fail. Something like:

diff --git a/app/workers/import/reassign_placeholder_user_records_worker.rb b/app/workers/import/reassign_placeholder_user_records_worker.rb
index 0004fbc1c95e24..f6e64c64861b32 100644
--- a/app/workers/import/reassign_placeholder_user_records_worker.rb
+++ b/app/workers/import/reassign_placeholder_user_records_worker.rb
@@ -11,7 +11,7 @@ class ReassignPlaceholderUserRecordsWorker
     data_consistency :sticky
     feature_category :importers
     deduplicate :until_executed
-    sidekiq_options retry: 5, dead: false
+    sidekiq_options retry: 1, dead: false
     sidekiq_options max_retries_after_interruption: 20
     concurrency_limit -> { 4 }
 
@@ -20,6 +20,8 @@ class ReassignPlaceholderUserRecordsWorker
     end
 
     def perform(import_source_user_id, params = {})
+      raise StandardError, 'PG::QueryCanceled: ERROR: canceling statement due to statement timeout'
+
       @import_source_user = Import::SourceUser.find_by_id(import_source_user_id)
       return unless import_source_user_valid?

3. Navigate to the import's top-level group's membership page, then to the "placeholders" tab, and assign a placeholder user to a real user. Then, accept the assignment (this can be done by impersonating the real assigned user, opening the reassignment notification email, and accepting).
4. Accepting the reassignment begins the backend process to reassign all imported records to the real user. The exception added in step 2 should be thrown and cause the reassignment to fail. Allow the async worker to finish.
5. Using GraphQL, query for the ImportSourceUser that just failed. Validate reassignmentError matches the message thrown in step 2.
6. Verify that the reassignment error is also now visible in the UI

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #589777

Sam Word (4143c72b) at 17 Mar 20:23