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

[WIP] Virtual attributes for Containers and friends (performance changes) #22575

Open
wants to merge 2 commits 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
32 changes: 9 additions & 23 deletions app/models/container_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ class ContainerGroup < ApplicationRecord
# :labels, :restart_policy, :dns_policy

has_many :containers, :dependent => :destroy
has_many :container_images, -> { distinct }, :through => :containers
has_many :running_containers, -> { where(:state => "running") }, :class_name => "Container", :inverse_of => :container_group # rubocop:disable Rails/HasManyOrHasOneDependent
has_many :container_images, -> { distinct }, :through => :containers, :inverse_of => :container_group
belongs_to :ext_management_system, :foreign_key => "ems_id"
has_many :labels, -> { where(:section => "labels") }, # rubocop:disable Rails/HasManyOrHasOneDependent
:class_name => "CustomAttribute",
Expand All @@ -26,6 +27,7 @@ class ContainerGroup < ApplicationRecord
:as => :resource,
:inverse_of => :resource
has_many :container_conditions, :class_name => "ContainerCondition", :as => :container_entity, :dependent => :destroy
has_one :ready_condition, -> { where(:name => "Ready") }, :class_name => "ContainerCondition", :as => :container_entity, :inverse_of => :container_entity # rubocop:disable Rails/HasManyOrHasOneDependent
belongs_to :container_node
has_and_belongs_to_many :container_services, :join_table => :container_groups_container_services
belongs_to :container_replicator
Expand All @@ -41,29 +43,13 @@ class ContainerGroup < ApplicationRecord
has_many :vim_performance_states, :as => :resource
delegate :my_zone, :to => :ext_management_system, :allow_nil => true

virtual_column :ready_condition_status, :type => :string, :uses => :container_conditions
virtual_column :running_containers_summary, :type => :string
virtual_delegate :status, :prefix => true, :to => :ready_condition, :allow_nil => true, :default => "None", :type => :string
virtual_attribute :running_containers_summary, :integer, :arel => (lambda do |t|
t.grouping(Arel::Nodes::NamedFunction.new('CONCAT', [t[:total_running_containers], Arel.sql("'/'"), t[:total_containers]]))
end)

def ready_condition
if container_conditions.loaded?
container_conditions.detect { |condition| condition.name == "Ready" }
else
container_conditions.find_by(:name => "Ready")
end
end

def ready_condition_status
ready_condition.try(:status) || 'None'
end

def container_states_summary
containers.group(:state).count.symbolize_keys
end

def running_containers_summary
summary = container_states_summary
"#{summary[:running] || 0}/#{summary.values.sum}"
end
virtual_total :total_containers, :containers
virtual_total :total_running_containers, :running_containers

# validates :restart_policy, :inclusion => { :in => %w(always onFailure never) }
# validates :dns_policy, :inclusion => { :in => %w(ClusterFirst Default) }
Expand Down
6 changes: 1 addition & 5 deletions app/models/container_image.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class ContainerImage < ApplicationRecord
serialize :exposed_ports, Hash
serialize :environment_variables, Hash

virtual_column :display_registry, :type => :string
virtual_delegate :display_registry, :to => "container_image_registry.full_name", :allow_nil => true, :type => :string
Copy link
Member

Choose a reason for hiding this comment

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

What happens when this returns nil? Is there a presentation-side change where it will say "Unknown image source", or does it leave it blank?

virtual_total :total_containers, :containers

after_create :raise_creation_event
Expand Down Expand Up @@ -84,10 +84,6 @@ def docker_id
# The guid is required by the smart analysis infrastructure
alias_method :guid, :docker_id

def display_registry
container_image_registry.present? ? container_image_registry.full_name : _("Unknown image source")
end

def scan(userid = "system")
ext_management_system.scan_job_create(self, userid)
end
Expand Down
13 changes: 11 additions & 2 deletions app/models/container_image_registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,18 @@ class ContainerImageRegistry < ApplicationRecord
has_many :service_container_groups, :through => :container_services, :as => :container_groups

acts_as_miq_taggable
virtual_column :full_name, :type => :string
virtual_attribute :full_name, :string, :arel => (lambda do |t|
t.grouping(Arel::Nodes::Case.new
.when(t[:port].eq(nil)).then(t[:host])
.when(t[:port].eq("")).then(t[:host])
.else(Arel::Nodes::NamedFunction.new('CONCAT', [t[:host], Arel.sql("':'"), t[:port]])))
end)

def full_name
port.present? ? "#{host}:#{port}" : host
if has_attribute?("full_name")
self["full_name"]
else
port.present? ? "#{host}:#{port}" : host
end
end
end
7 changes: 1 addition & 6 deletions app/models/container_node.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,10 @@ class ContainerNode < ApplicationRecord
has_many :miq_alert_statuses, :as => :resource
delegate :my_zone, :to => :ext_management_system, :allow_nil => true


virtual_column :ready_condition_status, :type => :string, :uses => :container_conditions
virtual_delegate :status, :prefix => true, :to => :ready_condition, :allow_nil => true, :default => "None", :type => :string
virtual_delegate :system_distribution, :to => "operating_system.distribution", :allow_nil => true, :type => :string
virtual_delegate :kernel_version, :to => :operating_system, :allow_nil => true, :type => :string

