[WIP] Use foreign_key on the base model's name if the associations don't exist #23296

jrafanie · 2024-12-16T22:22:12Z

EmsCluster was properly having ems_cluster_id as the ems_event_filter_column,
subclasses had cluster_id as the column.

Fixes a UI timeline bug shown for Ems clusters as seen below:

Background: #22361 (see the table of models and desired values for ems_event_filter_column and miq_event_filter_column.)

jrafanie · 2024-12-16T22:24:29Z

@kbrock any thoughts on this change? Note, event_where_clause for ems cluster is a real cluster in that it tries to do an or of all host_ids and vm ids from event streams... that feels like it will destroy performance completely. I did just the ems_cluster_id where clause for now as that's what I need to fix the issue but we'll only see timelines for events specific for that cluster, which I think is what we want.

jrafanie · 2024-12-18T20:36:14Z

spec/models/mixins/event_mixin_spec.rb

-        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)


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.

FYI, without the code change, the test fails for each ems cluster subclass like below:

1) EventMixin event_stream_filters ManageIQ::Providers::InfraManager::Cluster uses ems_cluster_id and target_id and target_type Failure/Error: expect(obj.event_stream_filters["EmsEvent"]).to eq(column => obj.id) expected: {"ems_cluster_id"=>nil} got: {"cluster_id"=>nil} (compared using ==) Diff: @@ -1 +1 @@ -"ems_cluster_id" => nil, +"cluster_id" => nil,

kbrock · 2024-12-18T20:37:01Z

app/models/ems_cluster.rb

+  has_many :ems_events,
+            ->(cluster) {
+              where("ems_cluster_id" => cluster.id)


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

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.

kbrock · 2024-12-18T21:10:02Z

Ok. so all in all, showing any even from any object in the cluster seems useful, but since the query is so bad and so many objects come back, it is not useful.

This new change is cluster only events and that reduces the number of events and will speed it up a bit.

I do feel we should update it to use :foreign_key. Or I think we can just use a standard has_many :ems_events and it should work fine.

kbrock

just the one change

kbrock · 2024-12-18T21:10:39Z

app/models/ems_cluster.rb

@@ -182,6 +182,17 @@ def parent_datacenter
    detect_ancestor(:of_type => 'EmsFolder') { |a| a.kind_of?(Datacenter) }
  end

+  has_many :ems_events,


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

FYI, I didn't realize but we had an ems_events method that uses event_where_clause. To minimally fix this, I fixed the ems_event_filter_column.

Fryguy · 2024-12-20T19:54:48Z

EmsCluster was properly having ems_cluster_id as the ems_event_filter_column, subclasses had cluster_id as the column.

I read this and it makes me think the change is to change the subclasses to not change the column name and just use the base class' ems_cluster_id, but that's not the change in the PR, so I don't understand.

jrafanie · 2025-01-09T22:26:21Z

EmsCluster was properly having ems_cluster_id as the ems_event_filter_column, subclasses had cluster_id as the column.

I read this and it makes me think the change is to change the subclasses to not change the column name and just use the base class' ems_cluster_id, but that's not the change in the PR, so I don't understand.

Ok, fixed just this issue and didn't add event_streams association because the method already exists and uses the event_where_clause and brings back cluster, host, vms events for the cluster and not just events for the cluster. We can deal with that afterwards.

jrafanie · 2025-01-09T22:27:09Z

app/models/mixins/event_mixin.rb

-      @ems_event_filter_column ||= reflect_on_association(:ems_events).try(:foreign_key) || name.foreign_key
+      @ems_event_filter_column ||=
+        reflect_on_association(:ems_events).try(:foreign_key)    ||
+        reflect_on_association(:event_streams).try(:foreign_key) || # All Events are Ems Events for some classes right now


Specifically, PhysicalSwitch defines only event_streams.

This is not right, I don't think. EventStream is the base class for all event types, both native provider events and synthetic events; EmsEvent is the sub class for only native events, MiqEvent is the subclass for synthetic events, etc. There are other subclasses for other types of events that we don't want.

EventStream.hierarchy(&:to_s) => {"RequestEvent"=>{}, "MiqEvent"=>{}, "CustomEvent"=>{}, "CustomButtonEvent"=>{}, "EmsEvent"=>{}}

I think Physical Switch is defined wrong and we need to fix it.

I can hold off on fixing this issue for now until I have cycles to add the ems_events association to all the possible timeline classes. It might just be physical switches but I'll need to find out.

Fryguy · 2025-01-10T15:44:00Z

spec/models/mixins/event_mixin_spec.rb

      end
+    it "#{klass} behaves like event_where_clause for ems_events" do


Something is up with the indenting here - not sure if GitHub or the actual code, but the end on line 80 matches the indent of the end on line 92

Thanks, I was not scrolling down to here until now... fixed.

jrafanie · 2025-01-10T15:58:38Z

Putting back in WIP. I will need to revisit the timeline classes and ensure they define ems_events associations for ems_event_filter_column method. Physical switch defines only event_streams which could return ems events and policy/miq events. Also, EmsCluster defines ems_events but it's a method that uses event_where_clause which returns all events for the cluster and includes events for any host and vms in the cluster. We'll need to make that an association and possibly only return just the events for the cluster. I'm not sure if calling code relies on host/vm events come back from ems_events from the cluster.

Will need to review the other timeline classes to ensure they're consistently defining ems_events associations so we can simplify the conditional.

…esn't exist EmsCluster#ems_event_filter_column was correctly ems_cluster_id, subclasses had cluster_id as the column. Fix PhysicalSwitch to correctly implement ems_events association. Previously, it assumed event_streams association was the same as ems_events just because all event_streams for that model have always been ems events. Instead, we implement event_streams association so we can correctly detect the foreign key in ems_event_filter_column. Fix tests to test descendant classes so we can recreate and verify the fix for EmsCluster base class and subclasses. Add HostAggregate and hysicalServerProfile to pending list as they include the mixin but don't implement the interface (event_where_clause, _id column, no ems_events association).

Fryguy · 2025-01-10T19:19:35Z

app/models/host_aggregate.rb

@@ -2,6 +2,9 @@ class HostAggregate < ApplicationRecord
  include SupportsFeatureMixin
  include NewWithTypeStiMixin
  include Metric::CiMixin
+
+  # TODO: this model doesn't have an event_where_clause, doesn't have a *_id in event_streams but includes
+  # event mixin.  Track down if we need timeline events for this as they look to not be completely supported yet.


@agrare Do you know if HostAggregates can have events associated with them?

See also the same question below in other models.

HostAggregates are an openstack thing, I only see vm, host (like InfraManager::Host), and availability_zone in the openstack event parser

The only model which does have events that was touched in this PR is PhysicalSwitch

Do you have any idea why event mixin was included in host aggregates?

That's ultimately the problem. I'm trying to fix a bug in an unrelated model and the inconsistency in these other models becomes readily apparent when I try to expand the tests to cover the other models that pull in the event mixin so I can cover the failure with EmsCluster subclasses. These others don't behave like the other event mixin types. I hope it's a deletion but will need to dig into that in a followup.

Do you have any idea why event mixin was included in host aggregates?

Honestly if I had to guess, they copied another model as the basis for the HostAggregate class.

The include went all the way back to the first commit in this model

commit 5be79d08113b52daa183453fdd709aba73cf7e62 Author: Scott Seago <sseago@redhat.com> Date: Mon Jun 27 11:24:24 2016 -0400 Host aggregate model code

Fryguy · 2025-01-10T19:22:10Z

spec/models/mixins/event_mixin_spec.rb

-        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)
+      skip = true if not_implemented.include?(klass)


