Automatic merge of gitlab-org/gitlab-ce master

changelogs/unreleased/jc-detect-nfs-for-rugged.yml

+---
+title: Use Rugged if we detect storage is NFS and we can access the disk
+merge_request: 29725
+author:
+type: performance

doc/ci/review_apps/index.md

@@ -27,7 +27,7 @@ In the above example:
 - Once the review as passed, `topic branch` is merged into `master` where it's deploy to staging.
 - After been approved in staging, the changes that were merged into `master` are deployed in to production.
 
-### How Review Apps work
+## How Review Apps work
 
 A Review App is a mapping of a branch with an [environment](../environments.md).
 Access to the Review App is made available as a link on the [merge request](../../user/project/merge_requests.md) relevant to the branch.
@@ -41,27 +41,34 @@ In this example, a branch was:
 - Successfully built.
 - Deployed under a dynamic environment that can be reached by clicking on the **View app** button.
 
+After adding Review Apps to your workflow, you follow the branched Git flow. That is:
+
+1. Push a branch and let the Runner deploy the Review App based on the `script` definition of the dynamic environment job.
+1. Wait for the Runner to build and deploy your web application.
+1. Click on the link provided in the merge request related to the branch to see the changes live.
+
 ## Configuring Review Apps
 
 Review Apps are built on [dynamic environments](../environments.md#configuring-dynamic-environments), which allow you to dynamically create a new environment for each branch.
 
 The process of configuring Review Apps is as follows:
 
-1. Set up the infrastructure to host and deploy the Review Apps.
+1. Set up the infrastructure to host and deploy the Review Apps (check the [examples](#review-apps-examples) below).
 1. [Install](https://docs.gitlab.com/runner/install/) and [configure](https://docs.gitlab.com/runner/commands/) a Runner to do deployment.
 1. Set up a job in `.gitlab-ci.yml` that uses the [predefined CI environment variable](../variables/README.md) `${CI_COMMIT_REF_NAME}` to create dynamic environments and restrict it to run only on branches.
 1. Optionally, set a job that [manually stops](../environments.md#stopping-an-environment) the Review Apps.
 
-### Examples
+## Review Apps examples
 
 The following are example projects that demonstrate Review App configuration:
 
 - [NGINX](https://gitlab.com/gitlab-examples/review-apps-nginx).
 - [OpenShift](https://gitlab.com/gitlab-examples/review-apps-openshift).
 
+<i class="fa fa-youtube-play youtube" aria-hidden="true"></i>
 See also the video [Demo: Cloud Native Development with GitLab](https://www.youtube.com/watch?v=jfIyQEwrocw), which includes a Review Apps example.
 
-### Route Maps
+## Route Maps
 
 > Introduced in GitLab 8.17. In GitLab 11.5, the file links are available in the merge request widget.
 
@@ -82,7 +89,7 @@ To set up a route map, add a a file inside the repository at `.gitlab/route-map.
 which contains a YAML array that maps `source` paths (in the repository) to `public`
 paths (on the website).
 
-#### Route Maps example
+### Route Maps example
 
 The following is an example of a route map for [Middleman](https://middlemanapp.com),
 a static site generator (SSG) used to build [GitLab's website](https://about.gitlab.com),
@@ -146,51 +153,102 @@ Once you have the route mapping set up, it will take effect in the following loc
 
     !["View on env" button in file view](img/view_on_env_blob.png)
 
-## Working with Review Apps
-
-After adding Review Apps to your workflow, you follow the branched Git flow. That is:
-
-1. Push a branch and let the Runner deploy the Review App based on the `script` definition of the dynamic environment job.
-1. Wait for the Runner to build and deploy your web application.
-1. Click on the link that provided in the merge request related to the branch to see the changes live.
-
-### Visual Reviews **(STARTER)**
+## Visual Reviews **(STARTER)**
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/10761) in GitLab Starter 12.0.
 
-The Visual Reviews feedback form can be added to a Review App to enable reviewers to post comments
-directly from the app back to the merge request that spawned the Review App.
+With Visual Reviews, you can provide a feedback form to your Review Apps so
+that reviewers can post comments directly from the app back to the merge request
+that spawned the Review App.
 
-For example, a form like the following can be configured to post the contents of the
-text field into the discussion thread of a merge request:
+### Configuring Visual Reviews
 
-![feedback form](img/toolbar_feeback_form.png)
+The feedback form is served through a script you add to pages in your Review App.
+If you have [Developer permissions](../../user/permissions.md) to the project,
+you can access it by clicking the **Review** button in the **Pipeline** section
+of the merge request.
 
-#### Using Visual Reviews
+![review button](img/review_button.png)
 
-If Visual Reviews has been [enabled](#configuring-visual-reviews) for the Review App, the Visual Reviews feedback form is overlaid on the app's pages at the bottom-right corner.
+The provided script should be added to the `<head>` of you application and
+consists of some project and merge request specific values. Here's what it
+looks like:
+
+```html
+<script
+  data-project-id='11790219'
+  data-merge-request-id='1'
+  data-mr-url='https://gitlab.example.com'
+  data-project-path='sarah/review-app-tester'
+  id='review-app-toolbar-script'
+  src='https://gitlab.example.com/assets/webpack/visual_review_toolbar.js'>
+</script>
+```
 
-To use the feedback form, you will need to create a [personal access token](../../user/profile/personal_access_tokens.md) with the API scope selected.
+Ideally, you should use [environment variables](../variables/predefined_variables.md)
+to replace those values at runtime when each review app is created:
+
+- `data-project-id` is the project ID, which can be found by the `CI_PROJECT_ID`
+  variable.
+- `data-merge-request-id` is the merge request ID, which can be found by the
+  `CI_MERGE_REQUEST_IID` variable. `CI_MERGE_REQUEST_IID` is available only if
+  [`only: [merge_requests]`](../merge_request_pipelines/index.md)
+  is used and the merge request is created.
+- `data-mr-url` is the URL of the GitLab instance and will be the same for all
+  review apps.
+- `data-project-path` is the project's path, which can be found by `CI_PROJECT_PATH`.
+- `id` is always `review-app-toolbar-script`, you don't need to change that.
+- `src` is the source of the review toolbar script, which resides in the
+  respective GitLab instance and will be the same for all review apps.
+
+For example, in a Ruby application, you would need to have this script:
+
+```html
+<script
+  data-project-id="ENV['CI_PROJECT_ID']"
+  data-merge-request-id="ENV['CI_MERGE_REQUEST_IID']"
+  data-mr-url='https://gitlab.example.com'
+  data-project-path="ENV['CI_PROJECT_PATH']"
+  id='review-app-toolbar-script'
+  src='https://gitlab.example.com/assets/webpack/visual_review_toolbar.js'>
+</script>
+```
 
-Paste the token into the feedback box, when prompted. If you select **Remember me**, your browser stores the token so that future visits to Review Apps at the same URL will not require you to re-enter the token. To clear the token, click **Log out**.
+Then, when your app is deployed via GitLab CI/CD, those variables should get
+replaced with their real values.
 
-Because tokens must be entered on a per-domain basis and they can only be accessed once, you can save the token to your password manager specifically for the purpose of Visual Reviews. This way, you will not need to create additional tokens for each merge request.
+NOTE: **Note:**
+Future enhancements [are planned](https://gitlab.com/gitlab-org/gitlab-ee/issues/11322)
+to make this process even easier.
 
-Comments can make use of all the [Markdown annotations](../../user/markdown.md)
-available in merge request comment boxes.
+### Using Visual Reviews
 
-#### Configuring Visual Reviews
+After Visual Reviews has been [enabled](#configuring-visual-reviews) for the
+Review App, the Visual Reviews feedback form is overlaid on the app's pages at
+the bottom-right corner.
 
-The feedback form is served through a script you add to pages in your Review App.
-To access the code to include the script, click the **Review** button in the **Pipeline** section of the merge request.
+![Visual review feedback form](img/toolbar_feeback_form.png)
 
-![review button](img/review_button.png)
+To use the feedback form:
+
+1. Create a [personal access token](../../user/profile/personal_access_tokens.md)
+   with the API scope selected.
+1. Paste the token into the feedback box when prompted. If you select **Remember me**,
+   your browser stores the token so that future visits to Review Apps at the same URL
+   will not require you to re-enter the token. To clear the token, click **Log out**.
+1. Make a comment on the visual review. You can make use of all the
+   [Markdown annotations](../../user/markdown.md) that are also available in
+   merge request comments.
+1. Finally, click **Send feedback**.
 
-The provided script hardcodes the project and merge request IDs. You may want to consider
-using features of your programming language to use environment variables or other
-means to inject these at runtime.
+After you make and submit a comment in the visual review box, it will appear
+automatically in the respective merge request.
 
-Future enhancements [are planned](https://gitlab.com/gitlab-org/gitlab-ee/issues/11322) to make this process even easier.
+TIP: **Tip:**
+Because tokens must be entered on a per-domain basis and they can only be accessed
+once, different review apps will not remember your token. You can save the token
+to your password manager specifically for the purpose of Visual Reviews. This way,
+you will not need to create additional tokens for each merge request.
 
 ## Limitations
 

lib/gitlab/git/rugged_impl/blob.rb

@@ -11,10 +11,11 @@ module Gitlab
       module Blob
         module ClassMethods
           extend ::Gitlab::Utils::Override
+          include Gitlab::Git::RuggedImpl::UseRugged
 
           override :tree_entry
           def tree_entry(repository, sha, path, limit)
-            if Feature.enabled?(:rugged_tree_entry)
+            if use_rugged?(repository, :rugged_tree_entry)
               rugged_tree_entry(repository, sha, path, limit)
             else
               super

lib/gitlab/git/rugged_impl/commit.rb

@@ -12,6 +12,7 @@ module Gitlab
       module Commit
         module ClassMethods
           extend ::Gitlab::Utils::Override
+          include Gitlab::Git::RuggedImpl::UseRugged
 
           def rugged_find(repo, commit_id)
             obj = repo.rev_parse_target(commit_id)
@@ -34,7 +35,7 @@ module Gitlab
 
           override :find_commit
           def find_commit(repo, commit_id)
-            if Feature.enabled?(:rugged_find_commit)
+            if use_rugged?(repo, :rugged_find_commit)
               rugged_find(repo, commit_id)
             else
               super
@@ -43,7 +44,7 @@ module Gitlab
 
           override :batch_by_oid
           def batch_by_oid(repo, oids)
-            if Feature.enabled?(:rugged_list_commits_by_oid)
+            if use_rugged?(repo, :rugged_list_commits_by_oid)
               rugged_batch_by_oid(repo, oids)
             else
               super
@@ -52,6 +53,7 @@ module Gitlab
         end
 
         extend ::Gitlab::Utils::Override
+        include Gitlab::Git::RuggedImpl::UseRugged
 
         override :init_commit
         def init_commit(raw_commit)
@@ -65,7 +67,7 @@ module Gitlab
 
         override :commit_tree_entry
         def commit_tree_entry(path)
-          if Feature.enabled?(:rugged_commit_tree_entry)
+          if use_rugged?(@repository, :rugged_commit_tree_entry)
             rugged_tree_entry(path)
           else
             super

lib/gitlab/git/rugged_impl/repository.rb

@@ -11,6 +11,7 @@ module Gitlab
     module RuggedImpl
       module Repository
         extend ::Gitlab::Utils::Override
+        include Gitlab::Git::RuggedImpl::UseRugged
 
         FEATURE_FLAGS = %i(rugged_find_commit rugged_tree_entries rugged_tree_entry rugged_commit_is_ancestor rugged_commit_tree_entry rugged_list_commits_by_oid).freeze
 
@@ -46,7 +47,7 @@ module Gitlab
 
         override :ancestor?
         def ancestor?(from, to)
-          if Feature.enabled?(:rugged_commit_is_ancestor)
+          if use_rugged?(self, :rugged_commit_is_ancestor)
             rugged_is_ancestor?(from, to)
           else
             super

lib/gitlab/git/rugged_impl/tree.rb

@@ -11,10 +11,11 @@ module Gitlab
       module Tree
         module ClassMethods
           extend ::Gitlab::Utils::Override
+          include Gitlab::Git::RuggedImpl::UseRugged
 
           override :tree_entries
           def tree_entries(repository, sha, path, recursive)
-            if Feature.enabled?(:rugged_tree_entries)
+            if use_rugged?(repository, :rugged_tree_entries)
               tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive)
             else
               super

lib/gitlab/git/rugged_impl/use_rugged.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Git
+    module RuggedImpl
+      module UseRugged
+        def use_rugged?(repo, feature_key)
+          feature = Feature.get(feature_key)
+          return feature.enabled? if Feature.persisted?(feature)
+
+          Gitlab::GitalyClient.can_use_disk?(repo.storage)
+        end
+      end
+    end
+  end
+end

lib/gitlab/gitaly_client.rb

@@ -30,6 +30,7 @@ module Gitlab
     SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'
     MAXIMUM_GITALY_CALLS = 30
     CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze
+    GITALY_METADATA_FILENAME = '.gitaly-metadata'
 
     MUTEX = Mutex.new
 
@@ -378,6 +379,45 @@ module Gitlab
       0
     end
 
+    def self.storage_metadata_file_path(storage)
+      Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+        File.join(
+          Gitlab.config.repositories.storages[storage].legacy_disk_path, GITALY_METADATA_FILENAME
+        )
+      end
+    end
+
+    def self.can_use_disk?(storage)
+      cached_value = MUTEX.synchronize do
+        @can_use_disk ||= {}
+        @can_use_disk[storage]
+      end
+
+      return cached_value unless cached_value.nil?
+
+      gitaly_filesystem_id = filesystem_id(storage)
+      direct_filesystem_id = filesystem_id_from_disk(storage)
+
+      MUTEX.synchronize do
+        @can_use_disk[storage] = gitaly_filesystem_id.present? &&
+          gitaly_filesystem_id == direct_filesystem_id
+      end
+    end
+
+    def self.filesystem_id(storage)
+      response = Gitlab::GitalyClient::ServerService.new(storage).info
+      storage_status = response.storage_statuses.find { |status| status.storage_name == storage }
+      storage_status.filesystem_id
+    end
+
+    def self.filesystem_id_from_disk(storage)
+      metadata_file = File.read(storage_metadata_file_path(storage))
+      metadata_hash = JSON.parse(metadata_file)
+      metadata_hash['gitaly_filesystem_id']
+    rescue Errno::ENOENT, JSON::ParserError
+      nil
+    end
+
     def self.timeout(timeout_name)
       Gitlab::CurrentSettings.current_application_settings[timeout_name]
     end

spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb

@@ -26,6 +26,7 @@ describe Gitlab::Cache::Ci::ProjectPipelineStatus, :clean_gitlab_redis_cache do
       end
 
       it 'loads 10 projects without hitting Gitaly call limit', :request_store do
+        allow(Gitlab::GitalyClient).to receive(:can_access_disk?).and_return(false)
         projects = Gitlab::GitalyClient.allow_n_plus_1_calls do
           (1..10).map { create(:project, :repository) }
         end

spec/lib/gitlab/git/commit_spec.rb

@@ -408,7 +408,7 @@ describe Gitlab::Git::Commit, :seed_helper do
 
       context 'when oids is empty' do
         it 'makes no Gitaly request' do
-          expect(Gitlab::GitalyClient).not_to receive(:call)
+          expect(Gitlab::GitalyClient).not_to receive(:call).with(repository.storage, :commit_service, :list_commits_by_oid)
 
           described_class.batch_by_oid(repository, [])
         end

spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'json'
+require 'tempfile'
+
+describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
+  let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
+  let(:feature_flag_name) { 'feature-flag-name' }
+  let(:feature_flag) { Feature.get(feature_flag_name) }
+  let(:temp_gitaly_metadata_file) { create_temporary_gitaly_metadata_file }
+
+  before(:all) do
+    create_gitaly_metadata_file
+  end
+
+  subject(:wrapper) do
+    klazz = Class.new { include Gitlab::Git::RuggedImpl::UseRugged }
+    klazz.new
+  end
+
+  before do
+    Gitlab::GitalyClient.instance_variable_set(:@can_use_disk, {})
+  end
+
+  context 'when feature flag is not persisted' do
+    before do
+      allow(Feature).to receive(:persisted?).with(feature_flag).and_return(false)
+    end
+
+    it 'returns true when gitaly matches disk' do
+      expect(subject.use_rugged?(repository, feature_flag_name)).to be true
+    end
+
+    it 'returns false when disk access fails' do
+      allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return("/fake/path/doesnt/exist")
+
+      expect(subject.use_rugged?(repository, feature_flag_name)).to be false
+    end
+
+    it "returns false when gitaly doesn't match disk" do
+      allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return(temp_gitaly_metadata_file)
+
+      expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey
+
+      File.delete(temp_gitaly_metadata_file)
+    end
+
+    it "doesn't lead to a second rpc call because gitaly client should use the cached value" do
+      expect(subject.use_rugged?(repository, feature_flag_name)).to be true
+
+      expect(Gitlab::GitalyClient).not_to receive(:filesystem_id)
+
+      subject.use_rugged?(repository, feature_flag_name)
+    end
+  end
+
+  context 'when feature flag is persisted' do
+    before do
+      allow(Feature).to receive(:persisted?).with(feature_flag).and_return(true)
+    end
+
+    it 'returns false when the feature flag is off' do
+      allow(feature_flag).to receive(:enabled?).and_return(false)
+
+      expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey
+    end
+
+    it "returns true when feature flag is on" do
+      allow(feature_flag).to receive(:enabled?).and_return(true)
+      allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(false)
+
+      expect(subject.use_rugged?(repository, feature_flag_name)).to be true
+    end
+  end
+
+  def create_temporary_gitaly_metadata_file
+    tmp = Tempfile.new('.gitaly-metadata')
+    gitaly_metadata = {
+      "gitaly_filesystem_id" => "some-value"
+    }
+    tmp.write(gitaly_metadata.to_json)
+    tmp.flush
+    tmp.close
+    tmp.path
+  end
+
+  def create_gitaly_metadata_file
+    File.open(File.join(SEED_STORAGE_PATH, '.gitaly-metadata'), 'w+') do |f|
+      gitaly_metadata = {
+        "gitaly_filesystem_id" => SecureRandom.uuid
+      }
+      f.write(gitaly_metadata.to_json)
+    end
+  end
+end