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.

Sign up for GitHub

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

Jump to bottom

If we can't update_attributes on a queue row, set state to error #14365

Merged
merged 1 commit into from
Mar 28, 2017
Merged

If we can't update_attributes on a queue row, set state to error #14365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/models/miq_queue_worker_base/runner.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def thresholds_exceeded?
def get_message_via_drb
loop do
begin
msg_id, lock_version = @worker_monitor_drb.get_queue_message(@worker.pid)
msg_id, lock_version = worker_monitor_drb.get_queue_message(@worker.pid)
Copy link
Member Author

Choose a reason for hiding this comment

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

We already have a attr_reader on the @worker_monitor_drb so i'm using it here so it's easier to test.

rescue DRb::DRbError => err
do_exit("Error communicating with WorkerMonitor because <#{err.message}>", 1)
end
Expand Down Expand Up @@ -64,6 +64,7 @@ def get_message_via_drb
_log.debug("#{log_prefix} #{MiqQueue.format_short_log_msg(msg)} stale, retrying...")
next
rescue => err
msg.update_column(:state, MiqQueue::STATUS_ERROR)
Copy link
Member

Choose a reason for hiding this comment

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

What if this fails? Say, if the database is down? should we do some kind of rescue nil here?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is needed.

If we have got here, the database was running a second ago. If it is no longer running, we cannot do anything sensible at that point, thus we can fail horribly.

Copy link
Member Author

Choose a reason for hiding this comment

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

So @Fryguy I was debating if I should just delete the row if this fails but I really can't understand how this line could throw an error if we retrieved the msg on line 45. We're bypassing all other columns and validations. If we fail here, the only recourse is to raise. Who knows if a delete of the row would work either.

raise _("%{log} \"%{error}\" attempting to get next message") % {:log => log_prefix, :error => err}
end
end
Expand Down
1 change: 1 addition & 0 deletions spec/factories/miq_worker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
status "ready"
end

factory :miq_generic_worker, :class => "MiqGenericWorker", :parent => :miq_worker
factory :miq_ui_worker, :class => "MiqUiWorker", :parent => :miq_worker
factory :miq_websocket_worker, :class => "MiqWebsocketWorker", :parent => :miq_worker

Expand Down
24 changes: 24 additions & 0 deletions spec/models/miq_queue_worker_base/runner_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
describe MiqQueueWorkerBase::Runner do
context "#get_message_via_drb" do
let(:server) { EvmSpecHelper.local_miq_server }
let(:worker) { FactoryGirl.create(:miq_generic_worker, :miq_server => server, :pid => 123) }
let(:runner) do
allow_any_instance_of(MiqQueueWorkerBase::Runner).to receive(:sync_active_roles)
allow_any_instance_of(MiqQueueWorkerBase::Runner).to receive(:sync_config)
allow_any_instance_of(MiqQueueWorkerBase::Runner).to receive(:set_connection_pool_size)
described_class.new(:guid => worker.guid)
end

it "sets message to 'error' and raises for unhandled exceptions" do
# simulate what may happen if invalid yaml is deserialized
allow_any_instance_of(MiqQueue).to receive(:args).and_raise(ArgumentError)
q1 = FactoryGirl.create(:miq_queue)
allow(runner)
.to receive(:worker_monitor_drb)
.and_return(double(:get_queue_message => [q1.id, q1.lock_version]))

expect { runner.get_message_via_drb }.to raise_error(StandardError)
expect(q1.reload.state).to eql(MiqQueue::STATUS_ERROR)
end
end
end