Use `push` web hook events for pull mirroring

What does this MR do?

Are there points in the code the reviewer needs to double check?

Why was this MR needed?

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

• Changelog entry added, if necessary
• Documentation created/updated
• API support added
• Tests added for this feature/bug
• Review
  • Has been reviewed by UX
  • Has been reviewed by Frontend
  • Has been reviewed by Backend
  • Has been reviewed by Database
• Conform by the merge request performance guides
• Conform by the style guides
• Squashed related commits together
• Internationalization required/considered
• If paid feature, have we considered GitLab.com plan and how it works for groups and is there a design for promoting it to users who aren't on the correct plan

What are the relevant issue numbers?

closes #3821 (closed)

added 1 commit

• f13796e2 - Adding endpoint

Compare with previous version

@DouweM I have several doubts regarding this endpoint:

• Should it be also in v3?
• Do you think the endpoint route is appropiate? (/projects/:id/mirror/update)
• Should this endpoint be available only if is configured by the mirrored project admin? (authorize_admin_project)

added 1 commit

• 5f1d451f - Added a couple of tests

Compare with previous version

• Should it be also in v3?

@fjsanpedro Nope, that just exists for backward compatibility. New endpoints should only go into v4.

• Do you think the endpoint route is appropiate? (/projects/:id/mirror/update)

Yep, sounds reasonable.

• Should this endpoint be available only if is configured by the mirrored project admin? (authorize_admin_project)

I think we should use the same rules we use for the current web endpoint. We already have a Update Mirror button in the UI, right?

added 1 commit

• a8fbcce6 - Removing admin privileges to trigger the event and removing from api v3

Compare with previous version

@DouweM should we add any time restriction to the trigger event? Right now, there is a 5 minutes restriction if you do it through the UI.

added 1 commit

• 4352cba9 - Added authorization

Compare with previous version

changed title from WIP: Use push web hook events for pull mirroring to Use push web hook events for pull mirroring

@fjsanpedro Honestly, I think we can remove that both on the API and in the web interface. There is a small potential for someone messing with our scheduling by clicking that button every few seconds and constantly jumping the queue ahead of other mirrors that want to be updated, but that potential for abuse is not that large.

We should just make sure that when an update is already scheduled, we don't schedule one again!

Does that make sense?

@DouweM I agree with you about the last point but imagine this situation:

• There is a project scheduled for 16:00 and Time.now = 15:50
• There is an API call at 15:51. Until 16:00 the mirror won't update because it is already scheduled.

So, it depends on what we want to offer.

If we want a more precise solution for the former situation, the implementation becomes more difficult because we would have to do the following if the project is scheduled:

• we have to look for the scheduled worker and delete it.
• Transition the project import state to finished or whatever
• we schedule again the project

If the project import has started we can avoid triggering the event because once the process finishes it updates the next_execution_timestamp.

What do you think? Is it ok only with the basic implementation and offer a 'best-effort' solution? or do we go one step further be more 'accurate' with the imports?.

Sorry for all these questions but I want to make sure that I get it right.

By the way, I'd leave the UI time restriction because it is quite easy to mess the state machine that handles the import process, and I think we should add more restrictions if we remove it.

@fjsanpedro I'm liking the questions, don't worry :)

I'm not sure I follow though.

If a project has a next_execution_timestamp of 16:00, and it's 15:50 now, it wouldn't be in state scheduled or started, right? It would be finished or failed. So hitting the endpoint (or the button) should call force_import_job!, which will set next_execution_timestamp = Time.now, and call UpdateAllMirrorsWorker.perform_async, so that it immediately picks up the project and schedules its update job. So if the API call happens at 15:51, the job will be scheduled at 15:51, and will probably run at 15:51 or 15:52.

By the way, I'd leave the UI time restriction because it is quite easy to mess the state machine that handles the import process, and I think we should add more restrictions if we remove it.

@fjsanpedro If we don't have an API time restriction, wouldn't we have the same problem with potentially messing up the state machine? How do you mean the state machine can get messed up anyway?

If a project has a next_execution_timestamp of 16:00, and it's 15:50 now, it wouldn't be in state scheduled or started, right? It would be >finished or failed. So hitting the endpoint (or the button) should call force_import_job!, which will set next_execution_timestamp = Time.now, and >call UpdateAllMirrorsWorker.perform_async, so that it immediately picks up the project and schedules its update job. So if the API call happens at >15:51, the job will be scheduled at 15:51, and will probably run at 15:51 or 15:52.

Oh, you're right, I thought the UpdateAllMirrorsWorker enqueues workers in the future and transition the project into scheduled, but it seems it only performs the import once Time.now > next_execution_timestamp. Cool!

By the way, I'd leave the UI time restriction because it is quite easy to mess the state machine that handles the import process, and I think we should add more restrictions if we remove it.

@fjsanpedro If we don't have an API time restriction, wouldn't we have the same problem with potentially messing up the state machine? How do you mean the >state machine can get messed up anyway?

I think I was mistaken about this. I was playing with the console and the api and I reach a point where the project was the scheduled but the worker was not present. But I think it was something related to sigkill the workers process. I think there is nothing to worry about.

added 1 commit

• 45948f5e - Added guard clause to prevent triggering the event in scheduled or started state

Compare with previous version

added 1 commit

• d67a8be2 - Renamed endpoint name

Compare with previous version

added 1 commit

• f0c1e4ac - Updated API doc

Compare with previous version

added 1 commit

• 26e798da - Added changelog

Compare with previous version

marked the checklist item Changelog entry added, if necessary as completed

marked the checklist item Documentation created/updated as completed