new version 6.9 "Two step Merge Request" brings 502 troubles

mentioned in issue #294 (closed)

I've experienced the same issue. We've got a ~400MB repo and some MRs were timing out (mostly when the diff would usually be considered too big to display by the MR screen). I raised the unicorn timeout from 30sec to 5 minutes temporarily to allow us to submit the MRs and we saw the full diff (which was huge).

I'd expect the workflow to decide that the diff is too big to view rather than attempting to show it always.

I'm guessing the issue lies somewhere below this line - https://gitlab.com/gitlab-org/gitlab-ce/blob/536373ad05b55c69442e7d7d6cb549791031cac2/app/views/projects/merge_requests/_new_submit.html.haml#L56

mentioned in issue #294 (closed)

Diffs are coming from https://gitlab.com/gitlab-org/gitlab-ce/blob/536373ad05b55c69442e7d7d6cb549791031cac2/app/controllers/projects/merge_requests_controller.rb#L81

Edit: Maybe we need to check for a timeout as in this case? https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/views/projects/compare/show.html.haml#L28

@dblessing the timeout might be the key; have a look at https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/controllers/projects/compare_controller.rb also.

Any progress on this? Any scheduled fixes?

@nick.huanca We're looking into this. But we don't have a fix yet and the creation of one might not be completed before the code freeze of 7.0.

One interesting thing we noted:

sets a timeout: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/controllers/projects/compare_controller.rb#L18
doesn't set a timeout: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/controllers/projects/merge_requests_controller.rb#L60

We are rating this as non-critical for 7.0 since you can work around it by merging from the command line. We're working very hard on features that must ship in 7.0 so we are lacking capacity to look into this. Any help is appreciated.

I will explore the timeout theory either tonight or tomorrow night. @nick.huanca, @mhanson and @liuyang1204 - If I come up with a potential fix could any of you help test it? I'm not confident I'll be able to reproduce the issue and validate the fix.

Thanks @dblessing

I made some fix to MR creation. Not sure if it fixes the issue. Can anyone try it? Commit 98416ec9

@dblessing you may want to look at comment before

Unfortunately I'm not able to test - we had to roll back to the previous release as the time needed to render the diffs once we ramped up the unicorn timeout was unacceptable. It should be easily reproducible on any repository with a large number of files by pushing a branch with every file deleted then attempting to merge it (as that would cause a huge diff).

@dblessing @dzaporozhets Thanks for your help, but same with Matt, I'm not able to test, neither. My Gitlab is under compact production use. I already got complaints from devs in my team because of the last upgrading & downgrading, we lost all comments of that whole day. It is risky for me to test it again.

Same with Matt, again, I think it is easily reproducible.

ok. thanks for explanation

I found a problem and push fix soon

@dzaporozhets I will look for the changes you're going to push and try to test this issue today.

I noticed another issue with this step of MRs yesterday where all file changes were shown in the list but not all the diffs were shown. Do any of these changes you pushed fix that, that you know of? If you're not sure, I will open a new issue to track that.

Now it should be fixed by 5fdcaadf

Status changed to closed

Are you planning to include this patch into 7.0? I have 6.9-stable version, how I can to fix it? Can I deploy only this single commit?

@olga.plisko This will go into 7.0. I'm not sure if you can deploy this single commit.

@olga.plisko it seems that applying the commit via cherry-pick works.

> sudo su git
> cd /home/git/gitlab
> git fetch origin
> git cherry-pick 59e1819561912cb38bf0
[6-9-stable e46b644] Apply soft diff limits to MergeRequest#new page
 Author: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
 2 files changed, 6 insertions(+), 2 deletions(-)

Then restart your gitlab service.

Status changed to reopened

I upgraded to 7.0 but this problem seems still existing.

