Merge branch '24059-round-robin-repository-storage' into 'master'

Resolve "Introduce round-robin project creation to spread load over multiple shards"

## What does this MR do?

Allow multiple shards to be enabled in the admin settings page, balancing project creation across all enabled shards.

## Are there points in the code the reviewer needs to double check?

* `f.select ..., multiple: true` isn't the most beautiful UI in the world, but switching to `collection_check_boxes` (or a facsimile thereof) isn't trivial
* Should `pick_repository_storage` be a method of `ApplicationSetting`, or `Project`? It's going to accrete logic over time so perhaps it should be its own class already?
* This is written to avoid the need for a database migration, so it is`serialize :repository_storage` without `, Array`. This is tested, but alternatives include:
  * Add a database migration
  * Write a custom Coder that will accept a String or Array in `load` and always `dump an Array.

## Why was this MR needed?

## Screenshots (if relevant)

![Screen_Shot_2016-11-03_at_14.42.41](/uploads/7de15d6c1b3fa60bb7a34d6a7d9f00ce/Screen_Shot_2016-11-03_at_14.42.41.png)

## Does this MR meet the acceptance criteria?

- [X] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG.md) entry added
- [X] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [X] Added for this feature/bug
  - [ ] All builds are passing
- [X] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [X] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [X] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [X] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Closes #24059

See merge request !7273

app/controllers/admin/application_settings_controller.rb

@@ -116,8 +116,8 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       :metrics_packet_size,
       :send_user_confirmation_email,
       :container_registry_token_expire_delay,
-      :repository_storage,
       :enabled_git_access_protocol,
+      repository_storages: [],
       restricted_visibility_levels: [],
       import_sources: [],
       disabled_oauth_sign_in_sources: []

app/helpers/application_settings_helper.rb

@@ -93,11 +93,11 @@ module ApplicationSettingsHelper
     end
   end
 
-  def repository_storage_options_for_select
+  def repository_storages_options_for_select
     options = Gitlab.config.repositories.storages.map do |name, path|
       ["#{name} - #{path}", name]
     end
 
-    options_for_select(options, @application_setting.repository_storage)
+    options_for_select(options, @application_setting.repository_storages)
   end
 end

app/models/application_setting.rb

@@ -18,6 +18,7 @@ class ApplicationSetting < ActiveRecord::Base
   serialize :disabled_oauth_sign_in_sources, Array
   serialize :domain_whitelist, Array
   serialize :domain_blacklist, Array
+  serialize :repository_storages
 
   cache_markdown_field :sign_in_text
   cache_markdown_field :help_page_text
@@ -74,9 +75,8 @@ class ApplicationSetting < ActiveRecord::Base
             presence: true,
             numericality: { only_integer: true, greater_than: 0 }
 
-  validates :repository_storage,
-    presence: true,
-    inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
+  validates :repository_storages, presence: true
+  validate :check_repository_storages
 
   validates :enabled_git_access_protocol,
             inclusion: { in: %w(ssh http), allow_blank: true, allow_nil: true }
@@ -166,7 +166,7 @@ class ApplicationSetting < ActiveRecord::Base
       disabled_oauth_sign_in_sources: [],
       send_user_confirmation_email: false,
       container_registry_token_expire_delay: 5,
-      repository_storage: 'default',
+      repository_storages: ['default'],
       user_default_external: false,
     )
   end
@@ -201,6 +201,29 @@ class ApplicationSetting < ActiveRecord::Base
     self.domain_blacklist_raw = file.read
   end
 
+  def repository_storages
+    value = read_attribute(:repository_storages)
+    value = [value] if value.is_a?(String)
+    value = [] if value.nil?
+
+    value
+  end
+
+  # repository_storage is still required in the API. Remove in 9.0
+  def repository_storage
+    repository_storages.first
+  end
+
+  def repository_storage=(value)
+    self.repository_storages = [value]
+  end
+
+  # Choose one of the available repository storage options. Currently all have
+  # equal weighting.
+  def pick_repository_storage
+    repository_storages.sample
+  end
+
   def runners_registration_token
     ensure_runners_registration_token!
   end
@@ -208,4 +231,12 @@ class ApplicationSetting < ActiveRecord::Base
   def health_check_access_token
     ensure_health_check_access_token!
   end
+
+  private
+
+  def check_repository_storages
+    invalid = repository_storages - Gitlab.config.repositories.storages.keys
+    errors.add(:repository_storages, "can't include: #{invalid.join(", ")}") unless
+      invalid.empty?
+  end
 end

app/models/project.rb

@@ -28,7 +28,7 @@ class Project < ActiveRecord::Base
   default_value_for :archived, false
   default_value_for :visibility_level, gitlab_config_features.visibility_level
   default_value_for :container_registry_enabled, gitlab_config_features.container_registry
-  default_value_for(:repository_storage) { current_application_settings.repository_storage }
+  default_value_for(:repository_storage) { current_application_settings.pick_repository_storage }
   default_value_for(:shared_runners_enabled) { current_application_settings.shared_runners_enabled }
   default_value_for :issues_enabled, gitlab_config_features.issues
   default_value_for :merge_requests_enabled, gitlab_config_features.merge_requests

app/views/admin/application_settings/_form.html.haml

@@ -353,9 +353,9 @@
   %fieldset
     %legend Repository Storage
     .form-group
-      = f.label :repository_storage, 'Storage path for new projects', class: 'control-label col-sm-2'
+      = f.label :repository_storages, 'Storage paths for new projects', class: 'control-label col-sm-2'
       .col-sm-10
-        = f.select :repository_storage, repository_storage_options_for_select, {}, class: 'form-control'
+        = f.select :repository_storages, repository_storages_options_for_select, {include_hidden: false}, multiple: true, class: 'form-control'
         .help-block
           Manage repository storage paths. Learn more in the
           = succeed "." do

changelogs/unreleased/24059-round-robin-repository-storage.yml

+---
+title: Introduce round-robin project creation to spread load over multiple shards
+merge_request: 7266
+author: 

db/migrate/20161103171205_rename_repository_storage_column.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RenameRepositoryStorageColumn < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    rename_column :application_settings, :repository_storage, :repository_storages
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20161025231710) do
+ActiveRecord::Schema.define(version: 20161103171205) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -88,7 +88,7 @@ ActiveRecord::Schema.define(version: 20161025231710) do
     t.integer "container_registry_token_expire_delay", default: 5
     t.text "after_sign_up_text"
     t.boolean "user_default_external", default: false, null: false
-    t.string "repository_storage", default: "default"
+    t.string "repository_storages", default: "default"
     t.string "enabled_git_access_protocol"
     t.boolean "domain_blacklist_enabled", default: false
     t.text "domain_blacklist"

doc/administration/repository_storages.md

@@ -91,6 +91,9 @@ be stored via the **Application Settings** in the Admin area.
 
 ![Choose repository storage path in Admin area](img/repository_storages_admin_ui.png)
 
+Beginning with GitLab 8.13.4, multiple paths can be chosen. New projects will be
+randomly placed on one of the selected paths.
+
 [ce-4578]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4578
 [restart gitlab]: restart_gitlab.md#installations-from-source
 [reconfigure gitlab]: restart_gitlab.md#omnibus-gitlab-reconfigure

doc/api/settings.md

@@ -42,6 +42,7 @@ Example response:
    "sign_in_text" : null,
    "container_registry_token_expire_delay": 5,
    "repository_storage": "default",
+   "repository_storages": ["default"],
    "koding_enabled": false,
    "koding_url": null
 }
@@ -73,7 +74,8 @@ PUT /application/settings
 | `user_oauth_applications` | boolean | no | Allow users to register any application to use GitLab as an OAuth provider |
 | `after_sign_out_path` | string | no | Where to redirect users after logout |
 | `container_registry_token_expire_delay` | integer | no | Container Registry token duration in minutes |
-| `repository_storage` | string | no | Storage path for new projects. The value should be the name of one of the repository storage paths defined in your gitlab.yml |
+| `repository_storages` | array of strings | no | A list of names of enabled storage paths, taken from `gitlab.yml`. New projects will be created in one of these stores, chosen at random. |
+| `repository_storage` | string | no | The first entry in `repository_storages`. Deprecated, but retained for compatibility reasons |
 | `enabled_git_access_protocol` | string | no | Enabled protocols for Git access. Allowed values are: `ssh`, `http`, and `nil` to allow both protocols. |
 | `koding_enabled` | boolean | no | Enable Koding integration. Default is `false`. |
 | `koding_url` | string | yes (if `koding_enabled` is `true`) |  The Koding instance URL for integration. |

lib/api/entities.rb

@@ -509,6 +509,7 @@ module API
       expose :after_sign_out_path
       expose :container_registry_token_expire_delay
       expose :repository_storage
+      expose :repository_storages
       expose :koding_enabled
       expose :koding_url
     end

lib/api/settings.rb

@@ -17,12 +17,12 @@ module API
       present current_settings, with: Entities::ApplicationSetting
     end
 
-    # Modify applicaiton settings
+    # Modify application settings
     #
     # Example Request:
     #   PUT /application/settings
     put "application/settings" do
-      attributes = current_settings.attributes.keys - ["id"]
+      attributes = ["repository_storage"] + current_settings.attributes.keys - ["id"]
       attrs = attributes_for_keys(attributes)
 
       if current_settings.update_attributes(attrs)

spec/models/application_setting_spec.rb

@@ -41,14 +41,62 @@ describe ApplicationSetting, models: true do
       subject { setting }
     end
 
-    context 'repository storages inclussion' do
+    # Upgraded databases will have this sort of content
+    context 'repository_storages is a String, not an Array' do
+      before { setting.__send__(:raw_write_attribute, :repository_storages, 'default') }
+
+      it { expect(setting.repository_storages_before_type_cast).to eq('default') }
+      it { expect(setting.repository_storages).to eq(['default']) }
+    end
+
+    context 'repository storages' do
       before do
-        storages = { 'custom' => 'tmp/tests/custom_repositories' }
+        storages = {
+          'custom1' => 'tmp/tests/custom_repositories_1',
+          'custom2' => 'tmp/tests/custom_repositories_2',
+          'custom3' => 'tmp/tests/custom_repositories_3',
+
+        }
         allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
       end
 
-      it { is_expected.to allow_value('custom').for(:repository_storage) }
-      it { is_expected.not_to allow_value('alternative').for(:repository_storage) }
+      describe 'inclusion' do
+        it { is_expected.to allow_value('custom1').for(:repository_storages) }
+        it { is_expected.to allow_value(['custom2', 'custom3']).for(:repository_storages) }
+        it { is_expected.not_to allow_value('alternative').for(:repository_storages) }
+        it { is_expected.not_to allow_value(['alternative', 'custom1']).for(:repository_storages) }
+      end
+
+      describe 'presence' do
+        it { is_expected.not_to allow_value([]).for(:repository_storages) }
+        it { is_expected.not_to allow_value("").for(:repository_storages) }
+        it { is_expected.not_to allow_value(nil).for(:repository_storages) }
+      end
+
+      describe '.pick_repository_storage' do
+        it 'uses Array#sample to pick a random storage' do
+          array = double('array', sample: 'random')
+          expect(setting).to receive(:repository_storages).and_return(array)
+
+          expect(setting.pick_repository_storage).to eq('random')
+        end
+
+        describe '#repository_storage' do
+          it 'returns the first storage' do
+            setting.repository_storages = ['good', 'bad']
+
+            expect(setting.repository_storage).to eq('good')
+          end
+        end
+
+        describe '#repository_storage=' do
+          it 'overwrites repository_storages' do
+            setting.repository_storage = 'overwritten'
+
+            expect(setting.repository_storages).to eq(['overwritten'])
+          end
+        end
+      end
     end
   end
 

spec/models/project_spec.rb

@@ -837,16 +837,19 @@ describe Project, models: true do
   context 'repository storage by default' do
     let(:project) { create(:empty_project) }
 
-    subject { project.repository_storage }
-
     before do
-      storages = { 'alternative_storage' => '/some/path' }
+      storages = {
+        'default' => 'tmp/tests/repositories',
+        'picked'  => 'tmp/tests/repositories',
+      }
       allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
-      stub_application_setting(repository_storage: 'alternative_storage')
-      allow_any_instance_of(Project).to receive(:ensure_dir_exist).and_return(true)
     end
 
-    it { is_expected.to eq('alternative_storage') }
+    it 'picks storage from ApplicationSetting' do
+      expect_any_instance_of(ApplicationSetting).to receive(:pick_repository_storage).and_return('picked')
+
+      expect(project.repository_storage).to eq('picked')
+    end
   end
 
   context 'shared runners by default' do

spec/requests/api/settings_spec.rb

@@ -33,6 +33,7 @@ describe API::API, 'Settings', api: true  do
         expect(json_response['default_projects_limit']).to eq(3)
         expect(json_response['signin_enabled']).to be_falsey
         expect(json_response['repository_storage']).to eq('custom')
+        expect(json_response['repository_storages']).to eq(['custom'])
         expect(json_response['koding_enabled']).to be_truthy
         expect(json_response['koding_url']).to eq('http://koding.example.com')
       end