def ready_condition_status
ready_condition.try(:status) || 'None'
end

include EventMixin
include Metric::CiMixin
include CustomAttributeMixin
Expand Down
20 changes: 11 additions & 9 deletions spec/models/container_group_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,25 @@
end

context "#ready_condition_status" do
let(:condition_ready) { FactoryBot.create(:container_condition, :container_entity => container_group, :name => "Ready") }
let(:condition_ready) { FactoryBot.create(:container_condition, :container_entity => container_group, :name => "Ready", :status => "Good") }
let(:condition_other) { FactoryBot.create(:container_condition, :container_entity => container_group, :name => "Other") }
let(:container_group) { FactoryBot.create(:container_group) }

it "preloads the conditions" do
it "handles no container_conditions (select and direct)" do
condition_other
cr = condition_ready
cg = ContainerGroup.includes(:ready_condition_status).references(:ready_condition_status).find_by(:id => container_group.id)

expect { expect(cg.ready_condition).to eq(cr) }.to_not make_database_queries
subj = described_class.where(:id => container_group.id)
expect(subj.first.ready_condition_status).to eq("None")
expect(subj.select(:ready_condition_status).first.ready_condition_status).to eq("None")
end

it "handles non-preloaded conditions" do
it "selects ready_condition_status" do
condition_ready
condition_other
cr = condition_ready
cg = ContainerGroup.find_by(:id => container_group.id)
expect { expect(cg.ready_condition).to eq(cr) }.to make_database_queries(:count => 1)

subj = described_class.where(:id => container_group.id)
expect(subj.first.ready_condition_status).to eq(condition_ready.status)
expect(subj.select(:ready_condition_status).first.ready_condition_status).to eq(condition_ready.status)
end
end

Expand Down
20 changes: 15 additions & 5 deletions spec/models/container_image_registry_spec.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,19 @@
RSpec.describe ContainerImageRegistry do
it "#full_name" do
reg = ContainerImageRegistry.new(:name => "docker.io", :host => "docker.io")
expect(reg.full_name).to eq("docker.io")
describe "#full_name" do
it "works with no port" do
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a spec where the port is "" since there's a case for that?

reg = FactoryBot.create(:container_image_registry, :name => "docker.io", :host => "docker.io")
expect(reg.full_name).to eq("docker.io")

reg.port = "1234"
expect(reg.full_name).to eq("docker.io:1234")
reg = ContainerImageRegistry.where(:id => reg.id).select(:full_name).first
expect(reg.full_name).to eq("docker.io")
end

it "works with port" do
reg = FactoryBot.create(:container_image_registry, :name => "docker.io", :host => "docker.io", :port => "1234")
expect(reg.full_name).to eq("docker.io:1234")

reg = ContainerImageRegistry.where(:id => reg.id).select(:full_name).first
expect(reg.full_name).to eq("docker.io:1234")
end
end
end
26 changes: 14 additions & 12 deletions spec/models/container_image_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,25 @@

context "#display_registry" do
it "finds unknown with unknown name" do
image = ContainerImage.new(:name => "fedora")
expect(image.display_registry).to eq("Unknown image source")
end
image = FactoryBot.create(:container_image, :name => "fedora")
expect(image.display_registry).to be_nil

it "localizes for unknown" do
I18n.with_locale(:es) do
image = ContainerImage.new(:name => "fedora")
reg = image.display_registry
expect(reg).to eq("Fuente de imagen desconocida")
end
image = ContainerImage.where(:id => image.id).select(:display_registry).first
expect do
expect(image.display_registry).to be_nil
end.not_to make_database_queries
end

it "finds name with valid name" do
image = ContainerImage.new(:name => "fedora")
reg = ContainerImageRegistry.new(:name => "docker.io", :host => "docker.io", :port => "1234")
image.container_image_registry = reg
reg = FactoryBot.create(:container_image_registry, :name => "docker.io", :host => "docker.io", :port => "1234")
image = FactoryBot.create(:container_image, :name => "fedora", :container_image_registry => reg)

expect(image.display_registry).to eq("docker.io:1234")

image = ContainerImage.where(:id => image.id).select(:display_registry).first
expect do
expect(image.display_registry).to eq("docker.io:1234")
end.not_to make_database_queries
end
end

Expand Down
5 changes: 4 additions & 1 deletion spec/models/container_node_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,13 @@
expect(subject.ready_condition_status).to eq("None")
end

it "finds container conditions" do
it "finds ready container conditions" do
FactoryBot.create(:container_conditions, :name => "Ready", :container_entity => subject, :status => "Great")
FactoryBot.create(:container_conditions, :name => "Other", :container_entity => subject, :status => "Not Good")
expect(subject.ready_condition_status).to eq("Great")

subj = described_class.where(:id => subject.id)
expect(subj.select(:ready_condition_status).first.ready_condition_status).to eq("Great")
end
end

Expand Down
Loading