Merge branch 'jliu/deprecate-git-data-dirs-path' into 'master'

gitaly: Deprecate `git_data_dirs` - v2

See merge request https://gitlab.com/gitlab-org/omnibus-gitlab/-/merge_requests/8091



Merged-by: Balasankar 'Balu' C <balasankar@gitlab.com>
Approved-by: Balasankar 'Balu' C <balasankar@gitlab.com>
Reviewed-by: Balasankar 'Balu' C <balasankar@gitlab.com>
Reviewed-by: James Liu <jliu@gitlab.com>
Reviewed-by: Emily Chui <echui@gitlab.com>
Co-authored-by: James Liu <jliu@gitlab.com>

doc/settings/configuration.md

@@ -154,27 +154,78 @@ In this example, the [PostgreSQL server certificate](database.md#configuring-ssl
 postgresql['internal_certificate'] = File.read('/path/to/server.crt')
 ```
+## Migrating from `git_data_dirs`
+
+Starting in 18.0, `git_data_dirs` will no longer be a supported means of configuring
+Gitaly storage locations. If you explicitly define `git_data_dirs`, you'll need to
+migrate the configuration.
+
+For example, if your `/etc/gitlab/gitlab.rb` configuration is as follows:
+
+```ruby
+git_data_dirs({
+  "default" => {
+    "path" => "/mnt/nas/git-data"
+   }
+})
+```
+
+you'll need to redefine the configuration under `gitaly['configuration']` instead.
+Note that the `/repositories` suffix must be appended to the path, as it was previously
+appended by Omnibus internally.
+
+```ruby
+gitaly['configuration'] = {
+  storage: [
+    {
+      name: 'default',
+      path: '/mnt/nas/git-data/repositories',
+    },
+  ],
+}
+```
+
+It's important to note that the parent directory of the `path` must also be managed by
+Omnibus. Following the example above, Omnibus will need to modify the permissions of
+`/mnt/nas/git-data` on reconfiguration and may store data in that directory during
+runtime. You should select an appropriate `path` that allows for this behavior.
+
 ## Store Git data in an alternative directory
 By default, Linux package installations store the Git repository data under
-`/var/opt/gitlab/git-data`. The repositories are stored in a subfolder called
-`repositories`.
-To change the location of the `git-data` parent directory:
+`/var/opt/gitlab/git-data/repositories`, and the Gitaly service listens on
+`unix:/var/opt/gitlab/gitaly/gitaly.socket`.
+To change the location of the directory,
 1. Edit `/etc/gitlab/gitlab.rb`:
    ```ruby
-   git_data_dirs({ "default" => { "path" => "/mnt/nas/git-data" } })
+   gitaly['configuration'] = {
+     storage: [
+       {
+         name: 'default',
+         path: '/mnt/nas/git-data/repositories',
+       },
+     ],
+   }
    ```
    You can also add more than one Git data directory:
    ```ruby
-   git_data_dirs({
-     "default" => { "path" => "/var/opt/gitlab/git-data" },
-     "alternative" => { "path" => "/mnt/nas/git-data" }
-   })
+   gitaly['configuration'] = {
+     storage: [
+       {
+         name: 'default',
+         path: '/var/opt/gitlab/git-data/repositories',
+       },
+       {
+         name: 'alternative',
+         path: '/mnt/nas/git-data/repositories',
+       },
+     ],
+   }
    ```
    The target directories and any of its subpaths must not be a symlink.
@@ -220,8 +271,7 @@ To change the location of the `git-data` parent directory:
       sudo gitlab-ctl start
       ```
-If you're running Gitaly on a separate server, remember to also include the
-`gitaly_address` for each Git data directory. See
+If you're running Gitaly on a separate server, see
 [the documentation on configuring Gitaly](https://docs.gitlab.com/ee/administration/gitaly/configure_gitaly.html#configure-gitaly-clients).
 If you're not looking to move all repositories, but instead want to move specific

doc/update/convert_to_omnibus.md

@@ -149,8 +149,21 @@ To convert an external PostgreSQL installation to a Linux package PostgreSQL ins
    # Use your own GitLab URL here
    external_url 'http://gitlab.example.com'
-   # We assume your repositories are in /home/git/repositories (default for source installs)
-   git_data_dirs({ 'default' => { 'path' => '/home/git' } })
+   # We assume your repositories are in /home/git/repositories (default for source installs) and that Gitaly
+   # listens on a socket at /home/git/gitlab/tmp/sockets/private/gitaly.socket
+   gitaly['configuration'] = {
+     storage: [
+       {
+         name: 'default',
+         path: '/home/git/repositories'
+       }
+     ]
+   }
+   gitlab_rails['repositories_storages'] = {
+     default: {
+       gitaly_address: '/home/git/gitlab/tmp/sockets/private/gitaly.socket'
+     }
+   }
    # Re-use the PostgreSQL that is already running on your system
    postgresql['enable'] = false

files/gitlab-config-template/gitlab.rb.template

@@ -171,7 +171,7 @@ external_url 'GENERATED_EXTERNAL_URL'
 ### Download location
 ###! When a user clicks e.g. 'Download zip' on a project, a temporary zip file
 ###! is created in the following directory.
-###! Should not be the same path, or a sub directory of any of the `git_data_dirs`
+###! Should not be the same path, or a sub directory of any of the `gitaly['configuration'].storage` paths.
 # gitlab_rails['gitlab_repository_downloads_path'] = 'tmp/repositories'
 ### Gravatar Settings
@@ -686,16 +686,6 @@ external_url 'GENERATED_EXTERNAL_URL'
 #    "SKIP" => "db,uploads,repositories,builds,artifacts,lfs,registry,pages"
 #}
-### For setting up different data storing directory
-###! Docs: https://docs.gitlab.com/omnibus/settings/configuration.html#store-git-data-in-an-alternative-directory
-###! **If you want to use a single non-default directory to store git data use a
-###!   path that doesn't contain symlinks.**
-# git_data_dirs({
-#   "default" => {
-#     "path" => "/mnt/nfs-01/git-data"
-#    }
-# })
-
 ### Gitaly settings
 # gitlab_rails['gitaly_token'] = 'secret token'

files/gitlab-cookbooks/gitaly/libraries/gitaly.rb

@@ -24,7 +24,6 @@ module Gitaly
     include OutputHelper
     def parse_variables
-      parse_git_data_dirs
       parse_gitaly_storages
       parse_gitconfig
       check_duplicate_storage_paths
@@ -55,37 +54,91 @@ module Gitaly
       end
     end
-    def parse_git_data_dirs
-      Gitlab['git_data_dirs'] = { "default" => { "path" => "/var/opt/gitlab/git-data" } } if Gitlab['git_data_dirs'].empty?
-      Gitlab['git_data_dirs'].map do |name, details|
-        Gitlab['git_data_dirs'][name]['path'] = details[:path] || details['path'] || '/var/opt/gitlab/git-data'
+    # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
+    def parse_gitaly_storages
+      # Merge all three forms of configuration into a single hash. We'll redistribute the configuration
+      # from here later on.
+      combined_storages = {}
+      Gitlab['git_data_dirs'].each do |name, details|
+        entry = {
+          'gitaly_address' => details['gitaly_address'] || gitaly_address,
+        }
+        entry['gitaly_token'] = details['gitaly_token'] if details['gitaly_token']
+        entry['path'] = File.join(details['path'] || details[:path], 'repositories') if details['path'] || details[:path]
+        combined_storages[name] = entry
       end
-      Gitlab['gitlab_rails']['repositories_storages'] =
-        Hash[Mash.new(Gitlab['git_data_dirs']).map do |name, data_directory|
-          shard_gitaly_address = data_directory['gitaly_address'] || gitaly_address
-
-          defaults = { 'path' => File.join(data_directory['path'], 'repositories'), 'gitaly_address' => shard_gitaly_address }
-          params = data_directory.merge(defaults)
-          [name, params]
-        end]
-    end
-    def parse_gitaly_storages
-      return unless Gitlab['gitaly'].dig('configuration', 'storage').nil?
-      storages = []
-      Gitlab['gitlab_rails']['repositories_storages'].each do |key, value|
-        storages << {
-          'name' => key,
-          'path' => value['path']
+      Gitlab['gitlab_rails']['repositories_storages']&.each do |name, details|
+        entry = {
+          'gitaly_address' => details['gitaly_address'] || gitaly_address,
+        }
+        entry['gitaly_token'] = details['gitaly_token'] if details['gitaly_token']
+        entry['path'] = File.join(details['path'], 'repositories') if details['path']
+        combined_storages[name] = if combined_storages[name]
+                                    combined_storages[name].merge(entry)
+                                  else
+                                    entry
+                                  end
+      end
+      if Gitlab['gitaly'].dig('configuration', 'storage')
+        Gitlab['gitaly']['configuration']['storage'].each do |storage|
+          entry = {
+            'path' => storage['path'],
+          }
+          combined_storages[storage['name']] = if combined_storages[storage['name']]
+                                                 combined_storages[storage['name']].merge(entry)
+                                               else
+                                                 entry
+                                               end
+        end
+      end
+      # If empty, we need to supply a default storage.
+      if combined_storages.empty?
+        combined_storages['default'] = {
+          'gitaly_address' => gitaly_address,
+          'path' => '/var/opt/gitlab/git-data/repositories'
         }
       end
+
+      # Redistribute the configuration amongst the various keys. When git_data_dirs is removed, we can simply
+      # remove the corresponding logic here.
+      Gitlab['git_data_dirs'] = {}
+
+      # Don't override the config if provided
+      Gitlab['gitlab_rails']['repositories_storages'] ||= {}
       Gitlab['gitaly']['configuration'] ||= {}
-      Gitlab['gitaly']['configuration']['storage'] = storages
+      Gitlab['gitaly']['configuration']['storage'] ||= []
+
+      update_rails_storage_config = Gitlab['gitlab_rails']['repositories_storages'].empty?
+      update_gitaly_storage_config = Gitlab['gitaly']['configuration']['storage'].empty?
+
+      combined_storages.each do |name, details|
+        details['gitaly_address'] = gitaly_address unless details['gitaly_address']
+
+        # The path shouldn't be set in git_data_dirs or repository_storages, since Rails shouldn't care about it.
+        without_path = details.clone.except('path')
+        Gitlab['git_data_dirs'][name] = without_path
+        Gitlab['gitlab_rails']['repositories_storages'][name] = without_path if update_rails_storage_config
+
+        # If user had specified `gitaly['configuration']['storage']`, then do
+        # not update it.
+        next unless update_gitaly_storage_config
+
+        # If the path doesn't exist, it means the current storage belongs to an external Gitaly and we don't
+        # need to generate a corresponding storage entry.
+        next unless details['path']
+
+        Gitlab['gitaly']['configuration']['storage'] << {
+          name: name.to_s,
+          path: details['path']
+        }
+      end
     end
+    # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
     # Compute the default gitconfig from the old Omnibus gitconfig setting.
     # This depends on the Gitlab cookbook having been parsed already.
@@ -173,8 +226,8 @@ module Gitaly
     # Validate that no storages are sharing the same path.
     def check_duplicate_storage_paths
-      # If Gitaly isn't running, there is no need to do this check.
-      return unless Services.enabled?('gitaly')
+      # If Gitaly isn't running or storages aren't configured, there is no need to do this check.
+      return unless Services.enabled?('gitaly') && Gitlab['gitaly'].dig('configuration', 'storage')
       # Deep copy storages to avoid mutating the original.
       storages = Marshal.load(Marshal.dump(Gitlab['gitaly']['configuration']['storage']))

files/gitlab-cookbooks/gitaly/recipes/git_data_dirs.rb

@@ -22,6 +22,8 @@ gitlab_group = account_helper.gitlab_group
 # Holds git-data, by default one shard at /var/opt/gitlab/git-data
 # Can be changed by user using git_data_dirs option
 Mash.new(Gitlab['git_data_dirs']).each do |_name, git_data_directory|
+  next unless git_data_directory['path']
+
   storage_directory git_data_directory['path'] do
     owner gitlab_user
     group gitlab_group
@@ -29,11 +31,10 @@ Mash.new(Gitlab['git_data_dirs']).each do |_name, git_data_directory|
   end
 end
-# Holds git repositories, by default at /var/opt/gitlab/git-data/repositories
-# Should not be changed by user. Different permissions to git_data_dir set.
-repositories_storages = node['gitlab']['gitlab_rails']['repositories_storages']
-repositories_storages.each do |_name, repositories_storage|
-  storage_directory repositories_storage['path'] do
+# Create the Git storage directories. There may be no directories if external Gitaly is used.
+repositories_storages = Gitlab['gitaly'].dig('configuration', 'storage') || []
+repositories_storages.each do |repositories_storage|
+  storage_directory repositories_storage[:path] do
     owner gitlab_user
     group gitlab_group
     mode "2770"

files/gitlab-cookbooks/gitlab/libraries/gitlab_rails.rb

@@ -54,7 +54,6 @@ module GitlabRails
       parse_ci_secure_files_dir
       parse_encrypted_settings_path
       parse_pages_dir
-      parse_repository_storage
     end
     def transform_secrets
@@ -361,19 +360,6 @@ module GitlabRails
       Gitlab['gitlab_rails']['pages_local_store_path'] ||= Gitlab['gitlab_rails']['pages_path']
     end
-    def parse_repository_storage
-      return if Gitlab['gitlab_rails']['repositories_storages']
-
-      gitaly_address = Gitaly.gitaly_address
-
-      Gitlab['gitlab_rails']['repositories_storages'] ||= {
-        "default" => {
-          "path" => "/var/opt/gitlab/git-data/repositories",
-          "gitaly_address" => gitaly_address
-        }
-      }
-    end
-
     def parse_gitlab_trusted_proxies
       Gitlab['nginx']['real_ip_trusted_addresses'] ||= Gitlab['node']['gitlab']['nginx']['real_ip_trusted_addresses']
       Gitlab['gitlab_rails']['trusted_proxies'] = Gitlab['nginx']['real_ip_trusted_addresses'] if Gitlab['gitlab_rails']['trusted_proxies'].nil?

files/gitlab-cookbooks/package/libraries/deprecations.rb

@@ -343,7 +343,7 @@ module Gitlab
             deprecation: '17.4',
             removal: '18.0',
             note: "`gitlab_shell['migration'] will be ignored from 17.3 and removed in 18.0. See https://gitlab.com/groups/gitlab-org/-/epics/14845."
-          }
+          },
         ]
         deprecations += praefect_legacy_configuration_deprecations
