Automatic merge of gitlab-org/gitlab-ce master

app/models/project.rb

@@ -1789,6 +1789,24 @@ class Project < ActiveRecord::Base
     handle_update_attribute_error(e, value)
   end
 
+  # Tries to set repository as read_only, checking for existing Git transfers in progress beforehand
+  #
+  # @return [Boolean] true when set to read_only or false when an existing git transfer is in progress
+  def set_repository_read_only!
+    with_lock do
+      break false if git_transfer_in_progress?
+
+      update_column(:repository_read_only, true)
+    end
+  end
+
+  # Set repository as writable again
+  def set_repository_writable!
+    with_lock do
+      update_column(repository_read_only, false)
+    end
+  end
+
   def pushes_since_gc
     Gitlab::Redis::SharedState.with { |redis| redis.get(pushes_since_gc_redis_shared_state_key).to_i }
   end
@@ -1903,15 +1921,17 @@ class Project < ActiveRecord::Base
   def migrate_to_hashed_storage!
     return unless storage_upgradable?
 
-    update!(repository_read_only: true)
-
-    if repo_reference_count > 0 || wiki_reference_count > 0
+    if git_transfer_in_progress?
       ProjectMigrateHashedStorageWorker.perform_in(Gitlab::ReferenceCounter::REFERENCE_EXPIRE_TIME, id)
     else
       ProjectMigrateHashedStorageWorker.perform_async(id)
     end
   end
 
+  def git_transfer_in_progress?
+    repo_reference_count > 0 || wiki_reference_count > 0
+  end
+
   def storage_version=(value)
     super
 

app/services/projects/hashed_storage/migrate_repository_service.rb

@@ -2,6 +2,8 @@
 
 module Projects
   module HashedStorage
+    RepositoryMigrationError = Class.new(StandardError)
+
     class MigrateRepositoryService < BaseService
       include Gitlab::ShellAdapter
 
@@ -18,6 +20,8 @@ module Projects
       end
 
       def execute
+        try_to_set_repository_read_only!
+
         @old_storage_version = project.storage_version
         project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository]
         project.ensure_storage_path_exists
@@ -50,6 +54,16 @@ module Projects
 
       private
 
+      def try_to_set_repository_read_only!
+        # Mitigate any push operation to start during migration
+        unless project.set_repository_read_only!
+          migration_error = "Target repository '#{old_disk_path}' cannot be made read-only as there is a git transfer in progress"
+          logger.error migration_error
+
+          raise RepositoryMigrationError, migration_error
+        end
+      end
+
       # rubocop: disable CodeReuse/ActiveRecord
       def has_wiki?
         gitlab_shell.exists?(project.repository_storage, "#{old_wiki_disk_path}.git")

changelogs/unreleased/53966-hashed-storage-read-only.yml

+---
+title: 'Hashed Storage: Only set as `read_only` when starting the per-project migration'
+merge_request: 24128
+author:
+type: changed

doc/administration/auth/how_to_configure_ldap_gitlab_ce/index.md

