mentioned in issue #38621 (closed)

restored source branch github-importer-refactor

For this MR my goal is to get https://github.com/kubernetes/kubernetes to import in roughly a minute or so, at least when using the parallel importer (the Rake task will still be sequential). This however requires a near total rewrite of the importer as I couldn't make this work in the old code.

@yorickpeterse As far as I know Gitlab::GithubImport::Importer is only used for Gitea import, GitHub import now uses Github::Import.

added 1 commit

• f6d6e37e - Rewrite the GitHub importer from scratch

Compare with previous version

@rymai Yeah I renamed that one to LegacyGithubImport. I didn't rename GiteaImport it since it used Octokit and some other GitHub specific bits, but perhaps that's a better name.

I also just pushed a new version which adds the ability to import issues and what not properly. Pull requests and releases are not yet handled. Issues are also imported in parallel, which results in https://github.com/yorickpeterse/oga being imported in 15-20 seconds instead of 120 seconds (excluding PRs and releases).

added 1 commit

• 5c01cbc4 - Rewrite the GitHub importer from scratch

Compare with previous version

I spent a few more hours hacking on this and things are starting to come together. Labels and milestones are assigned properly, data for non existing users is handled, etc.

Along the way I did get hit by the GitHub rate limit when trying to import https://github.com/kubernetes/kubernetes. This isn't entirely surprising because this project has 4626 issues and we need at least an equal amount of requests to get the comments for these issues. In the current setup this means we may (in the worst case) have to wait up to an hour to get the data.

My plan for dealing with this is to break importing comments up into a separate worker as this will consume most requests. These jobs will then re-schedule themselves in the future when hitting the rate limit, based on when the rate limit will reset. This means importing Kubernetes can still take an hour or two because of this, but at least we won't have threads blocked in a sleep call.

changed the description

Potential option: there's an API endpoint for getting comments across a repository (e.g. https://api.github.com/repos/YorickPeterse/oga/issues/comments). While the returned comment payloads don't contain a direct association to their "parent" (issue or PR), we could parse this from the issue_url field. Assuming this were to work it means we'd only need 4626 / 100 (the maximum number of objects per page) = 47 requests.

Yeah I renamed that one to LegacyGithubImport. I didn't rename GiteaImport it since it used Octokit and some other GitHub specific bits, but perhaps that's a better name.

Oh, ok I see: you've rewritten Gitlab::GithubImport::Importer (the old importer) and kept the new Github::Import (which won't be used anymore after this refactor). That was what confused me! :P

My plan for dealing with this is to break importing comments up into a separate worker as this will consume most requests. These jobs will then re-schedule themselves in the future when hitting the rate limit, based on when the rate limit will reset. This means importing Kubernetes can still take an hour or two because of this, but at least we won't have threads blocked in a sleep call.

Sounds good!

Potential option: there's an API endpoint for getting comments across a repository (e.g. https://api.github.com/repos/YorickPeterse/oga/issues/comments). While the returned comment payloads don't contain a direct association to their "parent" (issue or PR), we could parse this from the issue_url field. Assuming this were to work it means we'd only need 4626 / 100 (the maximum number of objects per page) = 47 requests.

@yorickpeterse Yes, that's what I discovered and proposed too: #38200 :)

Why not keep milestone.created_at/milestone.updated_at?

changed this line in version 8 of the diff

mentioned in issue #38873 (closed)

@yorickpeterse Man that's fast, local benchmarks when importing guard/guard (531 issues):

• master: 1228 seconds (~20 minutes) 🐌
• this branch: 74 seconds (1 minutes, 14 seconds) 🚀

added 1 commit

• adc9ed0a - Rewrite the GitHub importer from scratch

Compare with previous version

marked the task Import issue comments using a worker (1 job per comment) as completed

marked the task Use a separate class for importing issue comments as completed