@@ -584,6 +584,8 @@ module Gitlab
         messages += deprecate_registry_notifications(incoming_version, existing_config, type, ['registry', 'notifications'], 'threshold', 17.1, 18.0)
+        messages += remove_git_data_dirs(incoming_version, existing_config, type, '17.8', '18.0')
+
         messages
       end
@@ -615,6 +617,20 @@ module Gitlab
         messages
       end
+      def remove_git_data_dirs(incoming_version, existing_config, type, deprecated_version, removed_version)
+        return [] unless existing_config[:git_data_dirs]
+
+        messages = []
+
+        if Gem::Version.new(incoming_version) >= Gem::Version.new(removed_version) && type == :removal
+          messages << "* git_data_dirs has been deprecated since #{deprecated_version} and was removed in #{removed_version}. See https://docs.gitlab.com/omnibus/settings/configuration.html#migrating-from-git_data_dirs for migration instructions."
+        elsif Gem::Version.new(incoming_version) >= Gem::Version.new(deprecated_version) && type == :deprecation
+          messages << "* git_data_dirs has been deprecated since #{deprecated_version} and will be removed in #{removed_version}. See https://docs.gitlab.com/omnibus/settings/configuration.html#migrating-from-git_data_dirs for migration instructions."
+        end
+
+        messages
+      end
+
       def deprecate_registry_notifications(incoming_version, existing_config, type, config_keys, key, deprecated_version, removed_version)
         settings = existing_config.dig(*config_keys) || []

spec/chef/cookbooks/gitlab/recipes/gitlab-rails/gitlab_yml/repositories_storages_spec.rb

@@ -10,7 +10,6 @@ RSpec.describe 'gitlab::gitlab-rails' do
         expect(gitlab_yml[:production][:repositories][:storages]).to eq(
           default: {
             gitaly_address: 'unix:/var/opt/gitlab/gitaly/gitaly.socket',
-            path: '/var/opt/gitlab/git-data/repositories'
           }
         )
       end
@@ -21,7 +20,6 @@ RSpec.describe 'gitlab::gitlab-rails' do
         stub_gitlab_rb(
           git_data_dirs: {
             second_storage: {
-              path: '/tmp/foobar',
               gitaly_address: 'unix:/var/gitaly.socket'
             }
           }
@@ -32,7 +30,6 @@ RSpec.describe 'gitlab::gitlab-rails' do
         expect(gitlab_yml[:production][:repositories][:storages]).to eq(
           second_storage: {
             gitaly_address: 'unix:/var/gitaly.socket',
-            path: '/tmp/foobar/repositories'
           }
         )
       end
@@ -52,7 +49,6 @@ RSpec.describe 'gitlab::gitlab-rails' do
           expect(gitlab_yml[:production][:repositories][:storages]).to eq(
             default: {
               gitaly_address: 'unix:/var/gitaly.socket',
-              path: '/var/opt/gitlab/git-data/repositories'
             }
           )
         end

spec/chef/cookbooks/gitlab/recipes/gitlab-rails_spec.rb

@@ -125,9 +125,14 @@ RSpec.describe 'gitlab::gitlab-rails' do
                                        uploads_directory: '/tmp/uploads',
                                        uploads_storage_path: '/tmp/uploads_storage' },
                        gitlab_ci: { builds_directory: '/tmp/builds' },
-                       git_data_dirs: {
-                         "some_storage" => {
-                           "path" => "/tmp/git-data"
+                       gitaly: {
+                         configuration: {
+                           storage: [
+                             {
+                               'name' => 'some_storage',
+                               'path' => '/tmp/git-data/repositories'
+                             }
+                           ]
                          }
                        })
       end
@@ -135,7 +140,7 @@ RSpec.describe 'gitlab::gitlab-rails' do
       ChefSpec::SoloRunner.converge('gitlab::default')
     end
-    include_examples "git data directory", "/tmp/git-data"
+    include_examples "git data directory", "/tmp/git-data/repositories"
     it 'creates the shared directory' do
       expect(chef_run).to create_storage_directory('/tmp/shared').with(owner: 'git', group: 'gitlab-www', mode: '0751')

spec/chef/cookbooks/package/libraries/deprecations_spec.rb

-require 'spec_helper'
+require 'chef_helper'
 RSpec.describe Gitlab::Deprecations do
   let(:valid_config) { { gitlab: { nginx: { listen_addresses: "SomeRandomString" } } } }
@@ -125,6 +125,30 @@ RSpec.describe Gitlab::Deprecations do
     end
   end
+  describe '.remove_git_data_dirs' do
+    context 'when git_data_dirs is specified' do
+      it 'raises warning' do
+        config = {
+          git_data_dirs: {
+            default: {
+              path: 'abc',
+              gitaly_address: 'unix:/var/gitaly.socket'
+            },
+            valid: {
+              gitaly_address: 'unix:/var/gitaly.socket'
+            }
+          }
+        }
+
+        deprecated_message = "* git_data_dirs has been deprecated since 17.8 and will be removed in 18.0. See https://docs.gitlab.com/omnibus/settings/configuration.html#migrating-from-git_data_dirs for migration instructions."
+        expect(described_class.remove_git_data_dirs('17.8', config, :deprecation, '17.8', '18.0')).to eq([deprecated_message])
+
+        removed_message = "* git_data_dirs has been deprecated since 17.8 and was removed in 18.0. See https://docs.gitlab.com/omnibus/settings/configuration.html#migrating-from-git_data_dirs for migration instructions."
+        expect(described_class.remove_git_data_dirs('18.0', config, :removal, '17.8', '18.0')).to eq([removed_message])
+      end
+    end
+  end
+
   describe '.deprecate_only_if_value' do
     let(:chef_run) { ChefSpec::SoloRunner.converge('gitlab::default') }

spec/chef/support/shared_examples/git_data_dirs.rb

 RSpec.shared_examples 'git data directory' do |git_data_path|
-  it 'creates the git-data directory' do
-    expect(chef_run).to create_storage_directory(git_data_path).with(owner: 'git', group: 'git', mode: '2770')
-  end
-
   it 'creates the repositories directory' do
-    expect(chef_run).to create_storage_directory("#{git_data_path}/repositories").with(owner: 'git', group: 'git', mode: '2770')
+    expect(chef_run).to create_storage_directory(git_data_path).with(owner: 'git', group: 'git', mode: '2770')
   end
 end