Use foreign_key on the base model's name if ems_events association doesn't exist #23296

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

EmsCluster#ems_event_filter_column was correctly ems_cluster_id,
subclasses were wrong as it returned 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 PhysicalServerProfile to pending list
as they include the mixin but don't implement the interface (event_where_clause, _id column, no ems_events association).

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,

ASIDE: Really wish we were going away from descendants.
Also, if we start with klass, do we end up with a bunch of queries instead of one query with an IN clause?

This error suggests there is a problem with obj.event_where_clause, right? Looks like a missing/incorrect foreign_key in an association. Or when building the where() clause, it is using the wrong base class to generate the sql.

We used to use string keys instead of symbol keys because it fixed foreign keys while building sql in a generic way. Wonder if a newer version of rails changed that behavior. (it is also possible that I'm spouting a random and unrelated fact)

This particular code is just building a spec for each of the descendant classes - this seems like an appropriate use of descendants.

This error suggests there is a problem with obj.event_where_clause, right? Looks like a missing/incorrect foreign_key in an association. Or when building the where() clause, it is using the wrong base class to generate the sql.

To be fair, some event_where_clause are completely unusable in UI timelines as they do more complex joins on possibly multiple tables such as showing events for all objects under the cluster (host, vm, templates, etc.) as opposed to just events for the cluster. The first cut of event_stream_filters tried to do just a singular column "foreign key reference" as timelines can easily be impossible to view in the UI with too many events to display.

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.

spec/models/mixins/event_mixin_spec.rb

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.

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

Are we good with a TODO to kick the can down the road?

yeah, TODO is probably fine - no need to fix it all at once. That being said, I still want to discuss because I still don't understand the overall issue nor the fix.

I would think we'd unwind event_mixin inclusion in models that shouldn't need it.

spec/models/mixins/event_mixin_spec.rb

…esn't exist EmsCluster#ems_event_filter_column was correctly ems_cluster_id, subclasses were wrong as they 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).

jrafanie · 2025-01-16T15:20:40Z

ok, this is green and ready to go

kbrock · 2025-01-16T17:05:54Z

app/models/mixins/event_mixin.rb

@@ -63,7 +63,7 @@ def find_one_event(assoc, order)

  module ClassMethods
    def ems_event_filter_column
-      @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) || base_model.name.foreign_key


Is there a reason why the ems_events reflection isn't present for all base classes?

UPDATE:
Guess HostAggregate is an example of this.

see the other discussion, they should be fixed but not here.

Fryguy · 2025-01-16T20:00:25Z

Please can we discuss over voice before merge - I really don't understand this PR at all.

Fryguy

Discussed with @jrafanie and now I understand. I was struggling to understand what PhysicalSwitch had to do with EmsCluster, but it was a side-effect of testing the EmsCluster fix on subclasses, which uncovered a problem with PhysicalSwitch.

The reason cluster_id showed up is because the subclasses of EmsCluster in the provider are typically just ManageIQ::Providers::...::Cluster, and so name.foreign_key is just cluster_id.

Fryguy · 2025-01-16T20:27:36Z

Backported to radjabov in commit eccb6a2.

commit eccb6a2ca3c9eaa92213ef368df3c01f68df1b77
Author: Jason Frey <fryguy9@gmail.com>
Date:   Thu Jan 16 15:27:02 2025 -0500

    Merge pull request #23296 from jrafanie/fix-undefined-ems-cluster-ems_events-foreign-key
    
    Use foreign_key on the base model's name if ems_events association doesn't exist
    
    (cherry picked from commit 830d608621fea1af2ca55235c56a52247c45d7ca)

…_events-foreign-key Use foreign_key on the base model's name if ems_events association doesn't exist (cherry picked from commit 830d608)

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

spec/models/mixins/event_mixin_spec.rb Outdated Show resolved Hide resolved

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 2 times, most recently from 0f27b70 to 73e8504 Compare January 10, 2025 18:43

Fryguy reviewed Jan 10, 2025

View reviewed changes

spec/models/mixins/event_mixin_spec.rb Outdated Show resolved Hide resolved

jrafanie force-pushed the fix-undefined-ems-cluster-ems_events-foreign-key branch from 73e8504 to 5278c3f Compare January 13, 2025 16:18

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

jrafanie removed the wip label Jan 13, 2025

Fryguy self-assigned this Jan 15, 2025

Fryguy assigned kbrock Jan 15, 2025

jrafanie added the radjabov/yes label Jan 16, 2025

kbrock reviewed Jan 16, 2025

View reviewed changes

Fryguy approved these changes Jan 16, 2025

View reviewed changes

Fryguy merged commit 830d608 into ManageIQ:master Jan 16, 2025
8 checks passed

Fryguy removed the radjabov/yes label Jan 16, 2025

Fryguy added a commit that referenced this pull request Jan 16, 2025

Merge pull request #23296 from jrafanie/fix-undefined-ems-cluster-ems…

eccb6a2

…_events-foreign-key Use foreign_key on the base model's name if ems_events association doesn't exist (cherry picked from commit 830d608)

Fryguy added the radjabov/backported label Jan 16, 2025

kbrock deleted the fix-undefined-ems-cluster-ems_events-foreign-key branch January 17, 2025 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use foreign_key on the base model's name if ems_events association doesn't exist #23296

Use foreign_key on the base model's name if ems_events association doesn't exist #23296

jrafanie commented Dec 16, 2024 •

edited

Loading

jrafanie commented Dec 16, 2024

jrafanie Dec 18, 2024

jrafanie Jan 10, 2025

kbrock Jan 13, 2025

Fryguy Jan 13, 2025 •

edited

Loading

jrafanie Jan 14, 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

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

jrafanie Jan 14, 2025

Fryguy Jan 15, 2025

jrafanie Jan 15, 2025

jrafanie commented Jan 16, 2025

kbrock Jan 16, 2025 •

edited

Loading

jrafanie Jan 16, 2025

Fryguy commented Jan 16, 2025

Fryguy left a comment •

edited

Loading

Fryguy commented Jan 16, 2025

Use foreign_key on the base model's name if ems_events association doesn't exist #23296

Use foreign_key on the base model's name if ems_events association doesn'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

Fryguy Jan 13, 2025 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Jan 16, 2025

kbrock Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jan 16, 2025

Fryguy left a comment • edited Loading

Choose a reason for hiding this comment

Fryguy commented Jan 16, 2025

jrafanie commented Dec 16, 2024 •

edited

Loading

Fryguy Jan 13, 2025 •

edited

Loading

kbrock Jan 16, 2025 •

edited

Loading

Fryguy left a comment •

edited

Loading