Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add button to suspend worker #94

Merged
merged 13 commits into from
Mar 7, 2018
17 changes: 17 additions & 0 deletions app/controllers/kuroko2/workers_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,22 @@
class Kuroko2::WorkersController < Kuroko2::ApplicationController
before_action :set_worker, only: %i(update)

def index
@workers = Kuroko2::Worker.ordered.all
end

def update
@worker.update_attributes(worker_params)
redirect_to workers_path
end

private

def set_worker
@worker = Kuroko2::Worker.find(params[:id])
end

def worker_params
params.permit(:suspended)
end
end
26 changes: 24 additions & 2 deletions app/views/kuroko2/workers/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,18 @@
th.col-md-1 WID
th.col-md-2 Queue
th.col-md-1 Status
th.col-md-5 Execution
th.col-md-4 Execution
th.col-md-1 &nbsp;
th.col-md-1 &nbsp;
- for worker in @workers
tr
td= worker.hostname
td= worker.worker_id
td= worker.queue
td
- if worker.working
- if worker.suspended
span.label.label-warning SUSPENDED
- elsif worker.working
span.label.label-primary WORKING
- else
'&nbsp;
Expand All @@ -39,3 +42,22 @@
role: 'button', class: 'btn btn-sm btn-default')
- else
'&nbsp;
td
- if !worker.suspendable
'&nbsp;
- elsif !worker.suspended
= button_to('Suspend',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) how about give confirm dialog to user? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If user suspend worker accidentally, the user can unsuspend the worker. So I think confirm dialog is unneeded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my case, I don't want to suspend a worker accidentally, even though I know I can restore it with only one click. anyway, that's not a big problem(why I tagged nit) ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I added confirm dialog in eb5520e.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks!! XD

worker_path(worker),
method: :patch,
params: { suspended: true },
role: 'button',
class: 'btn btn-xs btn-default',
data: { confirm: 'Continue suspending the worker? If not, use "Cancel" button.' })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nits] "If not, use "Cancel" button" sounds too obvious to me

- else
= button_to('Unsuspend',
worker_path(worker),
method: :patch,
params: { suspended: false },
role: 'button',
class: 'btn btn-xs btn-default',
data: { confirm: 'Continue unsuspending the worker? If not, use "Cancel" button.' })
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
get 'page/:page', action: :index, on: :collection, as: 'paged'
end