@@ -255,7 +255,7 @@ After configuring LDAP, basic authentication will be available. Users can then l
 
 Users that are removed from the LDAP base group (e.g `OU=GitLab INT,DC=GitLab,DC=org`) will be **blocked** in GitLab. [More information](../ldap.md#security) on LDAP security.
 
-If `allow_username_or_email_login` is enabled in the LDAP configuration, GitLab will ignore everything after the first '@' in the LDAP username used on login. Example: The username `jon.doe@example.com` is converted to `jon.doe` when authenticating with the LDAP server. Disable this setting if you use `userPrincipalName` as the `uid`.
+If `allow_username_or_email_login` is enabled in the LDAP configuration, GitLab will ignore everything after the first '@' in the LDAP username used on login. Example: The username `` jon.doe@example.com `` is converted to `jon.doe` when authenticating with the LDAP server. Disable this setting if you use `userPrincipalName` as the `uid`.
 
 ## LDAP extended features on GitLab EE
 

doc/development/emails.md

@@ -76,14 +76,32 @@ See the [Rails guides] for more info.
 
 ## Email namespace
 
-If you need to implement a new feature which requires a new email handler, follow these rules:
+As of GitLab 11.7, we support a new format for email handler addresses.  This was done to 
+support catch-all mailboxes.
 
- - You must choose a namespace. The namespace cannot contain `/` or `+`, and cannot match `\h{16}`.
- - If your feature is related to a project, you will append the namespace **after** the project path, separated by a `+`
- - If you have different actions in the namespace, you add the actions **after** the namespace separated by a `+`. The action name cannot contain `/` or `+`, , and cannot match `\h{16}`.
- - You will register your handlers in `lib/gitlab/email/handler.rb`
+If you need to implement a feature which requires a new email handler, follow these rules
+for the format of the email key:
 
-Therefore, these are the only valid formats for an email handler:
+- Actions are always at the end, separated by `-`.  For example `-issue` or `-merge-request`
+- If your feature is related to a project, the key begins with the project identifiers (project path slug
+  and project id), separated by `-`.  For example, `gitlab-org-gitlab-ce-20`
+- Additional information, such as an author's token, can be added between the project identifiers and
+  the action, separated by `-`.  For example, `gitlab-org-gitlab-ce-20-Author_Token12345678-issue`
+- You register your handlers in `lib/gitlab/email/handler.rb`
+
+Examples of valid email keys:
+
+ - `gitlab-org-gitlab-ce-20-Author_Token12345678-issue` (create a new issue)
+ - `gitlab-org-gitlab-ce-20-Author_Token12345678-merge-request` (create a new merge request)
+ - `1234567890abcdef1234567890abcdef-unsubscribe` (unsubscribe from a conversation)
+ - `1234567890abcdef1234567890abcdef` (reply to a conversation)
+
+Please note that the action `-issue-` is used in GitLab Premium as the handler for the Service Desk feature.
+
+### Legacy format
+
+Although we continue to support the older legacy format, no new features should use a legacy format.
+These are the only valid legacy formats for an email handler:
 
  - `path/to/project+namespace`
  - `path/to/project+namespace+action`

doc/user/project/merge_requests/index.md

@@ -193,9 +193,9 @@ those conflicts in the GitLab UI.
 
 ## Create new merge requests by email
 
-*This feature needs [incoming email](../../../administration/incoming_email.md)
+_This feature needs [incoming email](../../../administration/incoming_email.md)
 to be configured by a GitLab administrator to be available for CE/EE users, and
-it's available on GitLab.com.*
+it's available on GitLab.com._
 
 You can create a new merge request by sending an email to a user-specific email
 address. The address can be obtained on the merge requests page by clicking on
@@ -207,8 +207,16 @@ will be used as the merge request description. You need
 this feature. If it's not enabled to your instance, you may ask your GitLab
 administrator to do so.
 
+This is a private email address, generated just for you. **Keep it to yourself**
+as anyone who gets ahold of it can create issues or merge requests as if they were you.
+You can add this address to your contact list for easy access.
+
 ![Create new merge requests by email](img/create_from_email.png)
 
+_In GitLab 11.7, we updated the format of the generated email address.
+However the older format is still supported, allowing existing aliases
+or contacts to continue working._
+
 ### Adding patches when creating a merge request via e-mail
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/22723) in GitLab 11.5.

spec/lib/gitlab/hashed_storage/migrator_spec.rb

@@ -19,15 +19,6 @@ describe Gitlab::HashedStorage::Migrator do
       end
     end
 
-    it 'sets projects as read only' do
-      allow(ProjectMigrateHashedStorageWorker).to receive(:perform_async).twice
-      subject.bulk_migrate(ids.min, ids.max)
-
-      projects.each do |project|
-        expect(project.reload.repository_read_only?).to be_truthy
-      end
-    end
-
     it 'rescues and log exceptions' do
       allow_any_instance_of(Project).to receive(:migrate_to_hashed_storage!).and_raise(StandardError)
       expect { subject.bulk_migrate(ids.min, ids.max) }.not_to raise_error
@@ -40,6 +31,16 @@ describe Gitlab::HashedStorage::Migrator do
 
       subject.bulk_migrate(ids.min, ids.max)
     end
+
+    it 'has migrated projects set as writable' do
+      perform_enqueued_jobs do
+        subject.bulk_migrate(ids.min, ids.max)
+      end
+
+      projects.each do |project|
+        expect(project.reload.repository_read_only?).to be_falsey
+      end
+    end
   end
 
   describe '#migrate' do
@@ -57,19 +58,20 @@ describe Gitlab::HashedStorage::Migrator do
       expect { subject.migrate(project) }.not_to raise_error
     end
 
-    it 'sets project as read only' do
-      allow(ProjectMigrateHashedStorageWorker).to receive(:perform_async)
+    it 'migrate project' do
+      perform_enqueued_jobs do
         subject.migrate(project)
+      end
 
-      expect(project.reload.repository_read_only?).to be_truthy
+      expect(project.reload.hashed_storage?(:attachments)).to be_truthy
     end
 
-    it 'migrate project' do
+    it 'has migrated project set as writable' do
       perform_enqueued_jobs do
         subject.migrate(project)
       end
 
-      expect(project.reload.hashed_storage?(:attachments)).to be_truthy
+      expect(project.reload.repository_read_only?).to be_falsey
     end
   end
 end

spec/models/project_spec.rb

@@ -2653,6 +2653,20 @@ describe Project do
     end
   end
 
+  describe '#set_repository_read_only!' do
+    let(:project) { create(:project) }
+
+    it 'returns true when there is no existing git transfer in progress' do
+      expect(project.set_repository_read_only!).to be_truthy
+    end
+
+    it 'returns false when there is an existing git transfer in progress' do
+      allow(project).to receive(:git_transfer_in_progress?) { true }
+
+      expect(project.set_repository_read_only!).to be_falsey
+    end
+  end
+
   describe '#pushes_since_gc' do
     let(:project) { create(:project) }
 
@@ -3425,6 +3439,33 @@ describe Project do
     end
   end
 
+  describe '#git_transfer_in_progress?' do
+    let(:project) { build(:project) }
+
+    subject { project.git_transfer_in_progress? }
+
+    it 'returns false when repo_reference_count and wiki_reference_count are 0' do
+      allow(project).to receive(:repo_reference_count) { 0 }
+      allow(project).to receive(:wiki_reference_count) { 0 }
+
+      expect(subject).to be_falsey
+    end
+
+    it 'returns true when repo_reference_count is > 0' do
+      allow(project).to receive(:repo_reference_count) { 2 }
+      allow(project).to receive(:wiki_reference_count) { 0 }
+
+      expect(subject).to be_truthy
+    end
+
+    it 'returns true when wiki_reference_count is > 0' do
+      allow(project).to receive(:repo_reference_count) { 0 }
+      allow(project).to receive(:wiki_reference_count) { 2 }
+
+      expect(subject).to be_truthy
+    end
+  end
+
   context 'legacy storage' do
     let(:project) { create(:project, :repository, :legacy_storage) }
     let(:gitlab_shell) { Gitlab::Shell.new }
@@ -3485,10 +3526,6 @@ describe Project do
         expect(project.migrate_to_hashed_storage!).to be_truthy
       end
 
-      it 'flags as read-only' do
-        expect { project.migrate_to_hashed_storage! }.to change { project.repository_read_only }.to(true)
-      end
-
       it 'does not validate project visibility' do
         expect(project).not_to receive(:visibility_level_allowed_as_fork)
         expect(project).not_to receive(:visibility_level_allowed_by_group)

spec/services/projects/hashed_storage/migrate_repository_service_spec.rb

@@ -15,6 +15,20 @@ describe Projects::HashedStorage::MigrateRepositoryService do
       allow(service).to receive(:gitlab_shell) { gitlab_shell }
     end
 
+    context 'repository lock' do
+      it 'tries to lock the repository' do
+        expect(service).to receive(:try_to_set_repository_read_only!)
+
+        service.execute
+      end
+
+      it 'fails when a git operation is in progress' do
+        allow(project).to receive(:repo_reference_count) { 1 }
+
+        expect { service.execute }.to raise_error(Projects::HashedStorage::RepositoryMigrationError)
+      end
+    end
+
     context 'when succeeds' do
       it 'renames project and wiki repositories' do
         service.execute