Add latest changes from gitlab-org/gitlab@master

app/services/projects/destroy_rollback_service.rb

+# frozen_string_literal: true
+
+module Projects
+  class DestroyRollbackService < BaseService
+    include Gitlab::ShellAdapter
+
+    def execute
+      return unless project
+
+      Projects::ForksCountService.new(project).delete_cache
+
+      unless rollback_repository(project.repository)
+        raise_error(s_('DeleteProject|Failed to restore project repository. Please contact the administrator.'))
+      end
+
+      unless rollback_repository(project.wiki.repository)
+        raise_error(s_('DeleteProject|Failed to restore wiki repository. Please contact the administrator.'))
+      end
+    end
+
+    private
+
+    def rollback_repository(repository)
+      return true unless repository
+
+      result = Repositories::DestroyRollbackService.new(repository).execute
+
+      result[:status] == :success
+    end
+  end
+end

app/services/projects/destroy_service.rb

@@ -6,9 +6,6 @@ module Projects
     DestroyError = Class.new(StandardError)
-    DELETED_FLAG = '+deleted'
-    REPO_REMOVAL_DELAY = 5.minutes.to_i
-
     def async_execute
       project.update_attribute(:pending_delete, true)
@@ -18,7 +15,7 @@ module Projects
       schedule_stale_repos_removal
       job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
-      Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.full_path} with job ID #{job_id}") # rubocop:disable Gitlab/RailsLogger
+      log_info("User #{current_user.id} scheduled destruction of project #{project.full_path} with job ID #{job_id}")
     end
     def execute
@@ -48,82 +45,34 @@ module Projects
       raise
     end
-    def attempt_repositories_rollback
-      return unless @project
-
-      flush_caches(@project)
-
-      unless rollback_repository(removal_path(repo_path), repo_path)
-        raise_error(s_('DeleteProject|Failed to restore project repository. Please contact the administrator.'))
-      end
-
-      unless rollback_repository(removal_path(wiki_path), wiki_path)
-        raise_error(s_('DeleteProject|Failed to restore wiki repository. Please contact the administrator.'))
-      end
-    end
-
     private
-    def repo_path
-      project.disk_path
-    end
-
-    def wiki_path
-      project.wiki.disk_path
-    end
-
     def trash_repositories!
-      unless remove_repository(repo_path)
+      unless remove_repository(project.repository)
         raise_error(s_('DeleteProject|Failed to remove project repository. Please try again or contact administrator.'))
       end
-      unless remove_repository(wiki_path)
+      unless remove_repository(project.wiki.repository)
         raise_error(s_('DeleteProject|Failed to remove wiki repository. Please try again or contact administrator.'))
       end
     end
-    def remove_repository(path)
-      # There is a possibility project does not have repository or wiki
-      return true unless repo_exists?(path)
-      new_path = removal_path(path)
-      if mv_repository(path, new_path)
-        log_info(%Q{Repository "#{path}" moved to "#{new_path}" for project "#{project.full_path}"})
-
-        project.run_after_commit do
-          GitlabShellWorker.perform_in(REPO_REMOVAL_DELAY, :remove_repository, self.repository_storage, new_path)
-        end
-      else
-        false
-      end
+    def remove_repository(repository)
+      return true unless repository
+      result = Repositories::DestroyService.new(repository).execute
+      result[:status] == :success
     end
     def schedule_stale_repos_removal
-      repo_paths = [removal_path(repo_path), removal_path(wiki_path)]
-      # Ideally it should wait until the regular removal phase finishes,
-      # so let's delay it a bit further.
-      repo_paths.each do |path|
-        GitlabShellWorker.perform_in(REPO_REMOVAL_DELAY * 2, :remove_repository, project.repository_storage, path)
-      end
-    end
-    def rollback_repository(old_path, new_path)
-      # There is a possibility project does not have repository or wiki
-      return true unless repo_exists?(old_path)
-
-      mv_repository(old_path, new_path)
-    end
-
-    def repo_exists?(path)
-      gitlab_shell.repository_exists?(project.repository_storage, path + '.git')
-    end
-
-    def mv_repository(from_path, to_path)
-      return true unless repo_exists?(from_path)
-
-      gitlab_shell.mv_repository(project.repository_storage, from_path, to_path)
+      repos = [project.repository, project.wiki.repository]
+      repos.each do |repository|
+        next unless repository
+        Repositories::ShellDestroyService.new(repository).execute(Repositories::ShellDestroyService::STALE_REMOVAL_DELAY)
+      end
     end
     def attempt_rollback(project, message)
@@ -191,32 +140,9 @@ module Projects
       raise DestroyError.new(message)
     end
-    # Build a path for removing repositories
-    # We use `+` because its not allowed by GitLab so user can not create
-    # project with name cookies+119+deleted and capture someone stalled repository
-    #
-    # gitlab/cookies.git -> gitlab/cookies+119+deleted.git
-    #
-    def removal_path(path)
-      "#{path}+#{project.id}#{DELETED_FLAG}"
-    end
-
     def flush_caches(project)
