Don't allow objects into MiqQueue. #8963
Conversation
kbrock
force-pushed
the
miq_queue_prevent_objects
branch
2 times, most recently
from
81e7fc4
to
edacfe7
Compare
|
@Fryguy looks like this only detected that one place in code where a report is put on the queue. It is for new records only (can't serialize the id here) - ideas how to address / if we want to address it? |
kbrock
changed the title
|
@Fryguy we agreed that for MiqReport, if it was a new record, let it go onto the queue. Question: Do we want this check to go into miq_queue? |
kbrock
force-pushed
the
miq_queue_prevent_objects
branch
from
edacfe7
to
747911f
Compare
|
I'm not sure about this one. On one hand I like the idea of blocking AR objects on the queue a lot. On the other hand, I'm not sure about what restrictions this might impose to using the queue. @jrafanie @gtanzillo @chessbyte Thoughts? |
|
@Fryguy @kbrock Seems ok to me. So if there are places where we need to existing objects on the queue we should be putting just the id or some other identifying info? I'm a bit surprised that those were the only places you found where we're doing this. I recall it being done in a lot more places. Perhaps those have been cleaned up over the years. |
|
@gtanzillo Yes. The object on the queue can get big, and tends to get stale. So we have been all but removed them. @Fryguy This is development only. Does that change your mind? I'm hearing you say close w/o merge but unsure. |
|
I agree with @gtanzillo, I'm surprised there were only 2 places that did this. I'm ok with this as we try to see if this happens elsewhere. |
| @@ -116,6 +116,11 @@ def self.put(options) | |||
|
|
|||
| options[:args] = [options[:args]] if options[:args] && !options[:args].kind_of?(Array) | |||
|
|
|||
| if !Rails.env.production? && options[:args] && | |||
| (arg = options[:args].detect { |a| a.kind_of?(ActiveRecord::Base) && ! a.new_record? }) | |||
| raise "MiqQueue.put(:args =>[]) does not support #{arg.class.name} objects" | |||
There was a problem hiding this comment.
This doesn't help people fix the code though... should we suggest you provide class and id so the resulting method can lookup the record instead of us serializing it on the queue?
|
@jrafanie I only had |
kbrock
force-pushed
the
miq_queue_prevent_objects
branch
3 times, most recently
from
216de8f
to
ff41dae
Compare
| @@ -116,6 +116,11 @@ def self.put(options) | |||
|
|
|||
| options[:args] = [options[:args]] if options[:args] && !options[:args].kind_of?(Array) | |||
|
|
|||
| if !Rails.env.production? && options[:args] && | |||
| (arg = options[:args].detect { |a| a.kind_of?(ActiveRecord::Base) && ! a.new_record? }) | |||
| raise "MiqQueue.put(:args =>[]) does not support #{arg.class.name}[#{arg.try(:id)}] objects" | |||
There was a problem hiding this comment.
@kbrock Maybe something like this:
raise "MiqQueue.put(:args =>[]) does not support #{arg.class.name} objects, try passing the object(s) class name and id as args to #{options[:class_name]} #{options[:method_name]} instead"
ff41dae
to
ac0fcc2
Compare
This does not raise in production. It is there to assist developers In a few instances, a changed (non-created) object is passed on the queue. We may want to address this issue at a later time
kbrock
force-pushed
the
miq_queue_prevent_objects
branch
from
ac0fcc2
to
8fa5c63
Compare
|
Checked commit kbrock@8fa5c63 with ruby 2.2.4, rubocop 0.37.2, and haml-lint 0.16.1 app/models/miq_queue.rb
|
|
@jrafanie added method and class to alert message - now are you good? |
|
It's better @kbrock. Thanks. |
For tests and development only, ensure no ar objects are inserted into the queue.
Currently, a Vm is being inserted into the Queue for refresh and it is very large. This puts some protective measure for developers to catch these errors early on in the development process
code to be fixed: