Merge branch 'fix/stuck-remote_mirror' into 'master'

Fix remote mirror stuck on started issue

Fixes https://gitlab.com/gitlab-org/gitlab-ee/issues/590

I think the issue was that after a mirror update while the remote mirror is in `started`, this is never picked up again by the worker. There has been a few problems with the remote mirror workers being killed manually due to some performance issues - that will increase the chances that the mirror remained in the `started` status.

See merge request !491

app/models/remote_mirror.rb

@@ -38,7 +38,7 @@ class RemoteMirror < ActiveRecord::Base
 
   scope :enabled, -> { where(enabled: true) }
   scope :started, -> { with_update_status(:started) }
-  scope :stuck,   -> { started.where('last_update_at < ?', 1.day.ago) }
+  scope :stuck,   -> { started.where('last_update_at < ? OR (last_update_at IS NULL AND updated_at < ?)', 1.day.ago, 1.day.ago) }
 
   state_machine :update_status, initial: :none do
     event :update_start do

app/services/projects/update_remote_mirror_service.rb

@@ -18,7 +18,7 @@ module Projects
 
         push_tags if changed_tags.present?
         delete_tags if deleted_tags.present?
-      rescue Gitlab::Shell::Error => e
+      rescue => e
         errors << e.message.strip
       end
 

spec/models/remote_mirror_spec.rb

@@ -23,19 +23,19 @@ describe RemoteMirror do
   describe 'encrypting credentials' do
     context 'when setting URL for a first time' do
       it 'should store the URL without credentials' do
-        mirror = create_mirror_with_url('http://foo:bar@test.com')
+        mirror = create_mirror(url: 'http://foo:bar@test.com')
 
         expect(mirror.read_attribute(:url)).to eq('http://test.com')
       end
 
       it 'should store the credentials on a separate field' do
-        mirror = create_mirror_with_url('http://foo:bar@test.com')
+        mirror = create_mirror(url: 'http://foo:bar@test.com')
 
         expect(mirror.credentials).to eq({ user: 'foo', password: 'bar' })
       end
 
       it 'should handle credentials with large content' do
-        mirror = create_mirror_with_url('http://bxnhm8dote33ct932r3xavslj81wxmr7o8yux8do10oozckkif:9ne7fuvjn40qjt35dgt8v86q9m9g9essryxj76sumg2ccl2fg26c0krtz2gzfpyq4hf22h328uhq6npuiq6h53tpagtsj7vsrz75@test.com')
+        mirror = create_mirror(url: 'http://bxnhm8dote33ct932r3xavslj81wxmr7o8yux8do10oozckkif:9ne7fuvjn40qjt35dgt8v86q9m9g9essryxj76sumg2ccl2fg26c0krtz2gzfpyq4hf22h328uhq6npuiq6h53tpagtsj7vsrz75@test.com')
 
         expect(mirror.credentials).to eq({
           user: 'bxnhm8dote33ct932r3xavslj81wxmr7o8yux8do10oozckkif',
@@ -46,7 +46,7 @@ describe RemoteMirror do
 
     context 'when updating the URL' do
       it 'should allow a new URL without credentials' do
-        mirror = create_mirror_with_url('http://foo:bar@test.com')
+        mirror = create_mirror(url: 'http://foo:bar@test.com')
 
         mirror.update_attribute(:url, 'http://test.com')
 
@@ -55,7 +55,7 @@ describe RemoteMirror do
       end
 
       it 'should allow a new URL with credentials' do
-        mirror = create_mirror_with_url('http://test.com')
+        mirror = create_mirror(url: 'http://test.com')
 
         mirror.update_attribute(:url, 'http://foo:bar@test.com')
 
@@ -64,7 +64,7 @@ describe RemoteMirror do
       end
 
       it 'should update the remote config if credentials changed' do
-        mirror = create_mirror_with_url('http://foo:bar@test.com')
+        mirror = create_mirror(url: 'http://foo:bar@test.com')
         repo = mirror.project.repository
 
         mirror.update_attribute(:url, 'http://foo:baz@test.com')
@@ -77,7 +77,7 @@ describe RemoteMirror do
   describe '#safe_url' do
     context 'when URL contains credentials' do
       it 'should mask the credentials' do
-        mirror = create_mirror_with_url('http://foo:bar@test.com')
+        mirror = create_mirror(url: 'http://foo:bar@test.com')
 
         expect(mirror.safe_url).to eq('http://*****:*****@test.com')
       end
@@ -85,15 +85,26 @@ describe RemoteMirror do
 
     context 'when URL does not contain credentials' do
       it 'should show the full URL' do
-        mirror = create_mirror_with_url('http://test.com')
+        mirror = create_mirror(url: 'http://test.com')
 
         expect(mirror.safe_url).to eq('http://test.com')
       end
     end
   end
 
-  def create_mirror_with_url(url)
+  context 'stuck mirrors' do
+    it 'includes mirrors stuck in started with no last_update_at set' do
+      mirror = create_mirror(url: 'http://cantbeblank',
+                             update_status: 'started',
+                             last_update_at: nil,
+                             updated_at: 25.hours.ago)
+
+      expect(RemoteMirror.stuck.last).to eq(mirror)
+    end
+  end
+
+  def create_mirror(params)
     project = FactoryGirl.create(:project)
-    project.remote_mirrors.create!(url: url)
+    project.remote_mirrors.create!(params)
   end
 end