From 04c129c9c7541420bfc3b62994a6f16097d4ab85 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Sat, 4 Feb 2017 00:59:44 +0800 Subject: [PATCH] Squashed commit of the following: commit 1ee838190e7f7e93dfe50ba26dde549d0fa1aa4e Author: Lin Jen-Shin Date: Sat Feb 4 00:21:29 2017 +0800 No need to tick the queue when creating the runner Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8664/diffs#note_22190504 commit 985c68a1ec34eedfe3d6e5b8ca2020af9f9c3457 Merge: 7a4d723e6b 98e6e3062d Author: Lin Jen-Shin Date: Sat Feb 4 00:20:54 2017 +0800 Merge remote-tracking branch 'upstream/master' into use-update-runner-service * upstream/master: (589 commits) Backport changes from EE squash Fix broken tests Rename Build to Job Adds changelog entry Change "Build" to "Job" in builds show page header and sidebar replace `find_with_namespace` with `find_by_full_path` PlantUML support for Markdown remove dateFormat global exception fix relative paths to xterm.js within fit.js use setFixtures instead of fixture.set prevent u2f tests from triggering a form submission while testing simplify test for focus state preload projects.json fixture preload projects.json fixture rework tests which rely on teaspoon-specific behavior Only render hr when user can't archive project. use setFixtures instead of fixture.set ensure helper classes and constants are exposed globally preload projects.json fixture fix fixture references in environments_spec allow console.xxx in tests, reorder eslint rules alphabetically ... commit 7a4d723e6ba287fe792dca0a8ddc3d8a77b1876c Author: Lin Jen-Shin Date: Sat Jan 21 02:38:58 2017 +0800 Remove the key from the queue when runner is deleted commit 7a109402a866db1c84ef9af1c14e148ec944aa22 Author: Lin Jen-Shin Date: Fri Jan 20 21:57:01 2017 +0800 Prefer service object over after_save hook Closes #26921 --- app/controllers/admin/runners_controller.rb | 6 +++--- .../projects/runners_controller.rb | 6 +++--- app/models/ci/runner.rb | 16 +++++++------- app/services/ci/update_runner_service.rb | 15 +++++++++++++ spec/models/ci/runner_spec.rb | 21 ++++++++++++++++++- .../services/ci/update_runner_service_spec.rb | 18 ++++++++++++++++ 6 files changed, 67 insertions(+), 15 deletions(-) create mode 100644 app/services/ci/update_runner_service.rb create mode 100644 spec/services/ci/update_runner_service_spec.rb diff --git a/app/controllers/admin/runners_controller.rb b/app/controllers/admin/runners_controller.rb index 7345c91f67df3b..348641e5ecb5a9 100644 --- a/app/controllers/admin/runners_controller.rb +++ b/app/controllers/admin/runners_controller.rb @@ -13,7 +13,7 @@ def show end def update - if @runner.update_attributes(runner_params) + if Ci::UpdateRunnerService.new(@runner).update(runner_params) respond_to do |format| format.js format.html { redirect_to admin_runner_path(@runner) } @@ -31,7 +31,7 @@ def destroy end def resume - if @runner.update_attributes(active: true) + if Ci::UpdateRunnerService.new(@runner).update(active: true) redirect_to admin_runners_path, notice: 'Runner was successfully updated.' else redirect_to admin_runners_path, alert: 'Runner was not updated.' @@ -39,7 +39,7 @@ def resume end def pause - if @runner.update_attributes(active: false) + if Ci::UpdateRunnerService.new(@runner).update(active: false) redirect_to admin_runners_path, notice: 'Runner was successfully updated.' else redirect_to admin_runners_path, alert: 'Runner was not updated.' diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index 53c36635efe1ec..ff75c408beb84b 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -16,7 +16,7 @@ def edit end def update - if @runner.update_attributes(runner_params) + if Ci::UpdateRunnerService.new(@runner).update(runner_params) redirect_to runner_path(@runner), notice: 'Runner was successfully updated.' else render 'edit' @@ -32,7 +32,7 @@ def destroy end def resume - if @runner.update_attributes(active: true) + if Ci::UpdateRunnerService.new(@runner).update(active: true) redirect_to runner_path(@runner), notice: 'Runner was successfully updated.' else redirect_to runner_path(@runner), alert: 'Runner was not updated.' @@ -40,7 +40,7 @@ def resume end def pause - if @runner.update_attributes(active: false) + if Ci::UpdateRunnerService.new(@runner).update(active: false) redirect_to runner_path(@runner), notice: 'Runner was successfully updated.' else redirect_to runner_path(@runner), alert: 'Runner was not updated.' diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index ed1843ba0053e8..07a086b0acae00 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -22,8 +22,6 @@ class Runner < ActiveRecord::Base scope :online, ->() { where('contacted_at > ?', LAST_CONTACT_TIME) } scope :ordered, ->() { order(id: :desc) } - after_save :tick_runner_queue, if: :form_editable_changed? - scope :owned_or_shared, ->(project_id) do joins('LEFT JOIN ci_runner_projects ON ci_runner_projects.runner_id = ci_runners.id') .where("ci_runner_projects.gl_project_id = :project_id OR ci_runners.is_shared = true", project_id: project_id) @@ -40,6 +38,8 @@ class Runner < ActiveRecord::Base acts_as_taggable + after_destroy :cleanup_runner_queue + # Searches for runners matching the given query. # # This method uses ILIKE on PostgreSQL and LIKE on MySQL. @@ -147,14 +147,14 @@ def is_runner_queue_value_latest?(value) private - def runner_queue_key - "runner:build_queue:#{self.token}" + def cleanup_runner_queue + Gitlab::Redis.with do |redis| + redis.del(runner_queue_key) + end end - def form_editable_changed? - FORM_EDITABLE.any? do |editable| - public_send("#{editable}_changed?") - end + def runner_queue_key + "runner:build_queue:#{self.token}" end def tag_constraints diff --git a/app/services/ci/update_runner_service.rb b/app/services/ci/update_runner_service.rb new file mode 100644 index 00000000000000..198b29b3a9d05c --- /dev/null +++ b/app/services/ci/update_runner_service.rb @@ -0,0 +1,15 @@ +module Ci + class UpdateRunnerService + attr_reader :runner + + def initialize(runner) + @runner = runner + end + + def update(params) + runner.update(params).tap do + runner.tick_runner_queue + end + end + end +end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 3f32248e52b0d9..f8513ac8b1c1ba 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -290,7 +290,7 @@ let!(:last_update) { runner.ensure_runner_queue_value } before do - runner.update(description: 'new runner') + Ci::UpdateRunnerService.new(runner).update(description: 'new runner') end it 'sets a new last_update value' do @@ -318,6 +318,25 @@ def expect_value_in_redis end end + describe '#destroy' do + let(:runner) { create(:ci_runner) } + + context 'when there is a tick in the queue' do + let!(:queue_key) { runner.send(:runner_queue_key) } + + before do + runner.tick_runner_queue + runner.destroy + end + + it 'cleans up the queue' do + Gitlab::Redis.with do |redis| + expect(redis.get(queue_key)).to be_nil + end + end + end + end + describe '.assignable_for' do let(:runner) { create(:ci_runner) } let(:project) { create(:empty_project) } diff --git a/spec/services/ci/update_runner_service_spec.rb b/spec/services/ci/update_runner_service_spec.rb new file mode 100644 index 00000000000000..8429881dd15129 --- /dev/null +++ b/spec/services/ci/update_runner_service_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe Ci::UpdateRunnerService, services: true do + let(:runner) { create(:ci_runner) } + + describe '#update' do + before do + allow(runner).to receive(:tick_runner_queue) + + described_class.new(runner).update(description: 'new runner') + end + + it 'updates the runner and ticking the queue' do + expect(runner.description).to eq('new runner') + expect(runner).to have_received(:tick_runner_queue) + end + end +end -- GitLab