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 ems_events for ems_event_filter_column for ems cluster subclasses #23296

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions app/models/ems_cluster.rb
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,17 @@ def parent_datacenter
detect_ancestor(:of_type => 'EmsFolder') { |a| a.kind_of?(Datacenter) }
end

has_many :ems_events,
Copy link
Member

Choose a reason for hiding this comment

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

yea. I think we can just use has_many :ems_events.

->(cluster) {
where("ems_cluster_id" => cluster.id)
Comment on lines +185 to +187
Copy link
Member

Choose a reason for hiding this comment

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

at least in terms of how ems_cluster_id is concerned, can we use foreign_key?

And getting rid of the or seems to change this quite a bit

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely. Great idea. It feels like this was a POC for a fix for the first issue of not being able to show cluster subclasses on the timelines. I had a test for the base cluster class but subclasses weren't tested so I recreated it with them and was able to fix it with the code above.

For the second question, do we show other objects on the cluster timeline such as events from vms or hosts in the cluster, I think that's secondary question. We would need to ensure the UI passes limits to display it properly. I don't know if we should cowardly refuse to try to draw a timeline with 10,000+ events on it. For now, I was going to try to fix the bug first and then open it up for discussion on how to properly display it for large number of objects/events.

# TODO: event_where_clause tacks on additional possibly expensive queries
# such as all events for all hosts or vms in the cluster
#.or(where("host_id" => host_ids)).
#or(where("dest_host_id" => host_ids)).
#or(where("vm_or_template_id" => vm_or_template_ids)).
#or(where("dest_vm_or_template_id" => vm_or_template_ids))
},
:class_name => "EmsEvent"
def event_where_clause(assoc = :ems_events)
return ["ems_cluster_id = ?", id] if assoc.to_sym == :policy_events

Expand Down
32 changes: 19 additions & 13 deletions spec/models/mixins/event_mixin_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,20 +58,26 @@ def event_where_clause(assoc)
VmOrTemplate vm_or_template_id
Vm vm_or_template_id
].each_slice(2) do |klass, column|
it "#{klass} uses #{column} and target_id and target_type" do
obj = FactoryBot.create(klass.tableize.singularize)
expect(obj.event_stream_filters["EmsEvent"]).to eq(column => obj.id)
expect(obj.event_stream_filters.dig("MiqEvent", "target_id")).to eq(obj.id)
expect(obj.event_stream_filters.dig("MiqEvent", "target_type")).to eq(obj.class.base_class.name)
end

it "#{klass} behaves like event_where_clause for ems_events" do
obj = FactoryBot.create(klass.tableize.singularize)
event = FactoryBot.create(:event_stream, column => obj.id)
FactoryBot.create(:event_stream)
expect(EventStream.where(obj.event_stream_filters["EmsEvent"]).to_a).to eq([event])
expect(EventStream.where(obj.event_where_clause(:ems_events)).to_a).to eq([event])
klasses = klass.constantize.descendants.collect(&:name).unshift(klass)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note, EmsCluster does the right thing, but subclasses weren't. This test recreated reported issue so the fix above can verify the problem is addressed.

klasses.each do |klass|
it "#{klass} uses #{column} and target_id and target_type" do
begin
obj = FactoryBot.build(klass.tableize.singularize, :name => "test")
rescue NameError
skip "Unable to build factory from name: #{klass}, possibly due to inflections"
end
expect(obj.event_stream_filters["EmsEvent"]).to eq(column => obj.id)
expect(obj.event_stream_filters.dig("MiqEvent", "target_id")).to eq(obj.id)
expect(obj.event_stream_filters.dig("MiqEvent", "target_type")).to eq(obj.class.base_class.name)
end
end
it "#{klass} behaves like event_where_clause for ems_events" do
obj = FactoryBot.create(klass.tableize.singularize)
event = FactoryBot.create(:event_stream, column => obj.id)
FactoryBot.create(:event_stream)
expect(EventStream.where(obj.event_stream_filters["EmsEvent"]).to_a).to eq([event])
expect(EventStream.where(obj.event_where_clause(:ems_events)).to_a).to eq([event])
end

# # TODO: some classes don't have this implemented or don't have columns for this
# # Do we consolidate policy events and miq events?
Expand Down
Loading