-      ignore_git_errors(repo_path) { project.repository.before_delete }
-
-      ignore_git_errors(wiki_path) { Repository.new(wiki_path, project, disk_path: repo_path).before_delete }
-
       Projects::ForksCountService.new(project).delete_cache
     end
-
-    # If we get a Gitaly error, the repository may be corrupted. We can
-    # ignore these errors since we're going to trash the repositories
-    # anyway.
-    def ignore_git_errors(disk_path, &block)
-      yield
-    rescue Gitlab::Git::CommandError => e
-      Gitlab::GitLogger.warn(class: self.class.name, project_id: project.id, disk_path: disk_path, message: e.to_s)
-    end
   end
 end

app/services/projects/overwrite_project_service.rb

@@ -55,7 +55,7 @@ module Projects
     end
     def attempt_restore_repositories(project)
-      ::Projects::DestroyService.new(project, @current_user).attempt_repositories_rollback
+      ::Projects::DestroyRollbackService.new(project, @current_user).execute
     end
     def add_source_project_to_fork_network(source_project)

app/services/repositories/base_service.rb

+# frozen_string_literal: true
+
+class Repositories::BaseService < BaseService
+  include Gitlab::ShellAdapter
+
+  DELETED_FLAG = '+deleted'
+
+  attr_reader :repository
+
+  delegate :project, :disk_path, :full_path, to: :repository
+  delegate :repository_storage, to: :project
+
+  def initialize(repository)
+    @repository = repository
+  end
+
+  def repo_exists?(path)
+    gitlab_shell.repository_exists?(repository_storage, path + '.git')
+  end
+
+  def mv_repository(from_path, to_path)
+    return true unless repo_exists?(from_path)
+
+    gitlab_shell.mv_repository(repository_storage, from_path, to_path)
+  end
+
+  # Build a path for removing repositories
+  # We use `+` because its not allowed by GitLab so user can not create
+  # project with name cookies+119+deleted and capture someone stalled repository
+  #
+  # gitlab/cookies.git -> gitlab/cookies+119+deleted.git
+  #
+  def removal_path
+    "#{disk_path}+#{project.id}#{DELETED_FLAG}"
+  end
+
+  # If we get a Gitaly error, the repository may be corrupted. We can
+  # ignore these errors since we're going to trash the repositories
+  # anyway.
+  def ignore_git_errors(&block)
+    yield
+  rescue Gitlab::Git::CommandError => e
+    Gitlab::GitLogger.warn(class: self.class.name, project_id: project.id, disk_path: disk_path, message: e.to_s)
+  end
+
+  def move_error(path)
+    error = %Q{Repository "#{path}" could not be moved}
+
+    log_error(error)
+    error(error)
+  end
+end

app/services/repositories/destroy_rollback_service.rb

+# frozen_string_literal: true
+
+class Repositories::DestroyRollbackService < Repositories::BaseService
+  def execute
+    # There is a possibility project does not have repository or wiki
+    return success unless repo_exists?(removal_path)
+
+    # Flush the cache for both repositories.
+    ignore_git_errors { repository.before_delete }
+
+    if mv_repository(removal_path, disk_path)
+      log_info(%Q{Repository "#{removal_path}" moved to "#{disk_path}" for repository "#{full_path}"})
+
+      success
+    else
+      move_error(removal_path)
+    end
+  end
+end

app/services/repositories/destroy_service.rb

+# frozen_string_literal: true
+
+class Repositories::DestroyService < Repositories::BaseService
+  def execute
+    return success unless repository
+    return success unless repo_exists?(disk_path)
+
+    # Flush the cache for both repositories. This has to be done _before_
+    # removing the physical repositories as some expiration code depends on
+    # Git data (e.g. a list of branch names).
+    ignore_git_errors { repository.before_delete }
+
+    if mv_repository(disk_path, removal_path)
+      log_info(%Q{Repository "#{disk_path}" moved to "#{removal_path}" for repository "#{full_path}"})
+
+      current_repository = repository
+      project.run_after_commit do
+        Repositories::ShellDestroyService.new(current_repository).execute
+      end
+
+      log_info("Project \"#{project.full_path}\" was removed")
+
+      success
+    else
+      move_error(disk_path)
+    end
+  end
+end

app/services/repositories/shell_destroy_service.rb

+# frozen_string_literal: true
+
+class Repositories::ShellDestroyService < Repositories::BaseService
+  REPO_REMOVAL_DELAY = 5.minutes.to_i
+  STALE_REMOVAL_DELAY = REPO_REMOVAL_DELAY * 2
+
+  def execute(delay = REPO_REMOVAL_DELAY)
+    return success unless repository
+
+    GitlabShellWorker.perform_in(delay,
+                                 :remove_repository,
+                                 repository_storage,
+                                 removal_path)
+  end
+end

doc/administration/geo/disaster_recovery/index.md

@@ -87,10 +87,14 @@ must disable the **primary** node.
 ### Step 3. Promoting a **secondary** node