E, [2014-07-14T11:51:23.020361 #14487] ERROR -- : worker=0 PID:8400 timeout (31s > 30s), killing
E, [2014-07-14T11:51:23.028017 #14487] ERROR -- : reaped #<Process::Status: pid 8400 SIGKILL (signal 9)> worker=0
I, [2014-07-14T11:51:23.031062 #8492]  INFO -- : worker=0 ready
E, [2014-07-14T11:59:36.440271 #14487] ERROR -- : worker=0 PID:8492 timeout (31s > 30s), killing
E, [2014-07-14T11:59:36.453003 #14487] ERROR -- : reaped #<Process::Status: pid 8492 SIGKILL (signal 9)> worker=0
I, [2014-07-14T11:59:36.457319 #11047]  INFO -- : worker=0 ready
E, [2014-07-14T12:02:27.590025 #14487] ERROR -- : worker=0 PID:11047 timeout (31s > 30s), killing

I guess:

app/controllers/projects/merge_requests_controller.rb L69

compare_action = Gitlab::Satellite::CompareAction.new(
        current_user,
        @merge_request.target_project,
        @merge_request.target_branch,
        @merge_request.source_project,
        @merge_request.source_branch
      )

or L89

@diffs = compare_action.diffs

one of them is the problem.

Additional information:

in log/githost.log

July 15, 2014 18:36 -> ERROR -> Grit::Git::GitTimeout

Found maybe related with this issue

I've tried to debug the new method of merge_requests_controller.rb and found two problems:

@commits = compare_action.commits fails after 30 seconds timeout
@diffs = compare_action.diffs fails after 30 seconds timeout

July 15, 2014 20:03: entering new 2014-07-15 20:03:47 +0800
July 15, 2014 20:03: entering new 2014-07-15 20:03:51 +0800
July 15, 2014 20:03: before compare action 2014-07-15 20:03:51 +0800
July 15, 2014 20:03: after compare action 2014-07-15 20:03:51 +0800
July 15, 2014 20:03: before get commits 2014-07-15 20:03:51 +0800
July 15, 2014 20:04: after get commits 2014-07-15 20:04:21 +0800
July 15, 2014 20:04: failing commits
July 15, 2014 20:04: before diff 2014-07-15 20:04:21 +0800
July 15, 2014 20:04: after diff 2014-07-15 20:04:51 +0800

But app/controllers/projects/compare_controller.rb works well and the "compare" feature works with no problem.

So finally I think the problem is located in compare_action.rb

@liuyang1204 can you try increasing this value https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/gitlab.yml.example#L229? It should prevent you from Grit::Git::GitTimeout errors. I have a commit with 4k changed files and increasing this config value to 15242880 helped

@dzaporozhets Mine is:

max_size: 52428800
timeout: 600

And the Grit::Git::GitTimeout still happening in 6.9 on big merge requests. Gunicorn also have 600 for timeout.

@ayr-ton As I mentioned on github, it seems the timeout value in the gitlab.yml config is not used when doing satellites (some or all, not sure) actions.

However, @dzaporozhets mentioned modifying the max_size and yours seems identical to the default value (well the default value before commit 3780444c which happened not long ago).

You could give a try. If it does not help, you could try this change I performed on my instance.

@matthieu.o.vachon It solved my problem :D Thanks.

What I did for 7.1:

Increased max_size default value from 5mb to 20mb
Catch timeout in satellite and show proper error message:

However if you use GitLab with huge merge requests(ex. 10k changed files) and big binary files(maybe HD movies 😄 ) you may need to play with config and unicorn/satellite timeouts.

@dzaporozhets That's a good improvement already, thanks 😄!. May I ask that the timeout defined in gitlab.yml overrides the hardcoded timeout (in action.rb) of satellite actions? That would help people with large repositories (or slow hard disk) so they can actually tweak the git timeout of satellite actions.

Hi, @dzaporozhets Thanks for help. I tried to increase max_size and timeout but my problem is still not solved.

FYI, I found this error not only happens when we have large diffs, but also small diffs. The key is "large repo". I've tried to make a MR with only less than 10 lines change, this error happens, either.

And one thing I feel strange is that "compare" feature response smoothly while "MR compare" seems very slow.

I've tried out a fix on my problem, change the new method app/controllers/projects/merge_requests_controller.rb (sorry that I may not be able to push and make merge request, so I just paste my change)

def new
    @merge_request = MergeRequest.new(params[:merge_request])
    @merge_request.source_project = @project unless @merge_request.source_project
    @merge_request.target_project ||= (@project.forked_from_project || @project)
    @target_branches = @merge_request.target_project.nil? ? [] : @merge_request.target_project.repository.branch_names
    @merge_request.target_branch ||= @merge_request.target_project.default_branch
    @source_project = @merge_request.source_project

    if @merge_request.target_branch && @merge_request.source_branch
        if @merge_request.target_project == @merge_request.source_project
            @compare_failed = false
            compare = Gitlab::Git::Compare.new(@merge_request.target_project.repository.raw_repository, @merge_request.target_branch, @merge_request.source_branch, MergeRequestDiff::COMMITS_SAFE_SIZE)
            @commits = compare.commits

            if @commits
              @commits.map! { |commit| Commit.new(commit) }
              @commit = @commits.first
            else
              # false value because failed to get commits from satellite
              @commits = []
              @compare_failed = true
            end

            @note_counts = Note.where(commit_id: @commits.map(&:id)).
                group(:commit_id).count

            begin
              @diffs = compare.diffs
              @merge_request.title = @merge_request.source_branch.titleize.humanize
              @target_project = @merge_request.target_project
              @target_repo = @target_project.repository

              diff_line_count = Commit::diff_line_count(@diffs)
              @suppress_diff = Commit::diff_suppress?(@diffs, diff_line_count)
              @force_suppress_diff = @suppress_diff
            rescue Gitlab::Satellite::BranchesWithoutParent
              @error = "Selected branches have no common commit so they cannot be merged."
            end
        else
            compare_action = Gitlab::Satellite::CompareAction.new(
              current_user,
              @merge_request.target_project,
              @merge_request.target_branch,
              @merge_request.source_project,
              @merge_request.source_branch
            )

            @compare_failed = false
            @commits = compare_action.commits

            if @commits
              @commits.map! { |commit| Commit.new(commit) }
              @commit = @commits.first
            else
              # false value because failed to get commits from satellite
              @commits = []
              @compare_failed = true
            end

            @note_counts = Note.where(commit_id: @commits.map(&:id)).
                group(:commit_id).count

            begin
              @diffs = compare_action.diffs
              @merge_request.title = @merge_request.source_branch.titleize.humanize
              @target_project = @merge_request.target_project
              @target_repo = @target_project.repository

              diff_line_count = Commit::diff_line_count(@diffs)
              @suppress_diff = Commit::diff_suppress?(@diffs, diff_line_count)
              @force_suppress_diff = @suppress_diff
            rescue Gitlab::Satellite::BranchesWithoutParent
              @error = "Selected branches have no common commit so they cannot be merged."
            end
        end
    end
end

The point is I found Gitlab::Git::Compare is much faster than Gitlab::Satellite::CompareAction. And I do not understand why we have two implementation of the similar "compare" function. I know one difference is that Gitlab::Git::Compare can not do comparison between different projects.

Well, this is only a temporary fix since it still can not solve the problem when use want to make MR between different projects in large repo

Is anyone able to reproduce this on 7.1?

@sytses @liuyang1204

The https://github.com/gitlabhq/gitlabhq/pull/7316 should help fixing this issue. Put a high value to gunicorn timeout and a high value to satellites timeout under gitlab.yml. It should fix the issue.

Wow excellent @ayr-ton, thanks for working on this.

@matthieu.o.vachon I basically used your logic to change satellites timeout, but with some changes to fetch this from gitlab.yml as you propose. And also, with unit tests (:

Thank you for the idea and thanks for @dzaporozhets for the patience in the reviewing process. Also, thank you for the suggestions in https://gitlab.com/gitlab-org/gitlab-ce/issues/295#note_160785. That fixed my first problem.

@ayr-ton Thanks! Your work should be able to solve some problems, but I think this issue is still not completely solved.

As I mentioned, the problem is that the comparison itself is very slow. I've already tried to increase the timeout in lib/gitlab/satellite/action.rb, from 30 to 300. But the result was that instead of getting error after waiting 30s, I got error after 5min. And the comparison only contains less than 10 lines diff, actually.

After I applied my fix https://gitlab.com/gitlab-org/gitlab-ce/issues/295#note_161316 The comparison becomes to cost less than 2s, IT SHOULD BE!!.

So I think the problem is the large performance difference between Gitlab::Satellite::CompareAction and Gitlab::Git::Compare.

I have tried applying @liuyang1204's patch on 7.1 and am still having this issue.

https://github.com/gitlabhq/gitlabhq/pull/7316 does nothing on this issue

on 7.1.0 i had to also set max_size to 20971520 in config/gitlab.yml before restarting gitlab, now it works

I can confirm this issue is still in Gitlab 7.1, with exactly the same symptoms as Liu Yang identified (large repository, any size commit).

Also couldn't get the code in Liu Yang's post to work with Gitlab 7.1. Possibly because we have different versions of Rails, I had to add a line and change the assignment of @merge_request:

params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
@merge_request = MergeRequest.new(merge_request_params)

Even doing that I still get an error that the method Commit::diff_suppress does not exist (which grepping through the code would seem to be correct):

Completed 500 Internal Server Error in 645ms

NoMethodError (undefined method `diff_suppress?' for Commit:Class):
  app/controllers/projects/merge_requests_controller.rb:94:in `new'

Am I doing something wrong, or does code to fix this need to be updated for Gitlab 7.1/Rails 4?

Earlier @liuyang1204 pointed out that the compare view /group/project/compare and the 'new merge request' diff /group/project/merge_requests/new use different code to get the diff. Starting with 2ef45507 we are trying to share more code between the compare view and the 'new merge request' diff in 7.2.

Could anybody check (in a staging environment) if the current master behaves better? What I would hope for to happen is that the max_size and timeout settings in config/gitlab.yml start to have more of an impact on the 'new merge request' diff.

This is happening in 7.2 for us. I will be attempting to do the fixes shown above.

None of the suggested fixes here are working for me.

This is ocuring on 7.4.3 for me as well. I have a 11mb repository which causes the ruby process to run out of memory upon creating a merge request. Using the latest scientific methods for profiling this (top), I am able to observe the ruby vm increasing memory allocation until it runs out of memory, then unicorn pops smoke and reaps the process and 502's the user creating the MR.

@jared-m-davies Please add enough swap, see https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/install/requirements.md

We seem to have the same issue (or at least a similar issue) on 7.9.0-ee.

I managed to get the merge request going after 3 or 4 tries, but before that i would just get a 502 error.

We've seen this problem while trying to create a MR on a 21MB repo. The server has 8GB RAM, with 1GB free at the moment.

This should be fixed in the current version of gitlab so I'll close this.
If you're still experiencing this issue, please feel free to reopen it.

Status changed to closed

This is still happening with 8.4.

I hit this issue while submitting a merge request with about 400 files, but nothing really big. I've tried to raise the timeout option, but the Unicorn worker is still running at 100% CPU. This doesn't seem to be a memory issue (less than 25% RAM, 0% swap).

@warren.seine Please create a new issue and provide details, including log files. 8.x releases no longer have satellites so although the issue appears to be the same, it may not be related at all. You can mention this issue in your new issue so we can draw comparisons.

new version 6.9 "Two step Merge Request" brings 502 troubles

Designs

Child items 0

Activity

Admin message

Admin message

new version 6.9 "Two step Merge Request" brings 502 troubles

Activity