resources :workers, only: :index
resources :workers, only: %i(index update)
resources :job_instances, path: 'instances', only: %w() do
get :working, action: :working, on: :collection
end
Expand Down
6 changes: 6 additions & 0 deletions db/migrate/031_add_suspended_to_workers.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddSuspendedToWorkers < ActiveRecord::Migration[5.0]
def change
add_column :workers, :suspendable, :boolean, default: false, null: false, after: :execution_id
add_column :workers, :suspended, :boolean, default: false, null: false, after: :suspendable
end
end
3 changes: 2 additions & 1 deletion lib/autoload/kuroko2/command/executor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ def initialize
elsif worker_id == (Command::Executor.num_workers - 1)
Command::Monitor.new(hostname: @hostname, worker_id: worker_id)
else
@worker = Worker.where(hostname: @hostname, worker_id: worker_id, queue: @queue).first_or_create!
@worker = Worker.where(hostname: @hostname, worker_id: worker_id, queue: @queue).first_or_initialize!
@worker.update_column(:suspendable, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update_column skips varidations and callbacks and doesn't update updated_at. Why did you choose update_column instead of update! ?
http://api.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-update_column

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first_or_initialize! method doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I fixed in 34839bc.

Command::Shell.new(hostname: @hostname, worker_id: worker_id, worker: @worker, queue: @queue)
end
end
Expand Down
1 change: 1 addition & 0 deletions lib/autoload/kuroko2/command/shell.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ def initialize(hostname:, worker_id: 0, worker:, queue: Execution::DEFAULT_QUEUE

def execute
@worker.reload
return nil if @worker.suspended?
unless @worker.execution_id?
if (execution = Execution.poll(@queue))
do_execute(execution)
Expand Down
10 changes: 10 additions & 0 deletions spec/command/shell_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ module Kuroko2::Command
end
end

context 'when all workers are suspended' do
let(:worker) { create(:worker, suspended: true) }

it 'skips execution' do
is_expected.to be_nil
execution.reload
expect(execution.started_at).to be_nil
end
end

describe 'memory expectancy calculation' do
let(:worker) { create(:worker) }
let(:memory_expectancy) { execution.job_definition.memory_expectancy }
Expand Down
14 changes: 14 additions & 0 deletions spec/controllers/workers_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,18 @@

RSpec.describe Kuroko2::WorkersController, :type => :controller do
routes { Kuroko2::Engine.routes }

before { sign_in }

let(:worker) { create(:worker) }

describe '#update' do
subject! { patch :update, params: { id: worker.id, suspended: true } }

it do
expect(response).to redirect_to(workers_path)

expect(assigns(:worker).suspended).to be_truthy
end
end
end
4 changes: 3 additions & 1 deletion spec/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 30) do
ActiveRecord::Schema.define(version: 31) do

create_table "admin_assignments", id: :integer, force: :cascade, options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4" do |t|
t.integer "user_id", null: false
Expand Down Expand Up @@ -196,6 +196,8 @@
t.string "queue", limit: 180, default: "@default", null: false
t.boolean "working", default: false, null: false
t.integer "execution_id"
t.boolean "suspendable", default: false, null: false
t.boolean "suspended", default: false, null: false
t.datetime "created_at"
t.datetime "updated_at"
t.index ["hostname", "worker_id"], name: "hostname", unique: true
Expand Down
1 change: 1 addition & 0 deletions spec/factories/worker_factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@
sequence(:worker_id)
queue "@default"
working true
suspendable true
end
end
33 changes: 33 additions & 0 deletions spec/features/workers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,37 @@
expect(page).not_to have_content('echo Hello!')
expect(page).to have_selector('#workers table tbody tr td .btn', text: 'Details', count: 0)
end

context 'toggle suspended' do
it js: true do
visit kuroko2.workers_path
expect(page).to have_selector('#workers table tbody tr', count: 2)
expect(page).not_to have_content('echo Hello!')
expect(page).to have_title('Kuroko Workers « Kuroko 2')
expect(page).to have_selector('i.fa.fa-rocket', text: '')
have_selector('h1', text: /Kuroko Workers/)

expect(page).to have_selector('#workers table tbody tr', count: 2)
expect(page).to have_selector('#workers table tbody tr td .btn[value=Suspend]', count: 1)
expect(page).to have_selector('#workers table tbody tr td .btn[value=Unsuspend]', count: 0)
expect(page).to have_selector('#workers table tbody tr td .label', text: 'WORKING', count: 1)
expect(page).to have_selector('#workers table tbody tr td .label', text: 'SUSPENDED', count: 0)

click_on 'Suspend'

expect(page).to have_selector('#workers table tbody tr', count: 2)
expect(page).to have_selector('#workers table tbody tr td .btn[value=Suspend]', count: 0)
expect(page).to have_selector('#workers table tbody tr td .btn[value=Unsuspend]', count: 1)
expect(page).to have_selector('#workers table tbody tr td .label', text: 'WORKING', count: 0)
expect(page).to have_selector('#workers table tbody tr td .label', text: 'SUSPENDED', count: 1)

click_on 'Unsuspend'

expect(page).to have_selector('#workers table tbody tr', count: 2)
expect(page).to have_selector('#workers table tbody tr td .btn[value=Suspend]', count: 1)
expect(page).to have_selector('#workers table tbody tr td .btn[value=Unsuspend]', count: 0)
expect(page).to have_selector('#workers table tbody tr td .label', text: 'WORKING', count: 1)
expect(page).to have_selector('#workers table tbody tr td .label', text: 'SUSPENDED', count: 0)
end
end
end