-NOTE: **Note:**
-A new **secondary** should not be added at this time. If you want to add a new
-**secondary**, do this after you have completed the entire process of promoting
-the **secondary** to the **primary**.
+Note the following when promoting a secondary:
+
+- A new **secondary** should not be added at this time. If you want to add a new
+  **secondary**, do this after you have completed the entire process of promoting
+  the **secondary** to the **primary**.
+- If you encounter an `ActiveRecord::RecordInvalid: Validation failed: Name has already been taken`
+  error during this process, please read
+  [the troubleshooting advice](../replication/troubleshooting.md#fixing-errors-during-a-failover-or-when-promoting-a-secondary-to-a-primary-node).
 #### Promoting a **secondary** node running on a single machine

doc/administration/geo/replication/troubleshooting.md

@@ -465,6 +465,47 @@ to start again from scratch, there are a few steps that can help you:
    gitlab-ctl start
    ```
+## Fixing errors during a failover or when promoting a secondary to a primary node
+
+The following are possible errors that might be encountered during failover or
+when promoting a secondary to a primary node with strategies to resolve them.
+
+### Message: ActiveRecord::RecordInvalid: Validation failed: Name has already been taken
+
+When [promoting a **secondary** node](../disaster_recovery/index.md#step-3-promoting-a-secondary-node),
+you might encounter the following error:
+
+```text
+Running gitlab-rake geo:set_secondary_as_primary...
+
+rake aborted!
+ActiveRecord::RecordInvalid: Validation failed: Name has already been taken
+/opt/gitlab/embedded/service/gitlab-rails/ee/lib/tasks/geo.rake:236:in `block (3 levels) in <top (required)>'
+/opt/gitlab/embedded/service/gitlab-rails/ee/lib/tasks/geo.rake:221:in `block (2 levels) in <top (required)>'
+/opt/gitlab/embedded/bin/bundle:23:in `load'
+/opt/gitlab/embedded/bin/bundle:23:in `<main>'
+Tasks: TOP => geo:set_secondary_as_primary
+(See full trace by running task with --trace)
+
+You successfully promoted this node!
+```
+
+If you encounter this message when running `gitlab-rake geo:set_secondary_as_primary`
+or `gitlab-ctl promote-to-primary-node`, either:
+
+- Enter a Rails console and run:
+  
+  ```ruby
+  Rails.application.load_tasks; nil
+  Gitlab::Geo.expire_cache_keys!([:primary_node, :current_node])
+  Rake::Task['geo:set_secondary_as_primary'].invoke
+  ```
+
+- Upgrade to GitLab 12.6.3 or newer if it is safe to do so. For example,
+  if the failover was just a test. A [caching-related
+  bug](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/22021) was
+  fixed.
+
 ## Fixing Foreign Data Wrapper errors
 This section documents ways to fix potential Foreign Data Wrapper errors.

doc/user/group/saml_sso/scim_setup.md

@@ -143,6 +143,19 @@ Upon the next sync, the user will be deprovisioned, which means that the user wi
 This section contains possible solutions for problems you might encounter.
+### How do I verify my SCIM configuration is correct?
+
+Review the following:
+
+- Ensure that the SCIM value for `id` matches the SAML value for `NameId`.
+- Ensure that the SCIM value for `externalId` matches the SAML value for `NameId`.
+
+Review the following SCIM parameters for sensible values:
+
+- `userName`
+- `displayName`
+- `emails[type eq "work"].value`
+
 ### Testing Azure connection: invalid credentials
 When testing the connection, you may encounter an error: **You appear to have entered invalid credentials. Please confirm you are using the correct information for an administrative account**. If `Tenant URL` and `secret token` are correct, check whether your group path contains characters that may be considered invalid JSON primitives (such as `.`). Removing such characters from the group path typically resolves the error.
@@ -159,3 +172,43 @@ As a workaround, try an alternate mapping:
 1. Follow the Azure mapping instructions from above.
 1. Delete the `name.formatted` target attribute entry.
 1. Change the `displayName` source attribute to have `name.formatted` target attribute.
+
+### How do I diagnose why a user is unable to sign in
+
+The **Identity** (`extern_uid`) value stored by GitLab is updated by SCIM whenever `id` or `externalId` changes. Users won't be able to sign in unless the GitLab Identity (`extern_uid`) value matches the `NameId` sent by SAML.
+
+This value is also used by SCIM to match users on the `id`, and is updated by SCIM whenever the `id` or `externalId` values change.
+
+It is important that this SCIM `id` and SCIM `externalId` are configured to the same value as the SAML `NameId`. SAML responses can be traced using [debugging tools](./index.md#saml-debugging-tools), and any errors can be checked against our [SAML troubleshooting docs](./index.md#troubleshooting).
+
+### How do I verify user's SAML NameId matches the SCIM externalId
+
+Group owners can see the list of users and the `externalId` stored for each user in the group SAML SSO Settings page.
+
+Alternatively, the [SCIM API](../../../api/scim.md#get-a-list-of-saml-users) can be used to manually retrieve the `externalId` we have stored for users, also called the `external_uid` or `NameId`.
+
+For example:
+
+```sh
+curl 'https://example.gitlab.com/api/scim/v2/groups/GROUP_NAME/Users?startIndex=1"' --header "Authorization: Bearer <your_scim_token>" --header "Content-Type: application/scim+json"
+```
+
+To see how this compares to the value returned as the SAML NameId, you can have the user use a [SAML Tracer](index.md#saml-debugging-tools).
+
+### Fix mismatched SCIM externalId and SAML NameId
+
+If GitLab's `externalId` doesn't match the SAML NameId, it will need to be updated in order for the user to log in. Ideally your identity provider will be configured to do such an update, but in some cases it may be unable to do so, such as when looking up a user fails due to an ID change.
+
+Fixing the fields your SCIM identity provider sends as `id` and `externalId` can correct this, however we use these IDs to look up users so if the identity provider is unaware of the current values for these it may try to create new duplicate users instead.
+
+If the `externalId` we have stored for a user has an incorrect value that doesn't match the SAML NameId, then it can be corrected with the manual use of the SCIM API.
+
+The [SCIM API](../../../api/scim.md#update-a-single-saml-user) can be used to manually correct the `externalId` stored for users so that it matches the SAML NameId. You'll need to know the desired value that matches the `NameId` as well as the current `externalId` to look up the user.
+
+It is then possible to issue a manual SCIM#update request, for example:
+
+```sh
+curl --verbose --request PATCH 'https://gitlab.com/api/scim/v2/groups/YOUR_GROUP/Users/OLD_EXTERNAL_UID' --data '{ "Operations": [{"op":"Replace","path":"externalId","value":"NEW_EXTERNAL_UID"}] }' --header "Authorization: Bearer <your_scim_token>" --header "Content-Type: application/scim+json"
+```
+
+It is important not to update these to incorrect values, since this will cause users to be unable to sign in. It is also important not to assign a value to the wrong user, as this would cause users to get signed into the wrong account.

doc/user/project/clusters/add_remove_clusters.md

 # Adding and removing Kubernetes clusters
-GitLab can integrate with the following Kubernetes providers:
+GitLab offers integrated cluster creation for the following Kubernetes providers:
 - Google Kubernetes Engine (GKE).
 - Amazon Elastic Kubernetes Service (EKS).
+In addition, GitLab can integrate with any standard Kubernetes provider, either on-premise or hosted.
+
 TIP: **Tip:**
 Every new Google Cloud Platform (GCP) account receives [$300 in credit upon sign up](https://console.cloud.google.com/freetrial),
 and in partnership with Google, GitLab is able to offer an additional $200 for new GCP accounts to get started with GitLab's
@@ -360,18 +362,20 @@ to install some [pre-defined applications](index.md#installing-applications).
 ## Add existing cluster
-If you have either of the following types of clusters already, you can add them to a project:
-- [Google Kubernetes Engine cluster](#existing-gke-cluster).
-- [Amazon Elastic Kubernetes Service](#existing-eks-cluster).
+If you have an existing Kubernetes cluster, you can add it to a project, group, or instance.
+
+For more information, see information for adding an:
+- [Existing Kubernetes cluster](#existing-kubernetes-cluster).
+- [Existing Elastic Kubernetes Service cluster](#existing-eks-cluster).
 NOTE: **Note:**
 Kubernetes integration is not supported for arm64 clusters. See the issue
 [Helm Tiller fails to install on arm64 cluster](https://gitlab.com/gitlab-org/gitlab-foss/issues/64044) for details.
-### Existing GKE cluster
-To add an existing GKE cluster to your project, group, or instance:
+### Existing Kubernetes cluster
+To add a Kubernetes cluster to your project, group, or instance:
 1. Navigate to your:
    - Project's **Operations > Kubernetes** page, for a project-level cluster.

doc/user/project/clusters/index.md

@@ -96,8 +96,12 @@ to the first container in the first pod of your environment.
 ## Adding and removing clusters
-See [Adding and removing Kubernetes clusters](add_remove_clusters.md) for details on how to
-set up integrations with Google Cloud Platform (GCP) and Amazon Elastic Kubernetes Service (EKS).
+See [Adding and removing Kubernetes clusters](add_remove_clusters.md) for details on how
+to:
+
+- Create a cluster in Google Cloud Platform (GCP) or Amazon Elastic Kubernetes Service
+  (EKS) using GitLab's UI.
+- Add an integration to an existing cluster from any Kubernetes platform.
 ## Cluster configuration

doc/user/project/clusters/serverless/index.md

@@ -170,53 +170,6 @@ You must do the following:
    or [serverless applications](#deploying-serverless-applications) onto your
    cluster.
-## Configuring logging
-
-> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/33330) in GitLab 12.5.
-
-### Prerequisites
-
-- A GitLab-managed cluster.
-- `kubectl` installed and working.
-
-Running `kubectl` commands on your cluster requires setting up access to the
-cluster first. For clusters created on:
-
-- GKE, see [GKE Cluster Access](https://cloud.google.com/kubernetes-engine/docs/how-to/cluster-access-for-kubectl)
-- Other platforms, see [Install and Set Up kubectl](https://kubernetes.io/docs/tasks/tools/install-kubectl/).
-
-### Enable request log template
-
-Run the following command to enable request logs:
-
-```shell
-kubectl edit cm -n knative-serving config-observability
-```
-
-Copy the `logging.request-log-template` from the `data._example` field to the data field one level up in the hierarchy.
-
-### Enable request logs
-
-Run the following commands to install Elasticsearch, Kibana, and Filebeat into a `kube-logging` namespace and configure all nodes to forward logs using Filebeat:
-
-```shell
-kubectl apply -f https://gitlab.com/gitlab-org/serverless/configurations/knative/raw/v0.7.0/kube-logging-filebeat.yaml
-kubectl label nodes --all beta.kubernetes.io/filebeat-ready="true"
-```
-
-### Viewing request logs
-
-To view request logs:
-
-1. Run `kubectl proxy`.
-1. Navigate to Kibana UI.
-
-Or:
-
-1. Open the Kibana UI.
-1. Click on **Discover**, then select `filebeat-*` from the dropdown on the left.
-1. Enter `kubernetes.container.name:"queue-proxy" AND message:/httpRequest/` into the search box.
-
 ## Supported runtimes
 Serverless functions for GitLab can be run using:
@@ -559,6 +512,53 @@ deployment. Copy and paste the domain into your browser to see the app live.
 ![knative app](img/knative-app.png)
+## Configuring logging
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/33330) in GitLab 12.5.
+
+### Prerequisites
+
+- A GitLab-managed cluster.
+- `kubectl` installed and working.
+
+Running `kubectl` commands on your cluster requires setting up access to the
+cluster first. For clusters created on:
+
+- GKE, see [GKE Cluster Access](https://cloud.google.com/kubernetes-engine/docs/how-to/cluster-access-for-kubectl)
+- Other platforms, see [Install and Set Up kubectl](https://kubernetes.io/docs/tasks/tools/install-kubectl/).
+
+### Enable request log template
+
+Run the following command to enable request logs:
+
+```shell
+kubectl edit cm -n knative-serving config-observability
+```
+
+Copy the `logging.request-log-template` from the `data._example` field to the data field one level up in the hierarchy.
+
+### Enable request logs
+
+Run the following commands to install Elasticsearch, Kibana, and Filebeat into a `kube-logging` namespace and configure all nodes to forward logs using Filebeat:
+
+```shell
+kubectl apply -f https://gitlab.com/gitlab-org/serverless/configurations/knative/raw/v0.7.0/kube-logging-filebeat.yaml
+kubectl label nodes --all beta.kubernetes.io/filebeat-ready="true"
+```
+
+### Viewing request logs
+
+To view request logs:
+
+1. Run `kubectl proxy`.
+1. Navigate to [Kibana UI](http://localhost:8001/api/v1/namespaces/kube-logging/services/kibana/proxy/app/kibana).
+
+Or:
+
+1. Open the [Kibana UI](http://localhost:8001/api/v1/namespaces/kube-logging/services/kibana/proxy/app/kibana).
+1. Click on **Discover**, then select `filebeat-*` from the dropdown on the left.
+1. Enter `kubernetes.container.name:"queue-proxy" AND message:/httpRequest/` into the search box.
+
 ## Function details
 Go to the **Operations > Serverless** page and click on one of the function

lib/feature/gitaly.rb

@@ -9,7 +9,6 @@ class Feature
       %w[
         cache_invalidator
         inforef_uploadpack_cache
-        filter_shas_with_signatures_go
         commit_without_batch_check
       ].freeze

spec/requests/api/internal/base_spec.rb

@@ -326,7 +326,7 @@ describe API::Internal::Base do
           expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
           expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
           expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-filter-shas-with-signatures-go' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true')
+          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true')
           expect(user.reload.last_activity_on).to eql(Date.today)
         end
       end
@@ -346,7 +346,7 @@ describe API::Internal::Base do
             expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
             expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
             expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-            expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-filter-shas-with-signatures-go' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true')
+            expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true')
             expect(user.reload.last_activity_on).to be_nil
           end
         end
@@ -594,7 +594,7 @@ describe API::Internal::Base do
           expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
           expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
           expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-filter-shas-with-signatures-go' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true')
+          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true')
         end
       end

spec/services/projects/destroy_rollback_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::DestroyRollbackService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:repository) { project.repository }
+  let(:repository_storage) { project.repository_storage }
+
+  subject { described_class.new(project, user, {}).execute }
+
+  describe '#execute' do
+    let(:path) { repository.disk_path + '.git' }
+    let(:removal_path) { "#{repository.disk_path}+#{project.id}#{Repositories::DestroyService::DELETED_FLAG}.git" }
+
+    before do
+      aggregate_failures do
+        expect(TestEnv.storage_dir_exists?(repository_storage, path)).to be_truthy
+        expect(TestEnv.storage_dir_exists?(repository_storage, removal_path)).to be_falsey
+      end
+
+      # Don't run sidekiq to check if renamed repository exists
+      Sidekiq::Testing.fake! { destroy_project(project, user, {}) }
+
+      aggregate_failures do
+        expect(TestEnv.storage_dir_exists?(repository_storage, path)).to be_falsey
+        expect(TestEnv.storage_dir_exists?(repository_storage, removal_path)).to be_truthy
+      end
+    end
+
+    it 'restores the repositories' do
+      Sidekiq::Testing.fake! { subject }
+
+      aggregate_failures do
+        expect(TestEnv.storage_dir_exists?(repository_storage, path)).to be_truthy
+        expect(TestEnv.storage_dir_exists?(repository_storage, removal_path)).to be_falsey
+      end
+    end
+  end
+
+  def destroy_project(project, user, params = {})
+    Projects::DestroyService.new(project, user, params).execute
+  end
+end

spec/services/projects/destroy_service_spec.rb

@@ -5,15 +5,11 @@ require 'spec_helper'
 describe Projects::DestroyService do
   include ProjectForksHelper
-  let!(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
   let!(:project) { create(:project, :repository, namespace: user.namespace) }
-  let!(:path) do
-    Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-      project.repository.path_to_repo
-    end
-  end
-  let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") }
-  let!(:async) { false } # execute or async_execute
+  let(:path) { project.repository.disk_path }
+  let(:remove_path) { removal_path(path) }
+  let(:async) { false } # execute or async_execute
   before do
     stub_container_registry_config(enabled: true)
@@ -21,7 +17,12 @@ describe Projects::DestroyService do
   end
   shared_examples 'deleting the project' do
-    it 'deletes the project' do
+    before do
+      # Run sidekiq immediately to check that renamed repository will be removed
+      destroy_project(project, user, {})
+    end
+
+    it 'deletes the project', :sidekiq_inline do
       expect(Project.unscoped.all).not_to include(project)
       expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
@@ -30,16 +31,10 @@ describe Projects::DestroyService do
   end
   shared_examples 'deleting the project with pipeline and build' do
-    context 'with pipeline and build' do # which has optimistic locking
+    context 'with pipeline and build', :sidekiq_inline do # which has optimistic locking
       let!(:pipeline) { create(:ci_pipeline, project: project) }
       let!(:build) { create(:ci_build, :artifacts, pipeline: pipeline) }
-      before do
-        perform_enqueued_jobs do
-          destroy_project(project, user, {})
-        end
-      end
-
       it_behaves_like 'deleting the project'
     end
   end
@@ -47,66 +42,63 @@ describe Projects::DestroyService do
   shared_examples 'handles errors thrown during async destroy' do |error_message|
     it 'does not allow the error to bubble up' do
       expect do
-        perform_enqueued_jobs { destroy_project(project, user, {}) }
+        destroy_project(project, user, {})
       end.not_to raise_error
     end
     it 'unmarks the project as "pending deletion"' do
-      perform_enqueued_jobs { destroy_project(project, user, {}) }
+      destroy_project(project, user, {})
       expect(project.reload.pending_delete).to be(false)
     end
     it 'stores an error message in `projects.delete_error`' do
-      perform_enqueued_jobs { destroy_project(project, user, {}) }
+      destroy_project(project, user, {})
       expect(project.reload.delete_error).to be_present
       expect(project.delete_error).to include(error_message)
     end
   end
-  context 'Sidekiq inline' do
-    before do
-      # Run sidekiq immediately to check that renamed repository will be removed
-      perform_enqueued_jobs { destroy_project(project, user, {}) }
-    end
-    it_behaves_like 'deleting the project'
-    context 'when has remote mirrors' do
-      let!(:project) do
-        create(:project, :repository, namespace: user.namespace).tap do |project|
-          project.remote_mirrors.create(url: 'http://test.com')
-        end
-      end
-      let!(:async) { true }
-      it 'destroys them', :sidekiq_might_not_need_inline do
-        expect(RemoteMirror.count).to eq(0)
+  it_behaves_like 'deleting the project'
+  it 'invalidates personal_project_count cache' do
+    expect(user).to receive(:invalidate_personal_projects_count)
+    destroy_project(project, user, {})
+  end
+  context 'when project has remote mirrors' do
+    let!(:project) do
+      create(:project, :repository, namespace: user.namespace).tap do |project|
+        project.remote_mirrors.create(url: 'http://test.com')
       end
     end
-    it 'invalidates personal_project_count cache' do
-      expect(user).to receive(:invalidate_personal_projects_count)
-      destroy_project(project, user)
+    it 'destroys them' do
+      expect(RemoteMirror.count).to eq(1)
+      destroy_project(project, user, {})
+
+      expect(RemoteMirror.count).to eq(0)
     end
-    context 'when project has exports' do
-      let!(:project_with_export) do
-        create(:project, :repository, namespace: user.namespace).tap do |project|
-          create(:import_export_upload,
-                 project: project,
-                 export_file: fixture_file_upload('spec/fixtures/project_export.tar.gz'))
-        end
+  end
+  context 'when project has exports' do
+    let!(:project_with_export) do
+      create(:project, :repository, namespace: user.namespace).tap do |project|
+        create(:import_export_upload,
+               project: project,
+               export_file: fixture_file_upload('spec/fixtures/project_export.tar.gz'))
       end
-      let!(:async) { true }
-      it 'destroys project and export', :sidekiq_might_not_need_inline do
-        expect { destroy_project(project_with_export, user) }.to change(ImportExportUpload, :count).by(-1)
-        expect(Project.all).not_to include(project_with_export)
-      end
+    end
+    it 'destroys project and export' do
+      expect do
+        destroy_project(project_with_export, user, {})
+      end.to change(ImportExportUpload, :count).by(-1)
+      expect(Project.all).not_to include(project_with_export)
     end
   end
@@ -117,20 +109,24 @@ describe Projects::DestroyService do
     end
     it { expect(Project.all).not_to include(project) }
-    it { expect(Dir.exist?(path)).to be_falsey }
-    it { expect(Dir.exist?(remove_path)).to be_truthy }
+
+    it do
+      expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+    end
+
+    it do
+      expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_truthy
+    end
   end
   context 'when flushing caches fail due to Git errors' do
     before do
       allow(project.repository).to receive(:before_delete).and_raise(::Gitlab::Git::CommandError)
       allow(Gitlab::GitLogger).to receive(:warn).with(
-        class: described_class.name,
+        class: Repositories::DestroyService.name,
         project_id: project.id,
         disk_path: project.disk_path,
         message: 'Gitlab::Git::CommandError').and_call_original
-
-      perform_enqueued_jobs { destroy_project(project, user, {}) }
     end
     it_behaves_like 'deleting the project'
@@ -153,14 +149,12 @@ describe Projects::DestroyService do
     end
   end
-  context 'with async_execute', :sidekiq_might_not_need_inline do
+  context 'with async_execute', :sidekiq_inline do
     let(:async) { true }
     context 'async delete of project with private issue visibility' do
       before do
         project.project_feature.update_attribute("issues_access_level", ProjectFeature::PRIVATE)
-        # Run sidekiq immediately to check that renamed repository will be removed
-        perform_enqueued_jobs { destroy_project(project, user, {}) }
       end
       it_behaves_like 'deleting the project'
@@ -204,7 +198,7 @@ describe Projects::DestroyService do
         it 'allows error to bubble up and rolls back project deletion' do
           expect do
-            perform_enqueued_jobs { destroy_project(project, user, {}) }
+            destroy_project(project, user, {})
           end.to raise_error(Exception, 'Other error message')
           expect(project.reload.pending_delete).to be(false)
@@ -312,15 +306,12 @@ describe Projects::DestroyService do
   end
   context 'repository +deleted path removal' do
-    def removal_path(path)
-      "#{path}+#{project.id}#{described_class::DELETED_FLAG}"
-    end
-
     context 'regular phase' do
       it 'schedules +deleted removal of existing repos' do
         service = described_class.new(project, user, {})
         allow(service).to receive(:schedule_stale_repos_removal)
+        expect(Repositories::ShellDestroyService).to receive(:new).and_call_original
         expect(GitlabShellWorker).to receive(:perform_in)
           .with(5.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path))
@@ -329,14 +320,16 @@ describe Projects::DestroyService do
     end
     context 'stale cleanup' do
-      let!(:async) { true }
+      let(:async) { true }
       it 'schedules +deleted wiki and repo removal' do
         allow(ProjectDestroyWorker).to receive(:perform_async)
+        expect(Repositories::ShellDestroyService).to receive(:new).with(project.repository).and_call_original
         expect(GitlabShellWorker).to receive(:perform_in)
           .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path))
+        expect(Repositories::ShellDestroyService).to receive(:new).with(project.wiki.repository).and_call_original
         expect(GitlabShellWorker).to receive(:perform_in)
           .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.wiki.disk_path))
@@ -345,33 +338,11 @@ describe Projects::DestroyService do
     end
   end
-  context '#attempt_restore_repositories' do
-    let(:path) { project.disk_path + '.git' }
-
-    before do
-      expect(TestEnv.storage_dir_exists?(project.repository_storage, path)).to be_truthy
-      expect(TestEnv.storage_dir_exists?(project.repository_storage, remove_path)).to be_falsey
-
-      # Dont run sidekiq to check if renamed repository exists
-      Sidekiq::Testing.fake! { destroy_project(project, user, {}) }
-
-      expect(TestEnv.storage_dir_exists?(project.repository_storage, path)).to be_falsey
-      expect(TestEnv.storage_dir_exists?(project.repository_storage, remove_path)).to be_truthy
-    end
-
-    it 'restores the repositories' do
-      Sidekiq::Testing.fake! { described_class.new(project, user).attempt_repositories_rollback }
-
-      expect(TestEnv.storage_dir_exists?(project.repository_storage, path)).to be_truthy
-      expect(TestEnv.storage_dir_exists?(project.repository_storage, remove_path)).to be_falsey
-    end
+  def destroy_project(project, user, params = {})
+    described_class.new(project, user, params).public_send(async ? :async_execute : :execute)
   end
-  def destroy_project(project, user, params = {})
-    if async
-      Projects::DestroyService.new(project, user, params).async_execute
-    else
-      Projects::DestroyService.new(project, user, params).execute
-    end
+  def removal_path(path)
+    "#{path}+#{project.id}#{Repositories::DestroyService::DELETED_FLAG}"
   end
 end

spec/services/repositories/destroy_rollback_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Repositories::DestroyRollbackService do
+  let_it_be(:user) { create(:user) }
+  let!(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:repository) { project.repository }
+  let(:path) { repository.disk_path }
+  let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" }
+
+  subject { described_class.new(repository).execute }
+
+  before do
+    # Dont run sidekiq to check if renamed repository exists
+    Sidekiq::Testing.fake! { destroy_project(project, user) }
+  end
+
+  it 'moves the repository from the +deleted folder' do
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_truthy
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+
+    subject
+
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy
+  end
+
+  it 'logs the successful action' do
+    expect(Gitlab::AppLogger).to receive(:info)
+
+    subject
+  end
+
+  it 'flushes the repository cache' do
+    expect(repository).to receive(:before_delete)
+
+    subject
+  end
+
+  it 'returns success and does not perform any action if repository path does not exist' do
+    expect(repository).to receive(:disk_path).and_return('foo')
+    expect(repository).not_to receive(:before_delete)
+
+    result = subject
+
+    expect(result[:status]).to eq :success
+  end
+
+  context 'when move operation cannot be performed' do
+    let(:service) { described_class.new(repository) }
+
+    before do
+      allow(service).to receive(:mv_repository).and_return(false)
+    end
+
+    it 'returns error' do
+      result = service.execute
+
+      expect(result[:status]).to eq :error
+    end
+
+    it 'logs the error' do
+      expect(Gitlab::AppLogger).to receive(:error)
+
+      service.execute
+    end
+  end
+
+  def destroy_project(project, user)
+    Projects::DestroyService.new(project, user, {}).execute
+  end
+end

spec/services/repositories/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Repositories::DestroyService do
+  let_it_be(:user) { create(:user) }
+  let!(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:repository) { project.repository }
+  let(:path) { repository.disk_path }
+  let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" }
+
+  subject { described_class.new(project.repository).execute }
+
+  it 'moves the repository to a +deleted folder' do
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+
+    subject
+
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_truthy
+  end
+
+  it 'schedules the repository deletion' do
+    subject
+
+    expect(Repositories::ShellDestroyService).to receive(:new).with(repository).and_call_original
+
+    expect(GitlabShellWorker).to receive(:perform_in)
+      .with(Repositories::ShellDestroyService::REPO_REMOVAL_DELAY, :remove_repository, project.repository_storage, remove_path)
+
+    # Because GitlabShellWorker is inside a run_after_commit callback we need to
+    # trigger the callback
+    project.touch
+  end
+
+  it 'removes the repository', :sidekiq_inline do
+    subject
+
+    project.touch
+
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+  end
+
+  it 'flushes the repository cache' do
+    expect(repository).to receive(:before_delete)
+
+    subject
+  end
+
+  it 'does not perform any action if repository path does not exist and returns success' do
+    expect(repository).to receive(:disk_path).and_return('foo')
+    expect(repository).not_to receive(:before_delete)
+
+    result = subject
+
+    expect(result[:status]).to eq :success
+  end
+
+  context 'when move operation cannot be performed' do
+    let(:service) { described_class.new(repository) }
+
+    before do
+      allow(service).to receive(:mv_repository).and_return(false)
+    end
+
+    it 'returns error' do
+      result = service.execute
+
+      expect(result[:status]).to eq :error
+    end
+
+    it 'logs the error' do
+      expect(Gitlab::AppLogger).to receive(:error)
+
+      service.execute
+    end
+  end
+end

spec/services/repositories/shell_destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Repositories::ShellDestroyService do
+  let_it_be(:user) { create(:user) }
+  let!(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:path) { project.repository.disk_path }
+  let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" }
+
+  it 'returns success if the repository is nil' do
+    expect(GitlabShellWorker).not_to receive(:perform_in)
+
+    result = described_class.new(nil).execute
+
+    expect(result[:status]).to eq :success
+  end
+
+  it 'schedules the repository deletion' do
+    expect(GitlabShellWorker).to receive(:perform_in)
+      .with(described_class::REPO_REMOVAL_DELAY, :remove_repository, project.repository_storage, remove_path)
+
+    described_class.new(project.repository).execute
+  end
+end