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

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

Merged
merged 1 commit into from
Jan 16, 2025
Merged

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

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
6 changes: 6 additions & 0 deletions app/models/ems_cluster.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,12 @@ def event_where_clause(assoc = :ems_events)
cond_params
end

# TODO: ems_events already exists as a method but it uses
# event_where_clause which tacks on additional possibly expensive queries
# such as all events for all hosts or vms in the cluster.
# Should we move to looking at ems events specifically for the ems cluster?
# like this:
# has_many :ems_events, :foreign_key => :ems_cluster_id
def ems_events
ewc = event_where_clause
return [] if ewc.blank?
Expand Down
3 changes: 3 additions & 0 deletions app/models/host_aggregate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

See also the same question below in other models.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

include EventMixin
include ProviderObjectMixin

Expand Down
2 changes: 1 addition & 1 deletion app/models/mixins/event_mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@kbrock kbrock Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

UPDATE:
Guess HostAggregate is an example of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

end

def miq_event_filter_column
Expand Down
3 changes: 3 additions & 0 deletions app/models/physical_server_profile.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ class PhysicalServerProfile < ApplicationRecord
include NewWithTypeStiMixin
include TenantIdentityMixin
include SupportsFeatureMixin

# 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.
include EventMixin
include ProviderObjectMixin
include EmsRefreshMixin
Expand Down
4 changes: 4 additions & 0 deletions app/models/physical_switch.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ class PhysicalSwitch < Switch
has_one :asset_detail, :as => :resource, :dependent => :destroy, :inverse_of => :resource
has_one :hardware, :dependent => :destroy, :foreign_key => :switch_id, :inverse_of => :physical_switch
has_many :physical_network_ports, :dependent => :destroy, :foreign_key => :switch_id

# TODO: Deprecate event_streams if it makes sense, find callers, and change to use ems_events. Even though event_streams
# have only ever been ems_events in this model, we shouldn't rely on that, so callers should use ems_events.
has_many :event_streams, :inverse_of => :physical_switch, :dependent => :nullify
has_many :ems_events, :inverse_of => :physical_switch, :dependent => :nullify

has_many :connected_components, :through => :physical_network_ports, :source => :connected_computer_system

Expand Down
30 changes: 25 additions & 5 deletions spec/models/mixins/event_mixin_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,26 +46,46 @@ def event_where_clause(assoc)
# 2) EmsCluster#event_where_clause does an OR: ems_cluster_id OR (host_id in ? OR dest_host_id IN ?) OR (vm_or_template in ? OR dest_vm_or_template_id in ?) (for now we just do ems_cluster_id in ems_event_filter)
# 3) Host#event_where_clause does an OR: host_id OR dest_host_id... right now we only do host_id in ems_event_filter
# 4) VmOrTemplate#event_where_clause does an OR: vm_or_template_id OR dest_vm_or_template_id, we only do vm_or_template_id in ems_event_filter
# 5) The following are tested below in the custom behavior section because they have different setup:
# Container container_id
# ContainerGroup container_group_id
# ContainerNode container_node_id
# ContainerProject container_project_id
# ContainerReplicator container_replicator_id
# The following classes are missing the event_where_clause and their _id column isn't in event_streams table... fix this by removing event mixin or making it work by adding the column.
not_implemented = %w[HostAggregate PhysicalServerProfile]
%w[
AvailabilityZone availability_zone_id
EmsCluster ems_cluster_id
ExtManagementSystem ems_id
Host host_id
HostAggregate host_aggregate_id
PhysicalChassis physical_chassis_id
PhysicalServer physical_server_id
PhysicalServerProfile physical_server_profile_id
PhysicalStorage physical_storage_id
PhysicalSwitch physical_switch_id
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)
skip = not_implemented.include?(klass)
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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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,

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

@Fryguy Fryguy Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

klasses.each do |klass|
it "#{klass} uses #{column} and target_id and target_type" do
skip("Class: #{klass} hasn't fully implemented timeline events") if skip
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
skip("Class: #{klass} hasn't implemented timeline events. This could be due to no event_where_clause, ems_events association, and/or _id column in event_streams") if skip
obj = FactoryBot.create(klass.tableize.singularize)
event = FactoryBot.create(:event_stream, column => obj.id)
FactoryBot.create(:event_stream)
Expand Down
Loading