You might have to reinitialize on every iteration of the loop? (or maybe not as it's probably nil by default). Regardless, I think this is more explicit.

Suggested change

skip = true if not_implemented.include?(klass)

skip = not_implemented.include?(klass)

jrafanie added bug wip labels Dec 16, 2024

jrafanie requested review from agrare, Fryguy and kbrock as code owners December 16, 2024 22:22

jrafanie commented Dec 18, 2024

View reviewed changes

kbrock reviewed Dec 18, 2024

View reviewed changes

kbrock approved these changes Dec 18, 2024

View reviewed changes

jrafanie force-pushed the fix-undefined-ems-cluster-ems_events-foreign-key branch 2 times, most recently from a55bf58 to 09f654b Compare January 9, 2025 22:23

jrafanie changed the title ~~Add ems_events for ems_event_filter_column for ems cluster subclasses~~ Use foreign_key on the base model's name if the associations don't exist Jan 9, 2025

jrafanie commented Jan 9, 2025

View reviewed changes

jrafanie added wip and removed wip labels Jan 10, 2025

Fryguy reviewed Jan 10, 2025

View reviewed changes

jrafanie changed the title ~~Use foreign_key on the base model's name if the associations don't exist~~ [WIP] Use foreign_key on the base model's name if the associations don't exist Jan 10, 2025

jrafanie force-pushed the fix-undefined-ems-cluster-ems_events-foreign-key branch from 09f654b to 0f27b70 Compare January 10, 2025 18:40

jrafanie force-pushed the fix-undefined-ems-cluster-ems_events-foreign-key branch from 0f27b70 to 73e8504 Compare January 10, 2025 18:43

Fryguy reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use foreign_key on the base model's name if the associations don't exist #23296

[WIP] Use foreign_key on the base model's name if the associations don't exist #23296

jrafanie commented Dec 16, 2024 •

edited

Loading

jrafanie commented Dec 16, 2024

jrafanie Dec 18, 2024

jrafanie Jan 10, 2025

kbrock Dec 18, 2024

jrafanie Dec 18, 2024

kbrock commented Dec 18, 2024

kbrock left a comment

kbrock Dec 18, 2024

jrafanie Jan 10, 2025

Fryguy commented Dec 20, 2024

jrafanie commented Jan 9, 2025

jrafanie Jan 9, 2025

Fryguy Jan 10, 2025

jrafanie Jan 10, 2025

Fryguy Jan 10, 2025

jrafanie Jan 10, 2025

jrafanie commented Jan 10, 2025

Fryguy Jan 10, 2025

Fryguy Jan 10, 2025

agrare Jan 10, 2025

agrare Jan 10, 2025

jrafanie Jan 10, 2025

agrare Jan 11, 2025

Fryguy Jan 10, 2025 •

edited

Loading

		end
		it "#{klass} behaves like event_where_clause for ems_events" do

	skip = true if not_implemented.include?(klass)
	skip = not_implemented.include?(klass)

[WIP] Use foreign_key on the base model's name if the associations don't exist #23296

Are you sure you want to change the base?

[WIP] Use foreign_key on the base model's name if the associations don't exist #23296

Conversation

jrafanie commented Dec 16, 2024 • edited Loading

jrafanie commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Dec 18, 2024

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Dec 20, 2024

jrafanie commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

jrafanie commented Dec 16, 2024 •

edited

Loading

Fryguy Jan 10, 2025 •

edited

Loading