Skip to content
Snippets Groups Projects

Add feature flag to allow maintainers to create MR pipelines async

Merged Dylan Griffith requested to merge 463355-async-pipeline-creation-for-merge-requests into master

What does this MR do and why?

This is related to #463355 (closed) as this endpoint often times out. Allowing it to be created asynchronously will mean that we can create pipelines that take longer than 60s to create.

From my local testing this change has minimal UX impact on the merge requests pipeline page. That is because we are already polling for new pipelines constantly so having it created async shouldn't really matter to the user. I even tested the scenario where the pipeline config is broken and both provide a similar UX before and after. The UX differences are:

  1. The spinner stops immediately because the async request returns immediately and the user has no visual indicator that it's still being created async
  2. You might have to wait slightly longer for the polling interval to see the new pipeline when you create it async
  3. If the pipeline has validation errors then the user won't get that feedback at all for async creation

Follow up UX improvements

I would like to introduce the UX improvements in a follow up issue #476627 (closed) because:

  1. It's behind a user scoped feature flag and I don't intend to roll this out to customers yet
  2. This provides immediate value to GitLab maintainers that understand the UX tradeoff because at present we often can't even create pipelines. Personally I see over 60% of my pipeline creation fails and I can't do my job
  3. Making GitLab maintainers more productive ASAP means we'll be able to improve the product faster and fix the underlying causes (e.g. performance) faster
  4. The async polling for the specific pipeline failure is quite a bit more complicated to implement as we need to store the information about recent pipeline attempts somewhere

There are more ideas in #463355 (closed) about how we might like to stream/poll from some endpoint to know if the specific pipeline I just tried to create failed or not. From a UX perspective this only seems relevant to timeouts today as it seems these are the ones where a user gets a flash notice of the failure. But the async process basically makes the timeouts go away so it is less relevant. In any case this is behind a feature flag and it will make me and other GitLab maintainers much more productive right now as we often cannot even start a pipeline and we're wasting a lot of resources trying and timing out constantly.

In future, if we need, we can update this API to return an identifier related to the async pipeline you created and then we can add a new endpoint to see whether or not your request succeeded. This could be done by streaming or polling and we could either use ActionCable subscriptions to notify the clients or we could store these recent jobs in redis with a short timeout if that's simpler than a whole new GraphQL endpoint.

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.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After
With good pipeline before after
With broken pipeline config before-broken-pipeline after-broken-pipeline

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

Related to #463355 (closed)

Edited by Dylan Griffith

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Dylan Griffith changed the description

    changed the description

  • requested review from @allison.browne

  • added 1 commit

    • a62deb78 - Add frontend for async MR pipeline creation with feature flag

    Compare with previous version

  • added 1 commit

    • 51bb2add - Add frontend for async MR pipeline creation with feature flag

    Compare with previous version

  • Dylan Griffith resolved all threads

    resolved all threads

  • Dylan Griffith requested review from @aalakkad

    requested review from @aalakkad

  • Dylan Griffith marked this merge request as ready

    marked this merge request as ready

  • added 1 commit

    • 53795d72 - Add frontend for async MR pipeline creation with feature flag

    Compare with previous version

  • Ghost User
  • mentioned in issue #463355 (closed)

  • Dylan Griffith resolved all threads

    resolved all threads

  • Dylan Griffith added 2 commits

    added 2 commits

    • 9762a4a1 - Add async option to merge request pipeline create API
    • c2f520a0 - Add frontend for async MR pipeline creation with feature flag

    Compare with previous version

  • Ammar Alakkad
  • Ammar Alakkad approved this merge request

    approved this merge request

  • @DylanGriffith I left 2 non-blocking suggestions for your consideration, I'll let you have a look and assign the maintainer whenever you see fit. Thanks!

  • Ammar Alakkad removed review request for @aalakkad

    removed review request for @aalakkad

  • added pipelinetier-2 label and removed pipelinetier-1 label

  • Before you set this MR to auto-merge

    This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3. We will trigger a new pipeline for each transition to a higher tier.

    Before you set this MR to auto-merge, please check the following:

    • You are the last maintainer of this merge request
    • The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
    • This pipeline is recent enough (created in the last 8 hours)

    If all the criteria above apply, please set auto-merge for this merge request.

    See pipeline tiers and merging a merge request for more details.

  • E2E Test Result Summary

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for 629ade10

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Verify      | 83     | 0      | 4       | 0     | 87    | ✅     |
    | Create      | 127    | 0      | 12      | 0     | 139   | ✅     |
    | Data Stores | 31     | 0      | 1       | 0     | 32    | ✅     |
    | Plan        | 70     | 0      | 0       | 0     | 70    | ✅     |
    | Govern      | 71     | 0      | 0       | 0     | 71    | ✅     |
    | Package     | 16     | 0      | 15      | 0     | 31    | ✅     |
    | Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Release     | 5      | 0      | 0       | 0     | 5     | ✅     |
    | Secure      | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Manage      | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Fulfillment | 1      | 0      | 0       | 0     | 1     | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 416    | 0      | 33      | 0     | 449   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :white_check_mark: test report for 629ade10

    expand test summary
    +-------------------------------------------------------------+
    |                       suites summary                        |
    +--------+--------+--------+---------+-------+-------+--------+
    |        | passed | failed | skipped | flaky | total | result |
    +--------+--------+--------+---------+-------+-------+--------+
    | Verify | 102    | 0      | 30      | 6     | 132   | ✅     |
    +--------+--------+--------+---------+-------+-------+--------+
    | Total  | 102    | 0      | 30      | 6     | 132   | ✅     |
    +--------+--------+--------+---------+-------+-------+--------+
  • Hordur Freyr Yngvason
  • Allison Browne
  • @DylanGriffith, I like this approach but my main concern is that there are a few UX changes like with the loading spinner and alerts for non-persisted pipelines.

  • requested review from @allison.browne

    • Resolved by Sunjung Park

      Over to @kushalpandya for frontend maintainer review since the first frontend review suggestions were non-blocking and I've applied some changes.

      Back to @allison.browne for backend review. I've applied your suggestions.

      @sunjungp could you please review for UX. I've noted in the MR description the UX tradeoffs here and also the fact that this is behind a feature flag and for now I want to unblock GitLab maintainers that can't run pipelines but hopefully we can introduce these more complicated UX improvements as a followup but let me know if you have any concerns with that plan.

  • requested review from @sunjungp and @kushalpandya

  • mentioned in issue #476627 (closed)

  • Dylan Griffith changed the description

    changed the description

  • Dylan Griffith resolved all threads

    resolved all threads

  • Dylan Griffith added 2 commits

    added 2 commits

    • 1e568505 - Add async option to merge request pipeline create API
    • 629ade10 - Add frontend for async MR pipeline creation with feature flag

    Compare with previous version

  • Dylan Griffith changed title from Add async option to merge request pipeline create API to Add feature flag to allow maintainers to create MR pipelines async

    changed title from Add async option to merge request pipeline create API to Add feature flag to allow maintainers to create MR pipelines async

  • Allison Browne approved this merge request

    approved this merge request

  • added pipelinetier-3 label and removed pipelinetier-2 label

  • Allison Browne requested review from @jivanvl and @lauraXD

    requested review from @jivanvl and @lauraXD

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading