Merge branch 'revert-382e882e' into 'master'

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

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



Merged-by: Balasankar 'Balu' C <balasankar@gitlab.com>
Approved-by: Andrew Patterson <apatterson@gitlab.com>
Approved-by: Balasankar 'Balu' C <balasankar@gitlab.com>
Co-authored-by: John Skarbek <jskarbek@gitlab.com>

doc/settings/configuration.md

@@ -154,73 +154,27 @@ 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',
-    },
-  ],
-}
-```
-
 ## Store Git data in an alternative directory
 By default, Linux package installations store the Git repository data under
-`/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,
+`/var/opt/gitlab/git-data`. The repositories are stored in a subfolder called
+`repositories`.
+To change the location of the `git-data` parent directory:
 1. Edit `/etc/gitlab/gitlab.rb`:
    ```ruby
-   gitaly['configuration'] = {
-     storage: [
-       {
-         name: 'default',
-         path: '/mnt/nas/git-data/repositories',
-       },
-     ],
-   }
+   git_data_dirs({ "default" => { "path" => "/mnt/nas/git-data" } })
    ```
    You can also add more than one Git data directory:
    ```ruby
-   gitaly['configuration'] = {
-     storage: [
-       {
-         name: 'default',
-         path: '/var/opt/gitlab/git-data/repositories',
-       },
-       {
-         name: 'alternative',
-         path: '/mnt/nas/git-data/repositories',
-       },
-     ],
-   }
+   git_data_dirs({
+     "default" => { "path" => "/var/opt/gitlab/git-data" },
+     "alternative" => { "path" => "/mnt/nas/git-data" }
+   })
    ```
    The target directories and any of its subpaths must not be a symlink.
@@ -266,7 +220,8 @@ To change the location of the directory,
       sudo gitlab-ctl start
       ```
-If you're running Gitaly on a separate server, see
+If you're running Gitaly on a separate server, remember to also include the
+`gitaly_address` for each Git data directory. 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,21 +149,8 @@ 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) 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'
-     }
-   }
+   # We assume your repositories are in /home/git/repositories (default for source installs)
+   git_data_dirs({ 'default' => { 'path' => '/home/git' } })
    # 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 `gitaly['configuration'].storage` paths.
+###! Should not be the same path, or a sub directory of any of the `git_data_dirs`
 # gitlab_rails['gitlab_repository_downloads_path'] = 'tmp/repositories'
 ### Gravatar Settings
@@ -686,6 +686,16 @@ 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,6 +24,7 @@ module Gitaly
     include OutputHelper
     def parse_variables
+      parse_git_data_dirs
       parse_gitaly_storages
       parse_gitconfig
       check_duplicate_storage_paths
@@ -54,82 +55,37 @@ module Gitaly
       end
     end
-    # 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,
-          'gitaly_token' => 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']&.each do |name, details|
-        entry = {
-          'gitaly_address' => details['gitaly_address'] || gitaly_address,
-          'gitaly_token' => 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
+    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'
       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'] = {}
-      Gitlab['gitlab_rails']['repositories_storages'] = {}
-      Gitlab['gitaly']['configuration'] ||= {} # don't override the config if provided
-      Gitlab['gitaly']['configuration']['storage'] = []
-
-      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 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']
+      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']
         }
       end
+      Gitlab['gitaly']['configuration'] ||= {}
+      Gitlab['gitaly']['configuration']['storage'] = storages
     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.
@@ -217,8 +173,8 @@ module Gitaly
     # Validate that no storages are sharing the same path.
     def check_duplicate_storage_paths
-      # 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')
+      # If Gitaly isn't running, there is no need to do this check.
+      return unless Services.enabled?('gitaly')
       # 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,8 +22,6 @@ 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
@@ -31,10 +29,11 @@ Mash.new(Gitlab['git_data_dirs']).each do |_name, git_data_directory|
   end
 end
-# 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
+# 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
     owner gitlab_user
     group gitlab_group
     mode "2770"

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

@@ -54,6 +54,7 @@ module GitlabRails
       parse_ci_secure_files_dir
       parse_encrypted_settings_path
       parse_pages_dir
+      parse_repository_storage
     end
     def transform_secrets
@@ -360,6 +361,19 @@ 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,8 +584,6 @@ 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
@@ -617,20 +615,6 @@ 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,6 +10,7 @@ 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
@@ -20,6 +21,7 @@ RSpec.describe 'gitlab::gitlab-rails' do
         stub_gitlab_rb(
           git_data_dirs: {
             second_storage: {
+              path: '/tmp/foobar',
               gitaly_address: 'unix:/var/gitaly.socket'
             }
           }
@@ -30,6 +32,7 @@ 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
@@ -49,6 +52,7 @@ 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,14 +125,9 @@ RSpec.describe 'gitlab::gitlab-rails' do
                                        uploads_directory: '/tmp/uploads',
                                        uploads_storage_path: '/tmp/uploads_storage' },
                        gitlab_ci: { builds_directory: '/tmp/builds' },
-                       gitaly: {
-                         configuration: {
-                           storage: [
-                             {
-                               'name' => 'some_storage',
-                               'path' => '/tmp/git-data/repositories'
-                             }
-                           ]
+                       git_data_dirs: {
+                         "some_storage" => {
+                           "path" => "/tmp/git-data"
                          }
                        })
       end
@@ -140,7 +135,7 @@ RSpec.describe 'gitlab::gitlab-rails' do
       ChefSpec::SoloRunner.converge('gitlab::default')
     end
-    include_examples "git data directory", "/tmp/git-data/repositories"
+    include_examples "git data directory", "/tmp/git-data"
     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 'chef_helper'
+require 'spec_helper'
 RSpec.describe Gitlab::Deprecations do
   let(:valid_config) { { gitlab: { nginx: { listen_addresses: "SomeRandomString" } } } }
@@ -125,30 +125,6 @@ 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 repositories directory' do
+  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')
+  end
 end