Importing comments is now done using the "comments per repo" endpoint (https://developer.github.com/v3/issues/comments/#list-comments-in-a-repository), saving a lot of API calls.

Pull requests will be a bit tricky. In the old approach we would restore some branches and (as far as I can tell) would generate diffs using our Git clone. This means we could hit NFS pretty hard if we were to run e.g. 100 PR importer jobs in parallel. Unfortunately the GitHub API does not appear to provide a single API endpoint to get the diffs in a merge request. I'll see what we can do about this.

Pull requests will be a bit tricky. In the old approach we would restore some branches and (as far as I can tell) would generate diffs using our Git clone. This means we could hit NFS pretty hard if we were to run e.g. 100 PR importer jobs in parallel. Unfortunately the GitHub API does not appear to provide a single API endpoint to get the diffs in a merge request. I'll see what we can do about this.

@yorickpeterse If you take a look at the new importer Github::Import, we're not restoring branches anymore. :)

@rymai Aha, that explains why I couldn't find any calls to restore_branch! and the likes any more; I could have sworn they were there, but them being refactored away explains why I couldn't find them. What is the commit (or commits) that introduced this change? That might make it easier for me to understand how to best port that logic over to this parallel importer.

Ah I found it, those calls were removed in 43b75b38. I'll take a look at that and the following commits.

Aha, that explains why I couldn't find any calls to restore_branch! and the likes any more; I could have sworn they were there, but them being refactored away explains why I couldn't find them. What is the commit (or commits) that introduced this change? That might make it easier for me to understand how to best port that logic over to this parallel importer.

@yorickpeterse Yeah, the MR was !14445 (merged), the description of the MR and its commits can probably help you understand what was done and why! Also, feel free to checkout https://gitlab.com/gitlab-org/gitlab-ce/issues/36284 for more background and work on improving importing and performance in general.

I poked around with the GitHub GraphQL API and this looks quite promising. For example, it would allow us to get issue/PR authors/assignees including their Emails, removing the need for API calls to get that data. This is quite important as looking up e.g. 1000 users could in the worst case lead to 1000 GitHub API calls.

To give an example, here's a GraphQL query to get all issues of a repository along with the associated data we'd need:

query($owner: String!, $repo: String!, $after: String) {
  repository(owner: $owner, name: $repo) {
    issues(states: [OPEN, CLOSED], first: 100, after: $after, orderBy: { field: CREATED_AT, direction: ASC }) {
      nodes {
        number
        title
        body
        author {
          ... on User {
            id: databaseId
            login
            email
          }
        }
        assignees(first: 10) {
          nodes {
            ... on User {
              id: databaseId
              login
              email
            }
          }
        }
        milestone_iid: milestone {
          number
        }
        label_names: labels(first: 10) {
          nodes {
            name
          }
        }
      }
      pageInfo {
        has_next_page: hasNextPage
        end_cursor: endCursor
      }
    }
  }
  rate_limit: rateLimit {
    remaining
    reset_at: resetAt
  }
}

This query includes some info necessary for pagination as well as rate limiting details. The output will be something along the lines of:

{
  "data": {
    "repository": {
      "issues": {
        "nodes": [
          {
            "number": 1,
            "title": "Lexing doctypes",
            "body": "Both the lexer and the HTML parser should support the processing of doctypes. Currently both already have basic support for doctypes but this should be improved so that it's easier to extract information such as the doctype URL.\n",
            "author": {
              "id": 86065,
              "login": "YorickPeterse",
              "email": "yorickpeterse@gmail.com"
            },
            "assignees": {
              "nodes": []
            },
            "milestone_iid": null,
            "label_names": {
              "nodes": [
                {
                  "name": "HTML"
                },
                {
                  "name": "Lexer"
                }
              ]
            }
          },
          ...
        ],
        "pageInfo": {
          "has_next_page": true,
          "end_cursor": "Y3Vyc29yOnYyOpK5MjAxNC0wMi0yOFQxMTozODo1NCswMTowMM4Bsr8v"
        }
      }
    },
    "rate_limit": {
      "remaining": 4966,
      "reset_at": "2017-10-10T15:09:30.000Z"
    }
  }
}

There's a Ruby client written by GitHub here: https://github.com/github/graphql-client. It seems to be able to send calls over HTTP out of the box, though the code seems to be more of a proof of concept: https://github.com/github/graphql-client/blob/master/lib/graphql/client/http.rb

And here's a similar query for getting pull requests, including labels and what not:

query($owner: String!, $repo: String!, $after: String) {
  repository(owner: $owner, name: $repo) {
    pullRequests(states: [OPEN, CLOSED, MERGED], first: 100, after: $after, orderBy: { field: CREATED_AT, direction: ASC }) {
      nodes {
        number
        title
        body
        author {
          ... on User {
            id: databaseId
            login
            email
          }
        }
        assignees(first: 10) {
          nodes {
            ... on User {
              id: databaseId
              login
              email
            }
          }
        }
        milestone_iid: milestone {
          number
        }
        label_names: labels(first: 10) {
          nodes {
            name
          }
        }
      }
      pageInfo {
        has_next_page: hasNextPage
        end_cursor: endCursor
      }
    }
  }
  rate_limit: rateLimit {
    remaining
    reset_at: resetAt
  }
}

One minor nuisance in the GraphQL API is that paginating nested objects (e.g. labels) will be annoying. However I think we could just say we'll only import up to 10 assignees and labels (or something along those lines) per issue/pull request. There shouldn't be that many cases (if any) where an issue/PR has more than 10 labels.

Annoyingly the GraphQL API does not support a way of getting comments across an entire repository, instead you can only get them per issue/PR/etc. This means we'd need to either embed those in the above queries and then paginate them, which I can see being quite annoying. This is annoying because we'd have to run many API calls per issue/PR to get the data, so it might be more efficient to just use the REST API.

So pros/cons of GraphQL:

Pros:

• Fewer calls necessary to get data such as labels of issues and PRS
• No additional calls necessary to get the Email address of users
• Query language is decent

Cons:

• Need to implement pagination yourself using GraphQL cursors
• Need API calls to traverse comment pages for every issue/PR
• Very new
• I'd have to rewrite most of the code I wrote thus far

I'll stick with REST for now, unless I really start hating it.

added 2 commits

• 17f487cf - Refactor User.find_by_any_email
• ada8dff2 - Rewrite the GitHub importer from scratch

Compare with previous version

added 1 commit

• 269085aa - Rewrite the GitHub importer from scratch

Compare with previous version

added 1 commit

• e409bfcd - Rewrite the GitHub importer from scratch

Compare with previous version

added 1 commit

• 55a5e4f5 - Rewrite the GitHub importer from scratch

Compare with previous version

@rymai @dbalexandre I noticed that creating notes will result in a call to Repository#keep_around to keep the commits. I'm wondering if this is necessary when importing? Getting rid of this would save us a lot of Git/NFS operations when importing large projects.

To add to that: the number of Gitaly calls that I see flying past in my console when importing PRs and diff notes is quite worrying. I have doubts about our setup being able to handle large imports if we keep hammering Git for every MR and note.

added 1 commit

• 05437693 - Rewrite the GitHub importer from scratch

Compare with previous version

mentioned in issue gitlab-com/infrastructure#2922 (closed)

mentioned in issue gitlab-com/infrastructure#2983 (closed)

added 1 commit

• 15b3f15d - Rewrite the GitHub importer from scratch

Compare with previous version

marked the task Import releases as completed

added 1 commit

• 9194783e - Rewrite the GitHub importer from scratch

Compare with previous version

added 1 commit

• 7d2368c0 - Rewrite the GitHub importer from scratch

Compare with previous version

added 1 commit

• d228e5e0 - Rewrite the GitHub importer from scratch

Compare with previous version

marked the task Make the parallel importing schedule a job to check for progress, instead of blocking the thread in a sleep call as completed

marked the task Issue and comment workers should reschedule themselves in the future if we hit a rate limit, the reschedule time will simply be the reset time of the rate limit (so if our rate limit resets in 10 seconds that means we schedule jobs for 10 seconds in the future) as completed

marked the task Make sure any stuck importer jobs don't mess with a running GitHub import as completed

I managed to break up the importer into a pipeline consisting out of the following stages:

1. ImportRepository: this will import base data such as the repository, wiki, labels, milestones, and releases (this data can be imported quickly enough in a single thread)
2. ImportPullRequests: this will import all pull requests from GitHub
3. ImportIssuesAndDiffNotes: this will create regular issues, enrich merge requests with labels, and create merge request diff notes
4. ImportNotes: this creates regular notes for both issues and merge requests, thus we have to schedule this after those have been imported
5. FinishImport: this will simply mark the job as finished and clear any caches

Between every stage we schedule a job for the AdvanceStageWorker. This worker will check if the work from the previous stage is done and if so it will advance the pipeline. If this isn't the case it will reschedule itself and check again later. Using this process we only very briefly block threads in sleep calls (when checking of jobs are completed in AdvanceStageWorker we wait for a little bit to reduce the number of reschedules), instead of doing so for hours on end.

A downside of this approach is that there's a minimum import time of around 1 minute or so, though this depends largely on how fast jobs can be completed before the AdvanceStageWorker reschedules itself. This however is a trade-off that's worth it as the alternative is blocking an entire thread (or multiple depending on the number of import jobs running) for a long period of time.

Using this setup I can import github.com/yorickpeterse/oga in about 100 seconds on my laptop, whereas it would take the old importer around 4.5 minutes.

API call wise importing Oga requires 91 API calls without user information being cached. With this data being cached we only need 11 API calls.

The code also takes care of refreshing the import JID so StuckImportJobsWorker doesn't mark the import process as failed after 15 minutes, though I have to still properly test this.

My next plan is to write automated tests for all this code (and probably fix a bunch of bugs in the process), then run some tests on larger projects such as Kubernetes.

added 1 commit

• b8adb73a - Rewrite the GitHub importer from scratch

Compare with previous version

added 1 commit

• 1dacd871 - Rewrite the GitHub importer from scratch

Compare with previous version

changed the description

added 1 commit

• 04e689fb - Rewrite the GitHub importer from scratch

Compare with previous version

added 493 commits

• 04e689fb...34fd07f6 - 491 commits from branch master
• 2f10fa3e - Refactor User.find_by_any_email
• 214b70a6 - Rewrite the GitHub importer from scratch

Compare with previous version

changed the description

added 1 commit

• 41a94cae - Rewrite the GitHub importer from scratch

Compare with previous version

marked the task Add tests for User.by_any_email as completed

resolved all discussions

added 1 commit

• 91c79675 - Rewrite the GitHub importer from scratch

Compare with previous version

added 2 commits

• e7cf1c7d - Refactor User.find_by_any_email
• 06462743 - Rewrite the GitHub importer from scratch

Compare with previous version

added 1 commit

• 8b4fe973 - Rewrite the GitHub importer from scratch

Compare with previous version

changed the description

Hi Yorick,

Thanks for this work. I've successfully migrated a GitHub repo, where the legacy importer failed to import the issues. Congrats for your work.

Btw, I've just "cherry-picked" the changes of the two commits (8b4fe973 and e7cf1c7d) and patched them on my omnibus installation 10.0.3 8895150 and that worked fine!

I've seen you've discussed the (re-)naming of the importer. I personally would suggest to leave the legacy importer un-renamed (and files not moved) and instead file the new code under e.g. github_import_ng (for next generation) or _v2.

Also, would it not be great to have all importers named the same, i.e. Gitlab::GithubImportNg::Importer rather than Github::Import?

I'm very new to this project and there might be good reasons for how you have chosen to name things.

Cheers, Martin.

mentioned in issue #4055

One observation I'd like to add:

After importing a GitHub repo with the patched version of the CE, I exported the same repo again in order to import it on gitlab.com. However, that import failed because some releases' description field were empty (""). Once I've corrected the project.json in the export by replacing $.releases[*].description with $.releases[*].tag when description === '', the import went through smoothly.

Now obviously, this is an issue of the ImportExport module, as it allows to export empty descriptions but does not import them again. However, it might be a follow-up error by the fact that this patch here does import empty descriptions where otherwise this would not be allowed?

See also #18235 Refactor GitHub importer

mentioned in issue #39133

Also see !13788 (closed)

One thing I have realised is, that labels are correctly generated. However, not assigned to MRs or issues.

changed milestone to 10.2

@martin.rueegg

I've seen you've discussed the (re-)naming of the importer. I personally would suggest to leave the legacy importer un-renamed (and files not moved) and instead file the new code under e.g. github_import_ng (for next generation) or _v2.

Including versions/generations in namespaces in my opinion is a bad idea, I'd much rather just have a "legacy" importer vs "the current" importer. It's worth mentioning that I may just rename LegacyGithubImporter to GiteaImporter since it's only used for Gitea.

Also, would it not be great to have all importers named the same, i.e. Gitlab::GithubImportNg::Importer rather than Github::Import?

This separation is the result of Sidekiq workers residing in app/workers and not lib/gitlab/X. Having said that I think creating directories such as app/workers/gitlab/github_import/ and placing the workers there should also work. I'll take a look at this.

Now obviously, this is an issue of the ImportExport module, as it allows to export empty descriptions but does not import them again. However, it might be a follow-up error by the fact that this patch here does import empty descriptions where otherwise this would not be allowed?

It seems the old importer would validate releases, skipping those that are invalid. This validation process requires a description to be present. Validations are currently bypassed since we insert releases in bulk, though we can quite easily validate data without having to initialize a Release object for every release.

One thing I have realised is, that labels are correctly generated. However, not assigned to MRs or issues.

They should be, at least this was working last time I tried the code in this MR locally. Perhaps I broke it while refactoring the code in recent pushes. I'm currently in the process of writing tests for all the code, hopefully that will fix any remaining bugs once I'm done.

marked the task Representation classes as completed

Regarding the milestone: I moved this to 10.2, at least for now. I initially hoped to be able to squeeze this into 10.1 behind a feature flag, but since the 22nd is this Sunday and the summit starts before that I don't think I'll make it.

Maybe I can make it, but the more likely alternative will be hot patching GitLab.com so we can test this, then ship it in 10.2.0.

@yorickpeterse,

Thanks for your response and all the explanations. Much appreciated.

Re release description: I would suggest to not skip the import of a given release when no description is given. Rather using the tag name as a description.

Re labels: I checked the code and I have not been able to identify the issue. However, they are not assigned (or not updated)

@martin.rueegg That link points to the old importer, not the one being added in this MR. The new code is in the LabelLinksImporter class in this MR. The old importer indeed had issues with not assigning labels properly.

added 1 commit

• 3d78cd50 - Rewrite the GitHub importer from scratch

Compare with previous version

changed the description

changed the description

@yorickpeterse, oh! :-D ok.

Well, then I have not run your importer! And I don't know if I can patch my 10.0.3 omnibus installation to test it ...

added 1 commit

• 15c268a8 - Rewrite the GitHub importer from scratch

Compare with previous version

marked the task Importer classes as completed

marked the task Set a release's description to the tag name if none is provided (since a description is required) as completed

added 1 commit

• e26f3eba - Rewrite the GitHub importer from scratch

Compare with previous version

All code now resides in the Gitlab::GithubImport namespace, instead of the Sidekiq code residing in GithubImporter.

@martin.rueegg

Well, then I have not run your importer! And I don't know if I can patch my 10.0.3 omnibus installation to test it ...

If you want to test things you'd have to apply the following patch using patch -p1 -N < file.patch: importer.patch

I haven't tested this with EE so it might not work there yet. You'll then have to muck around with the Rake task to get it to use the new importer, e.g. by applying this patch: rake.patch. You can then run the new importer using this:

env IMPORTER=parallel bundle exec rake import:github[token,gitlab-user,gitlab-user/gitlab-repo,github-user/github-repo]

For example:

env IMPORTER=parallel bundle exec rake import:github[hunter2,yorickpeterse,yorickpeterse/kubernetes-community,kubernetes/community]

For this to work you'll need to restart Sidekiq so it has workers available for the new queues. You can also use IMPORTER=sequential to use the new code in sequential mode, but this will be slower for large projects compared to IMPORTER=parallel.

marked the task Changes to Projects::ImportService to support async importers as completed

marked the task Changes to RepositoryImportWorker as completed

marked the task Sidekiq workers (GithubImporter namespace) as completed

marked the task Sidekiq worker concerns as completed

@smcgivern @DouweM This MR is finally reaching a point where all code is tested automatically, meaning it's time to document the changes, write a decent commit message, and most importantly of all: test it on GitLab.com somehow. In particular I want to test the impact of importing projects such as the one the Edge team are working on, how long it takes, if we need more monitoring, etc.

There are two approaches to doing this:

1. Include this code in 10.1 but hide it behind a feature flag (that's disabled by default) and don't announce it yet (or announce it as being in a beta state of sorts)
2. Don't include this in 10.1 and aim for 10.2, then hotpatch GitLab.com until we're done with testing

The first approach has the benefit of being the easiest to test as we don't need any patching. It also means we can start importing larger projects (if desired) without having to wait another month. There are also two downsides of this approach:

1. We're way past the merge window and the changes here are quite big (though a feature flag should prevent anything from breaking).
2. It will require reviewing before this Sunday, which can be difficult with the summit.

Option 2 doesn't suffer from these problems but instead suffers from the following:

1. Testing this code will be a pain because we'd have to hot-patch all Sidekiq workers, instantly affecting everybody that imports a GitHub project (unless we also hide the code behind a feature flag).
2. It will take at least a month before we can ship this to GitLab.com in a state that doesn't require us to run patches after every deploy.

As such I'd like to know what you think would be best here.

added 1 commit

• 590622b8 - Rewrite the GitHub importer from scratch

Compare with previous version

added 2 commits

• 3158030f - Rewrite the GitHub importer from scratch
• 73632809 - Enable the new GitHub importer with a feature flag

Compare with previous version

73632809 enables the new importer using a feature flag. It's not super pretty but it gives an idea of how we'd enable/test this until 10.2, if we were to include this in 10.1 that is.

unmarked as a Work In Progress

changed the description

@yorickpeterse thanks for laying out those options. I don't like the idea of merging and picking by Sunday. However, if it's behind a feature flag anyway, could we not just include it in a patch to 10.1? It is stretching the definition of 'patch' quite a lot, but if it has no impact without the feature flag, that seems like a reasonable compromise?

@yorickpeterse @smcgivern I like the compromise option!

added 321 commits

• 73632809...bd39b441 - 318 commits from branch master
• f3e238c2 - Refactor User.find_by_any_email
• 35538cd5 - Rewrite the GitHub importer from scratch
• eeb18a5b - Enable the new GitHub importer with a feature flag

Compare with previous version

@smcgivern That seems like a better option, that way we don't have to rush things out now.

Unrelated to this, it seems the build is failing due to static analysis tools failing on unrelated files. Not sure what's going on here.

@yorickpeterse hmm, that's weird. One of the errors seems slightly related, but the others are completely unrelated. Does bundle exec rake flay also fail locally?

added 2 commits

• f7a2fa17 - Rewrite the GitHub importer from scratch
• 0904a8ae - Enable the new GitHub importer with a feature flag

Compare with previous version

@smcgivern Locally it fails as well, but it seems to spit out similar output when I switch back to the master branch:

app/models/user.rb:41 :: parse error on value ":" (tCOLON)
skipping app/models/user.rb
app/models/concerns/awardable.rb:5 :: parse error on value nil (error)
skipping app/models/concerns/awardable.rb
lib/gitlab/git/storage/health.rb:29 :: parse error on value ["do", 29] (kDO_BLOCK)
skipping lib/gitlab/git/storage/health.rb
lib/gitlab/git/storage/forked_storage_check.rb:46 :: parse error on value "<<" (tLSHFT)
skipping lib/gitlab/git/storage/forked_storage_check.rb
lib/gitlab/background_migration/delete_conflicting_redirect_routes_range.rb:28 :: parse error on value "<<" (tLSHFT)
skipping lib/gitlab/background_migration/delete_conflicting_redirect_routes_range.rb
lib/gitlab/background_migration/create_fork_network_memberships_range.rb:28 :: parse error on value "FROM" (tCONSTANT)
skipping lib/gitlab/background_migration/create_fork_network_memberships_range.rb
lib/gitlab/background_migration/populate_fork_networks_range.rb:15 :: parse error on value "FROM" (tCONSTANT)
skipping lib/gitlab/background_migration/populate_fork_networks_range.rb
lib/gitlab/sql/glob.rb:9 :: parse error on value "<<" (tLSHFT)
skipping lib/gitlab/sql/glob.rb
lib/gitlab/exclusive_lease.rb:13 :: parse error on value "<<" (tLSHFT)
skipping lib/gitlab/exclusive_lease.rb

@yorickpeterse ah, it looks like that exits with zero, though. Our rake task is this:

desc 'Code duplication analyze via flay'
task :flay do
  output = `bundle exec flay --mass 35 app/ lib/gitlab/`

  if output.include? "Similar code found"
    puts output
    exit 1
  end
end

So I think that if we don't care about the similar code in the importers (haven't reviewed this), we could ignore that, or update the threshold. I have no idea why the untouched files also generate that message.

@smcgivern Ah ok. The similar code issue is something I'm tackling now so that should solve the failure.

added 2 commits

• 2a18b277 - Rewrite the GitHub importer from scratch
• 84c1fa49 - Enable the new GitHub importer with a feature flag

Compare with previous version

changed the description

added 2 commits

• da87fe65 - Rewrite the GitHub importer from scratch
• a64edfb4 - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 75428ea0 - Rewrite the GitHub importer from scratch
• bcb7790a - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• a3c1baa2 - Rewrite the GitHub importer from scratch
• b534270d - Enable the new GitHub importer with a feature flag

Compare with previous version

marked the task See if we can re-use GitHub's diffs from the API for diff notes, instead of using Git(aly) as completed

@rymai In the old importer we'd re-fetch the refs of a repository if a pull request was updated after the repository was cloned. In the parallel importer this is currently not done because:

1. Pull requests are imported in separate threads/processes/hosts meaning we can't store a timestamp in a local variable and re-use it.
2. Pull request jobs are not given any additional timestamps for this, and I prefer to keep it that way. Right now the Sidekiq interface is generic, making it easy to add more workers without having to duplicate a lot of code. Adding this fetch feature would require specialisation just for importing PRs.
3. This would require some form of synchronisation (e.g. an ExclusiveLease) as otherwise many jobs could attempt to run the same git fetch. Adding this is something I prefer to avoid

This leads me to this question: is this feature important enough that we want to include it?

To reduce the number of queries and Gitaly RPC calls I made some changes to the Note, DiffNote, and Issue importers. These changes result in the importers bypassing the callbacks and validations of models by using Gitlab::Database.bulk_insert instead of the models. Bypassing the models (and thus any callbacks and validations) saves us a lot of SQL queries and a handful of Gitaly RPC calls.

To test this I imported https://github.com/yorickpeterse/oga into my local development environment. The before/after can be seen below.

Before bypassing callbacks/validation

Gitaly calls:

RPC call	Amount
repository_exists	36
has_local_branches	1
fetch_remote	1
find_commit	877
commits_between	70
commit_diff	70
ref_exists	490

• Total RPC calls: 1545
• Total SQL queries: 10806
• Total time: 165 seconds

After bypassing callbacks/validation

Gitaly calls:

RPC call	Amount
repository_exists	36
has_local_branches	1
fetch_remote	1
find_commit	770
commits_between	70
commit_diff	70
ref_exists	420

• Total RPC calls: 1368
• Total SQL queries: 4621
• Total time: 87 seconds

Conclusion

So by making these changes we save ourselves 177 Gitaly RPC calls, 6185 SQL queries, and about 78 seconds of execution time. Do keep in mind that this test was run over the summit's hotel wifi, thus the timings are likely to be influenced by how responsive the wifi was during the test.

added 2 commits

• 98786f21 - Rewrite the GitHub importer from scratch
• b768f6a0 - Enable the new GitHub importer with a feature flag

Compare with previous version

With some further caching (of labels and milestones) we can reduce the number of SQL queries to 4470 for Oga. I'll see if I can further reduce the queries, since 4470 is still quite a lot for such a small project.

added 2 commits

• 2254fe42 - Rewrite the GitHub importer from scratch
• b7269bda - Enable the new GitHub importer with a feature flag

Compare with previous version

As it turns out the caching of user IDs/Emails wasn't entirely working as expected. Fixing this and some other issues I ran into reduces the number of SQL queries to 4409. This is still a lot, but we're slowly getting there.

added 3 commits

• 68a73f3b - Add returning IDs to Gitlab::Database.bulk_insert
• 0fa4e34a - Rewrite the GitHub importer from scratch
• d85c051f - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 701ad4c2 - Rewrite the GitHub importer from scratch
• 7ca59490 - Enable the new GitHub importer with a feature flag

Compare with previous version

mentioned in issue #39361 (closed)

added 2 commits

• f92dc038 - Rewrite the GitHub importer from scratch
• ef3c2f23 - Enable the new GitHub importer with a feature flag

Compare with previous version

changed the description

With some further caching being added for issue/MR lookups (when importing notes, diff notes, and label links) the number of SQL queries can be reduced to 3590. About 1450 queries are for getting the feature flag names (see #39361 (closed) for more information), which isn't something I can really work around in this MR.

The remaining queries are mostly SELECT queries to build the cache, and a lot of INSERTs for the data we're importing.

added 2 commits

• 7444f768 - Rewrite the GitHub importer from scratch
• 064d642d - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 972aa37e - Rewrite the GitHub importer from scratch
• 9a48ad41 - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 6ec1ad62 - Rewrite the GitHub importer from scratch
• f6ee0b5d - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• c345b5af - Rewrite the GitHub importer from scratch
• 61392125 - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• bf44cf60 - Rewrite the GitHub importer from scratch
• 8be71784 - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 1a1eb38f - Rewrite the GitHub importer from scratch
• 729cfb7f - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• d7715ec1 - Rewrite the GitHub importer from scratch
• 8248fc21 - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 7ad447dd - Rewrite the GitHub importer from scratch
• 573a9eb4 - Enable the new GitHub importer with a feature flag

Compare with previous version

changed the description

added 2 commits

• 4af5b7d9 - Rewrite the GitHub importer from scratch
• e3cfb168 - Enable the new GitHub importer with a feature flag

Compare with previous version

mentioned in issue #33135 (closed)

added 133 commits

• e3cfb168...e16add22 - 129 commits from branch master
• cb58a673 - Refactor User.find_by_any_email
• 87722696 - Add returning IDs to Gitlab::Database.bulk_insert
• 371d16e4 - Rewrite the GitHub importer from scratch
• 661489c0 - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 4836cf3a - Rewrite the GitHub importer from scratch
• 3bd8aa3d - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 57ccaa84 - Rewrite the GitHub importer from scratch
• f7e1239c - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 85749fb8 - Rewrite the GitHub importer from scratch
• 62399e02 - Enable the new GitHub importer with a feature flag

Compare with previous version

is this feature important enough that we want to include it?

We could probably fail a PR import if the ref isn't fetched and restart a new import, which would do a fetch first so that PR that weren't imported correctly would be imported fine since their refs would have been fetched this time... I still need to look at this massive diff to see if that'd be possible, but you probably see the idea.

mentioned in issue #39427 (closed)

added 2 commits

• 9b14070d - Rewrite the GitHub importer from scratch
• 240d77c7 - Enable the new GitHub importer with a feature flag

Compare with previous version

@yorickpeterse I tried to import a large repository today but fetching the repository data over the summit's hotel wifi take a while and the import failed with the following error message: Import timed out. Import took longer than 54000 seconds. Is there a way that we can refresh the JID expiration while fetching the repository data?

@dbalexandre I don't think in practise any repository would ever take more than 15 hours of cloning, at least not in production environments.

We have a worker (AdvanceStageWorker) that takes care of refreshing the JID (this also happens in a few other places), but this is only done when the stage it waits for has been finished. This is necessary as always refreshing the JID can result in the jobs never getting finished. Basically what could happen in that case is:

1. A job in the stage fails to update our Gitlab::JobWaiter status
2. AdvanceStageWorker refreshes the JID, sees work remains, reschedules itself
3. This keeps happening forever. StuckImportJobsWorker won't be able to mark the import as failed since the JID keeps getting refreshed.

So what we'd have to do is schedule a separate job that runs e.g. every minute and refreshes the JID, until the cloning is done. I however wonder if in production environments we'd ever need this.

Basically the problem is this:

If we schedule a job every X minutes that refreshes the status it's possible for the import to get stuck forever, and StuckImportJobsWorker won't be able to mark the import as failed. If we don't do this we need to complete the cloning work in 15 hours.

If 15 hours isn't enough we could set the TTL to 24 hours, but I honestly think 15 hours should be more than enough to clone a repository.

What might work is this:

Before cloning the repository we note down the JID, then we schedule job A. Job A will run every few minutes and check if the clone job is done. When job A runs it will also refresh the importer JID (a different JID than we use for checking if the clone job runs). Once the clone job has died or completed we stop this process.

If I'm right this should prevent a job from getting stuck while also preventing it from getting marked as "failed" if a clone takes longer than 15 hours.

This has been implemented.

added 2 commits

• 21b194d9 - Rewrite the GitHub importer from scratch
• 448d1e4c - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• c8728573 - Rewrite the GitHub importer from scratch
• 2f2dbd72 - Enable the new GitHub importer with a feature flag

Compare with previous version

@rymai

We could probably fail a PR import if the ref isn't fetched and restart a new import, which would do a fetch first so that PR that weren't imported correctly would be imported fine since their refs would have been fetched this time... I still need to look at this massive diff to see if that'd be possible, but you probably see the idea.

All pull requests are imported in parallel, and I'd rather not have all of them also check the repository status using Git as this is likely to blow up the NFS shard.

So if we want this we need to store a timestamp somewhere that marks the time we finished cloning the repository. We then need synchronisation across processes/threads to make this work, otherwise e.g. 100 threads will run git fetch in parallel.

All pull requests are imported in parallel, and I'd rather not have all of them also check the repository status using Git as this is likely to blow up the NFS shard.

@yorickpeterse Sorry, it seems I wasn't clear in my answer above, I meant if a MR diff generation fails (most probably because ref isn't fetched), then we should retry the import after it's finished (in a failed state because of the MR diff that couldn't be generated). The import retry would start with a fresh git fetch, so the MR diff that couldn't be generated should now be generated properly...

Anyways, I will need to go through the diff to understand better how this will (or won't) work.

added 2 commits

• 6f58e81d - Rewrite the GitHub importer from scratch
• 6149b7aa - Enable the new GitHub importer with a feature flag

Compare with previous version

@rymai Let's discuss the PR in this discussion instead of separate comments, to keep things clear.

What we could do is update projects.last_activity_at once done cloning. We can then compare this with the updated_at value of a pull request. This would still require synchronisation when fetching, but at least we don't need to pass along extra data. I however don't really like the idea of abusing last_activity_at for this.

I however don't really like the idea of abusing last_activity_at for this.

Yeah I feel the same, but we could probably start with that...

@rymai I think we can do the following:

Instead of fetching when processing individual PRs (this would require synchronisation) we do this when scheduling the jobs (this runs in a single thread). This works because when scheduling PR jobs we include the commit IDs and what not, meaning we have a stable view of the PR (even after a push).

The benefit of this approach is that we don't need to schedule any additional data with PR jobs, and we don't need synchronisation. We however still need to store a timestamp somewhere that tracks the last clone time.

Apparently we have a timestamp called projects.last_repository_updated_at. I think using that makes more sense than last_activity_at.

added 2 commits

• 9872c93b - Rewrite the GitHub importer from scratch
• 7b1d922a - Enable the new GitHub importer with a feature flag

Compare with previous version

added 141 commits

• 7b1d922a...6919f636 - 136 commits from branch master
• dad72cdd - Refactor User.find_by_any_email
• 98d2aab8 - Add returning IDs to Gitlab::Database.bulk_insert
• 03f8f3d3 - Cache feature names in RequestStore
• 0cc94878 - Rewrite the GitHub importer from scratch
• 023e5ae2 - Enable the new GitHub importer with a feature flag

Compare with previous version

marked the task Re-fetch if a PR was updated after the initial repository fetch as completed

changed the description

resolved all discussions

@DouweM While I continue running tests on staging/locally, could you start reviewing this? There's quite a lot to review, so it may be easier to look at the individual commits instead of the diff in its entirety. We can also review this via a video call if that's easier.

Would it make sense to use Gitlab::Cache::RequestCache, here or on FlipperFeature?

Yes, I wasn't aware of this code; I'll add it.

Oh, actually we can't. Gitlab::Cache::RequestCache messes around with instance variables. This means we'd modify a shared global state, which could lead to all sorts of nasty issues.

Would it make more sense for this check to be in Gitlab::ImportSources? We could allow the ImportSource importer attribute to be a proc, so it can evaluate to either Github::Import (as it does now), or Gitlab::GithubImport::ParallelImporter.

Also, why do we have both Github::Import and Gitlab::GithubImport::Importer, with github using the former, and gitea using the latter?

Edit: Ah, never mind, you already point that out in 0cc94878.

@DouweM Yeah the naming is stupid, maybe we should just rename it to Gitea importer instead of LegacyGithubImporter; though IIRC the code makes assumptions/references to GitHub, so that would also be odd.

Github::Import is the old importer, which will be nuked once we're confident with these changes. Initially I was going to dump this in a patch release, but with the merge window closing in I'm leaning towards hot-patching GitLab.com and removing Github::Import in this MR once we're confident. That way this MR introduces a "final" state, instead of something that's still a WIP.

For example, there's code such as:

• OmniAuth::Strategies::GitHub.default_options[:client_options].symbolize_keys
• Gitlab.config.omniauth.providers.find { |provider| provider.name == "github" }
• joins(:identities).where(identities[:provider].eq(:github)

So naming this GiteaImporter would be quite weird.

@yorickpeterse If it's only used for Gitea, shouldn't those github mentions be gitea anyway? :/ Or does Gitea actually use the github OmniAuth provider?

@DouweM The importer used to be used for GitHub, hence the mentions. Gitea offers a GitHub compatible API as far as I can tell, hence it's now used for Gitea while Github::Import handles GitHub. For Gitea things such as the Omniauth provider are still "github", and I have no idea if we can replace that.

added 2 commits

• c3389cd7 - Rewrite the GitHub importer from scratch
• fafeb06f - Enable the new GitHub importer with a feature flag

Compare with previous version

mentioned in issue #27429 (closed)

changed the description

changed the description

changed the description

marked the task Some GitHub pull requests use the same source/target branch, but GitLab does not seem to support this. Example: https://github.com/rubinius/rubinius/pull/704. This may happen when the source repository/project (or user) no longer exists. See https://sentry.gitlap.com/gitlab/staginggitlabcom/issues/107832/ for more info. as completed

added 2 commits

• 507f3dba - Rewrite the GitHub importer from scratch
• fc7eb290 - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 638b3060 - Rewrite the GitHub importer from scratch
• c97b5efe - Enable the new GitHub importer with a feature flag

Compare with previous version

"For every comment we schedule a job for the Gitlab::GithubImport::ImportNoteWorker worker"

changed this line in version 63 of the diff

regulra -> regular

changed this line in version 63 of the diff

Foror -> For

changed this line in version 63 of the diff

Should we also take group labels into account here, so that we don't try to create a new label on the project when it already exists on the group?

@DouweM The same applies here as milestones. If we can get them easily (there doesn't seem to be something like Project#group_labels), I think so; otherwise it will probably become too complex/annoying.

@yorickpeterse I think we can do that using the LabelsFinder.

We also have group milestones now: https://gitlab.com/groups/gitlab-org/milestones/new

Do we need to do anything special?

@DouweM Not sure. In the current state we import all repository milestones as project milestones, which could lead to duplicate milestones (one group milestone and a project milestone).

Given a project, how would you obtain the group milestones available to it? Is this list simply limited to the milestones available to the group that owns the project? Or can you access all milestones from groups that the project is shared with? If it's based on ownership we could change this code to handle group milestones, but otherwise I think it will get too complex/slow too quickly; meaning I'd rather just skip it.

@yorickpeterse I think we can use the MilestonesFinder.

@DouweM Looking at the code I'm outright scared of using that. MilestonesFinder seems to operate on raw IDs, so does Milestone.for_projects_and_groups.

Even then it's not clear how we'd pass the list of groups to this finder. Would we pass [project.namespace], or do we also have to get all ancestors and what not? What about the groups the project is shared with.

@yorickpeterse Judging by https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/services/issuable_base_service.rb#L104 and http://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/controllers/projects/milestones_controller.rb#L106, it only needs the project ID and its immediate parent group ID.

Projects inherit milestones from their parent group, but nothing else.

@DouweM Ah, that makes things easier. I'll take a look to see how difficult it would be to add this, if it's too much we can do it in a separate MR.

I'm going to create issues for this so we can tackle this separately, that way we can at least get something out of the door (the old importer didn't handle this either so we don't lose anything).

created #39724 to continue this discussion

Did you happen to see if it would be feasible to also import system (event) notes? See also #15377.

@DouweM We can add it, but it will probably require several extra days of development and testing to get going. As such I think it's better to do this separately, that way we can get the MR as it currently is out of the door quicker.

@yorickpeterse I agree, it was just a question that came to mind reading this code :)

I think we could/should reuse this for all importers that may take more than 15minutes, like the simple Git repo importer, and even the fork worker.

@DouweM In the future yes, but for this MR I prefer not to mess too much with all the other (largely untested) importers. In particular the problem is that for non GitHub importers we handle the cloning in Projects::ImportService#add_repository_to_project, which doesn't have access to the job ID of the current Sidekiq job.

Actually we can't do this unless we rewrite the other importers. RefreshImportJidWorker requires the cloning process to be done in a separate worker, whereas all other importers perform all their work in a single one. If we use RefreshImportJidWorker for other importers (in the current setup) this would lead to job IDs being refreshed until the import work is done. This in turn means an import job could never be marked as stuck, allowing them to run effectively indefinitely.

@yorickpeterse I'm fine with doing it in a separate issue/MR.

If we use RefreshImportJidWorker for other importers (in the current setup) this would lead to job IDs being refreshed until the import work is done. This in turn means an import job could never be marked as stuck, allowing them to run effectively indefinitely.

Isn't that a good thing? I don't think we should be marking imports as stuck that are actually still running. As I see it, an import is only stuck if it started, but somehow the job got killed and wasn't requeued, like when Sidekiq gets a SIGKILL while the job is running.

Isn't that a good thing? I don't think we should be marking imports as stuck that are actually still running.

Yes and no. We'd have to change StuckImportJobsWorker to only mark dead import jobs as stuck (= job ID no longer exists but the state is not "finished"). Then we'd have to change all importers to perform their cloning separately as otherwise jobs will never get marked as stuck (since RefreshImportJidWorker would be refreshing the same JID that it checks, thus never letting it time out).

tl;dr we'd have to change the other importers in a similar fashion, then make StuckImportJobsWorker handle all this better.

Yes and no. We'd have to change StuckImportJobsWorker to only mark dead import jobs as stuck (= job ID no longer exists but the state is not "finished").

@yorickpeterse Doesn't it already do that? The query is Project.with_import_status(:scheduled, :started).where.not(import_jid: nil), and then we use Gitlab::SidekiqStatus to check those JIDs.

Then we'd have to change all importers to perform their cloning separately as otherwise jobs will never get marked as stuck (since RefreshImportJidWorker would be refreshing the same JID that it checks, thus never letting it time out).

I don't think that's right, since Gitlab::SidekiqStatus::ServerMiddleware would still unset the JID when the job actually finishes, and RefreshImportJidWorker only refreshes if the JID is still set. So once it finishes, it won't be reset again.

We'd "just" need to make sure that RefreshImportJidWorker doesn't set the status for the JID if it was actually unset by Gitlab::SidekiqStatus::ServerMiddleware between checking SidekiqStatus.running? and calling project.refresh_import_jid_expiration.

---

Totally something that can go into a followup MR, but something I think we should definitely do :)

Hm maybe this should be possible after all. Either way I don't want to do this in this MR as it will get even bigger than it already is.

created #39725 to continue this discussion

Would it make sense to import issues and PRs in parallel, and then label links, diff notes and regular notes in parallel too? It sounds like that would remove a stage.

Or don't we want to do label links separately from issues, because we'd need to iterate over issues/PRs again?

Could we do label links in-line with issues and PRs?

@DouweM Issues are imported separately (and after PRs) since some issues are also PRs, allowing us to update the label links. The PRs endpoint for whatever reason doesn't include this information.

For small projects we could do a separate crawl through issues just for the label links, but for larger projects (e.g. kubernetes) this could waste too many requests. For example, Kubernetes has 22 117 issues which would require 222 API calls (when showing 100 per page). Crawling the data separately would mean 444 API calls. A large number of API calls is already spent on getting user data, by merging issues and label links into a single crawl we can reduce the likelihood of hitting the rate limit (which would mean having to wait up to an hour).

So long story short: I prefer not to because it will increase the number of API calls.

@yorickpeterse Sounds reasonable. It may be worth explicitly explaining this in the code or the docs, since the order of the stages is a little counterintuitive.

I think it will cause confusion that noteable_type and noteable_id on a GitHub note representation and an actual note model represent something similar, but different. Could we store MergeRequest/Issue instead of pull/issues for noteable_type, and call noteable_id noteable_iid?

@DouweM The reason for this is mostly because I prefer to serialise (when scheduling via Sidekiq) simple (and small, so we don't waste a ton of RAM) data such as :pull instead of class names (e.g. MergeRequest). We can do the latter, but then we'd have to do something like 'MergeRequest'.constantize which I'm generally not a fan of.

We can change the field names to something like note_target_type and note_target_id to make it more clear these are "raw" values.

@yorickpeterse I would personally prefer MergeRequest/Issue because that's what we send in forms etc anyway, but I'm fine with anything that makes this less confusing :) Note that I don't think we would actually need to explicitly 'MergeRequest'.constantize, since AR will do that automatically if you do note.noteable_type = 'MergeRequest'.

Could this be file_path, since it's not the path to a note?

changed this line in version 64 of the diff

If this is the author, could we call it that?

changed this line in version 62 of the diff

If this is the author, could we call it that?

changed this line in version 62 of the diff

If this is the author, could we call it that?

changed this line in version 62 of the diff

If this is the author, could we call it that?

changed this line in version 62 of the diff

@godfat @jameslopez You two may want to have a look too, since you've been working on the GitHub importer too for https://gitlab.com/gitlab-org/gitlab-ce/issues/36284.

added 2 commits

• b192b2bd - Rewrite the GitHub importer from scratch
• 8189c5ea - Enable the new GitHub importer with a feature flag

Compare with previous version

marked the task Sometimes inserts into merge_requests produce duplicate key errors in the index index_merge_requests_on_target_project_id_and_iid. This is probably due to GitHub sometimes returning duplicate data. We can solve this by not scheduling PRs for which we have already scheduled an importing run (using a Set of iid values). See https://sentry.gitlap.com/gitlab/staginggitlabcom/issues/107834/ for more info. as completed

added 115 commits

• 8189c5ea...01340794 - 110 commits from branch master
• 27c27955 - Refactor User.find_by_any_email
• ab8dfa8e - Add returning IDs to Gitlab::Database.bulk_insert
• 16166dfe - Cache feature names in RequestStore
• e0b7dad5 - Rewrite the GitHub importer from scratch
• 0d287d71 - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 325ddb00 - Rewrite the GitHub importer from scratch
• f03588c3 - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 0c465f18 - Rewrite the GitHub importer from scratch
• 935910d1 - Enable the new GitHub importer with a feature flag

Compare with previous version

changed the description

added 2 commits

• 20e98b71 - Rewrite the GitHub importer from scratch
• 578a8f17 - Enable the new GitHub importer with a feature flag

Compare with previous version

mentioned in issue gitlab-com/infrastructure#3124 (closed)

added 2 commits

• 3547aa2a - Rewrite the GitHub importer from scratch
• 492638ea - Enable the new GitHub importer with a feature flag

Compare with previous version

mentioned in issue gitlab-com/infrastructure#3128 (closed)

changed the description

changed the description

changed the description

changed the description

added 2 commits

• 434086a6 - Rewrite the GitHub importer from scratch
• 8feffb40 - Enable the new GitHub importer with a feature flag

Compare with previous version

marked the task Handle rescheduled jobs better when cloning (e.g. don't add the remotes if they already exist). Right now a job getting terminated in the middle (e.g. by the memory killer) can prevent us from starting over/resuming as completed

mentioned in issue #39724

resolved all discussions

mentioned in issue #39725

changed the description

For those seeing this and thinking "Gee, I want to try this out before 10.2.0 releases", here's how:

1. SSH into the server(s) running GitLab
2. Run curl -s https://gitlab.com/snippets/1681949/raw -o /tmp/importer.patch on every server
3. Run sudo patch -p1 -N -s -d /opt/gitlab/embedded/service/gitlab-rails < /tmp/importer.patch on every server. This assumes GitLab is installed in /opt/gitlab/embedded/service/gitlab-rails (the omnibus default). You may not need sudo if it's installed in a directory your user can access.
4. Restart GitLab on all servers (Unicorn, Sidekiq, etc)
5. Since the code is hidden behind a feature flag (this will be taken out before getting merged0 you will need to enable it for the user you want to use for testing. You can do this by running the following on a server:

sudo gitlab-rails r "Feature.enable(:parallel_github_importer, User.find_by(username: 'username of the user here'))"

So for example if the user is "yorickpeterse" you'd run:

sudo gitlab-rails r "Feature.enable(:parallel_github_importer, User.find_by(username: 'yorickpeterse'))"

Then you can import the project as usual. If nothing is happening you should make sure that Sidekiq (our background processing code) processes the following two queues:

• github_importer
• github_importer_advance_stage

Omnibus installations will process these queues automatically

The requirement for this is that you have to run at least EE 10.1.0. The supplied patch will not work on CE.

added 159 commits

• 8feffb40...64c9d780 - 154 commits from branch master
• 90fb0741 - Refactor User.find_by_any_email
• 2f2f2c2e - Add returning IDs to Gitlab::Database.bulk_insert
• cccb2fe3 - Cache feature names in RequestStore
• 11584bd8 - Rewrite the GitHub importer from scratch
• 161cb5c4 - Enable the new GitHub importer with a feature flag

Compare with previous version

mentioned in issue #37814

mentioned in issue #38948 (closed)

Awesome work @yorickpeterse! Would it be possible to split this MR into few iterations? 🤔

@grzesiek No. While there are a few separate commits that could be moved those changes are so small they'd have no impact on the reviewing process. The core changes themselves can't be split up because they don't work on their own / depend on each other.

mentioned in issue #12450 (closed)

mentioned in issue #17995 (closed)

changed the description

changed the description

changed the description

added 59 commits

• 161cb5c4...4841c3cb - 54 commits from branch master
• 4479845c - Refactor User.find_by_any_email
• cdb671c4 - Add returning IDs to Gitlab::Database.bulk_insert
• 71a34494 - Cache feature names in RequestStore
• ada56efe - Rewrite the GitHub importer from scratch
• 2a528131 - Enable the new GitHub importer with a feature flag

Compare with previous version

marked the task The logic used for refreshing repositories before importing PRs doesn't scale. Frequently updated PRs will lead to many refetches of the repository, even if the repository wasn't actually updated. Since we schedule the refs in a job we really only need to refetch if the refs can't be found. This will require a lease however. as completed

added 2 commits

• eafe3e44 - Rewrite the GitHub importer from scratch
• 1227b80e - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• f7c421de - Rewrite the GitHub importer from scratch
• 11bf14d1 - Enable the new GitHub importer with a feature flag

Compare with previous version

marked the task It seems that under certain conditions the page numbers of e.g. the pull requests data isn't tracked properly. It seems to work fine locally, so perhaps something else botched this up. This needs some additional testing. as completed

mentioned in merge request !13788 (closed)

added 247 commits

• 11bf14d1...aaee69c4 - 242 commits from branch master
• b9785387 - Refactor User.find_by_any_email
• 79d1d32f - Add returning IDs to Gitlab::Database.bulk_insert
• 47b39cc1 - Cache feature names in RequestStore
• 335f3742 - Rewrite the GitHub importer from scratch
• 287e54a1 - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• daa7ae9d - Rewrite the GitHub importer from scratch
• 0331e1ed - Enable the new GitHub importer with a feature flag

Compare with previous version

added 2 commits

• 47c2b926 - Rewrite the GitHub importer from scratch
• 9a8f3f60 - Replace old GH importer with the parallel importer

Compare with previous version

added 1 commit

• 59a8df32 - Replace old GH importer with the parallel importer

Compare with previous version

added 1 commit

• b68e63e2 - Replace old GH importer with the parallel importer

Compare with previous version

added 64 commits

• b68e63e2...21d8ec15 - 59 commits from branch master
• b433a499 - Refactor User.find_by_any_email
• 1f72403a - Add returning IDs to Gitlab::Database.bulk_insert
• b7216bfd - Cache feature names in RequestStore
• 0960651c - Rewrite the GitHub importer from scratch
• 2273d8a1 - Replace old GH importer with the parallel importer

Compare with previous version

changed the description

added 85 commits

• 2273d8a1...c71cf908 - 80 commits from branch master
• 1ed36936 - Refactor User.find_by_any_email
• 04c2b88d - Add returning IDs to Gitlab::Database.bulk_insert
• 08878777 - Cache feature names in RequestStore
• e7c215e6 - Rewrite the GitHub importer from scratch
• becb4499 - Replace old GH importer with the parallel importer

Compare with previous version

changed the description

marked the task Changelog entry added, if necessary as completed

marked the task Documentation created/updated as completed

marked the task API support added as completed

marked the task Tests added for this feature/bug as completed

marked the task Conform by the merge request performance guides as completed

marked the task Squashed related commits together as completed

@DouweM Apart from the unrelated static-analysis/karma failures, this is ready for final review. Since the last review a few things have changed such as:

• How we paginate data and resume where we left off (we track things in Redis in an atomic way)
• A bunch of bugs have been fixed (e.g. the code used for handling dead messages was broken)
• The HTTP client has been adjusted to better paginate data
• Feature flag code has been removed and the importer has been made the default, this also means the Github::Import code has been removed as it is no longer needed

Most of these however are fairly specific changes, the bigger picture still remains the same.

added 75 commits

• becb4499...666ab488 - 70 commits from branch master
• 2432261a - Refactor User.find_by_any_email
• c703a19e - Add returning IDs to Gitlab::Database.bulk_insert
• af6a1b38 - Cache feature names in RequestStore
• 59d618e5 - Rewrite the GitHub importer from scratch
• 57db4c8a - Replace old GH importer with the parallel importer

Compare with previous version

Why do we keep LegacyGithubImport around?

Right, Gitea, I keep forgetting. So confusing.

Is this the same thing as importer_class.try(:async?)?

changed this line in version 81 of the diff

Why does RefreshImportJidWorker need a check_job_id? Could it use the actual project.import_jid? They are equal anyway, since we update the import_jid column in Gitlab::GithubImport::ParallelImporter after we. schedule this ImportRepositoryWorker.

@DouweM Because otherwise it would keep running forever. If we were to check key A and refresh it while running (= meaning the key exists) we'd never stop since we'd be refreshing the same key (= it would never time out as a result). This could lead to the import process getting stuck if the clone job were to die unexpectedly and somehow not unset the JID key.

The values also aren't equal, since we're checking the JID of Stage::ImportRepositoryWorker, not the one originally produced by RepositoryImportWorker.

@DouweM Derp you're right, I don't even remember how my own code works. We set import_jid in the ParallelImporter.

I'll see if I can get jid of the check_job_id since it's somewhat redundant, and if the keep-running-forever problem described above is something we can deal with.

The more I think of it, the more I think RefreshImportJidWorker is not as useful as anticipated, at least not in the current implementation.

First of all it can lead to an import getting stuck. This can happen when a ImportRepositoryWorker job gets terminated in such a way that its JID does not get removed (e.g. a SIGKILL). In this case RefreshImportJidWorker will keep refreshing the same JID forever, preventing the import from getting marked as stuck.

Second there's a race condition. Because we use the JID of ImportRepositoryWorker the import can be marked as stuck after that worker finishes and before the next worker starts, this is because Sidekiq will unset the JID. To work around this we'd have to use a custom import_jid value, unrelated to any actual Sidekiq workers.

The only way I can see us solving both problems is by having an additional thread in every Sidekiq process. This thread has a synchronised list of JIDs that is updated by Sidekiq threads as jobs come and go. Once a minute this thread wakes up, takes the list of current JIDs (= the ones the process is running), then refreshes their TTL. This however still requires a unique JID for GitHub imports since the work is spread across multiple JIDs.

Basically we'd need a way to distinguish between a job that is still running, and a job that is no longer running but for which the key still exists.

I changed the code to at least use custom values for projects.import_jid, solving the problem of refreshing the same JID. This means we basically have 30 hours to perform the clone (15 hours timeout in case the clone job crashes, then another 15 hours for the custom JID). This is good enough for now, but in the long term we need something better.

"scheume"

changed this line in version 81 of the diff

Can we create a scope on User for this? That will make it slightly easier to find and fix this when we get to #38822 (closed). I know that this ID is numeric, so it doesn't really matter, but scopes make it easier to be consistent :)

changed this line in version 81 of the diff

changed the description

added 54 commits

• 57db4c8a...83e96c7b - 51 commits from branch master
• dea6aa55 - Refactor User.find_by_any_email
• dec01b5c - Add returning IDs to Gitlab::Database.bulk_insert
• 528ea70a - Cache feature names in RequestStore

Compare with previous version

added 2 commits

• 4b5afc81 - Rewrite the GitHub importer from scratch
• c7ead95a - Replace old GH importer with the parallel importer

Compare with previous version

added 2 commits

• 09282d2a - Rewrite the GitHub importer from scratch
• d04ad08f - Replace old GH importer with the parallel importer

Compare with previous version

added 125 commits

• d04ad08f...673b6be1 - 120 commits from branch master
• 44be82dd - Refactor User.find_by_any_email
• bda30182 - Add returning IDs to Gitlab::Database.bulk_insert
• 90be53c5 - Cache feature names in RequestStore
• 91c34e0d - Rewrite the GitHub importer from scratch
• 6ce286f9 - Replace old GH importer with the parallel importer

Compare with previous version

added 2 commits

• 4dfe26cd - Rewrite the GitHub importer from scratch
• 6e242e82 - Replace old GH importer with the parallel importer

Compare with previous version

resolved all discussions

assigned to @DouweM

approved this merge request

mentioned in commit 92249f1a

merged

mentioned in merge request gitlab-ee!3310 (merged)

EE MR: gitlab-ee!3310 (merged)

mentioned in merge request gitlab-com/www-gitlab-com!7736 (merged)

mentioned in commit gitlab-ee@d1825985

mentioned in issue #31425 (closed)

mentioned in issue #40107

mentioned in issue #37249