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

Collect metrics for archived containers #13686

Merged
merged 1 commit into from
Feb 24, 2017
Merged
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
4 changes: 1 addition & 3 deletions app/models/container.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
class Container < ApplicationRecord
include SupportsFeatureMixin
include NewWithTypeStiMixin
include ArchivedMixin

has_one :container_group, :through => :container_definition
belongs_to :ext_management_system, :foreign_key => :ems_id
Expand All @@ -18,9 +19,6 @@ class Container < ApplicationRecord
has_many :metric_rollups, :as => :resource
has_many :vim_performance_states, :as => :resource

# Needed for metrics
delegate :my_zone, :to => :ext_management_system

include EventMixin
Copy link
Contributor

Choose a reason for hiding this comment

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

@zeari are you sure this is not used anywhere else in the metrics capture execution?

Copy link
Author

Choose a reason for hiding this comment

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

@simon3z why? I added my_zone as a method further down in this class.
You mean if it should be a virtual_column or cached somehow?

include Metric::CiMixin

Expand Down
1 change: 1 addition & 0 deletions app/models/container_definition.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
class ContainerDefinition < ApplicationRecord
include ArchivedMixin
# :name, :image, :image_pull_policy, :memory, :cpu
belongs_to :container_group
belongs_to :ext_management_system, :foreign_key => :ems_id
Expand Down
4 changes: 1 addition & 3 deletions app/models/container_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class ContainerGroup < ApplicationRecord
include MiqPolicyMixin
include NewWithTypeStiMixin
include TenantIdentityMixin
include ArchivedMixin

# :name, :uid, :creation_timestamp, :resource_version, :namespace
# :labels, :restart_policy, :dns_policy
Expand Down Expand Up @@ -33,9 +34,6 @@ class ContainerGroup < ApplicationRecord
virtual_column :ready_condition_status, :type => :string, :uses => :container_conditions
virtual_column :running_containers_summary, :type => :string

# Needed for metrics
delegate :my_zone, :to => :ext_management_system

def ready_condition
container_conditions.find_by(:name => "Ready")
end
Expand Down
6 changes: 1 addition & 5 deletions app/models/container_project.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
class ContainerProject < ApplicationRecord
include SupportsFeatureMixin
include CustomAttributeMixin
include ArchivedMixin
belongs_to :ext_management_system, :foreign_key => "ems_id"
has_many :container_groups
has_many :container_routes
Expand All @@ -21,7 +22,6 @@ class ContainerProject < ApplicationRecord
has_many :metrics, :as => :resource
has_many :metric_rollups, :as => :resource
has_many :vim_performance_states, :as => :resource
delegate :my_zone, :to => :ext_management_system

has_many :labels, -> { where(:section => "labels") }, :class_name => "CustomAttribute", :as => :resource, :dependent => :destroy

Expand Down Expand Up @@ -66,8 +66,4 @@ def disconnect_inv
self.deleted_on = Time.now.utc
save
end

def archived?
ems_id.nil?
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def initialize(target, start_time, end_time, interval)
@end_time = end_time
@interval = interval
@tenant = target.try(:container_project).try(:name) || '_system'
@ext_management_system = @target.ext_management_system
@ext_management_system = @target.ext_management_system || @target.try(:old_ext_management_system)
Copy link
Author

Choose a reason for hiding this comment

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

I have the feeling that you need to change also this part of the code here:

@ext_management_system = @target.ext_management_system
Because then gets validated here

def validate_target
raise TargetValidationError, "Validation error: ems not defined" unless @ext_management_system
...
end

@simon3z

@ts_values = Hash.new { |h, k| h[k] = {} }
@metrics = []

Expand Down
4 changes: 3 additions & 1 deletion app/models/metric/ci_mixin/capture.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ def perf_capture_object
def queue_name_for_metrics_collection
ems = if self.kind_of?(ExtManagementSystem)
self
elsif self.respond_to?(:ext_management_system)
elsif respond_to?(:ext_management_system) && ext_management_system.present?
ext_management_system
elsif respond_to?(:old_ext_management_system) && old_ext_management_system.present?
old_ext_management_system
Copy link
Member

Choose a reason for hiding this comment

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

This concerns me as queue_name_for_metrics_collection targets which worker will run the collection. If old_ext_management_system points to a no longer enabled EMS, what will happen? Will we just put stuff on the queue that will never get executed?

Copy link
Author

@zeari zeari Feb 22, 2017

Choose a reason for hiding this comment

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

i dont think we archive those(@simon3z right?) so from what i understand old_ext_management_system would either be nil and get caught by the exception below or be a live ems and fetch metrics correctly

end
raise _("Unsupported type %{name} (id: %{number})") % {:name => self.class.name, :number => id} if ems.nil?
ems.metrics_collector_queue_name
Expand Down
22 changes: 22 additions & 0 deletions app/models/mixins/archived_mixin.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
module ArchivedMixin
extend ActiveSupport::Concern

included do
belongs_to :old_ext_management_system, :foreign_key => :old_ems_id, :class_name => 'ExtManagementSystem'
end

def archived?
ems_id.nil?
end

# Needed for metrics
def my_zone
if ext_management_system.present?
ext_management_system.my_zone
elsif old_ext_management_system.present?
# Archived container entities need to retain their zone for metric collection
# This makes the association more complex and might need a performance fix
old_ext_management_system.my_zone
end
end
end
25 changes: 25 additions & 0 deletions spec/models/metric/ci_mixin/capture_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -183,4 +183,29 @@ def parse_datetime(datetime)
datetime << "Z" if datetime.size == 19
Time.parse(datetime).utc
end

context "handles archived container entities" do
it "get the correct queue name and zone from archived container entities" do
ems = FactoryGirl.create(:ems_openshift, :name => 'OpenShiftProvider')
group = FactoryGirl.create(:container_group, :name => "group", :ext_management_system => ems)
container = FactoryGirl.create(:container,
:name => "container",
:container_group => group,
:ext_management_system => ems)
project = FactoryGirl.create(:container_project,
:name => "project",
:ext_management_system => ems)
container.disconnect_inv
group.disconnect_inv
project.disconnect_inv

expect(container.queue_name_for_metrics_collection).to eq ems.metrics_collector_queue_name
expect(group.queue_name_for_metrics_collection).to eq ems.metrics_collector_queue_name
expect(project.queue_name_for_metrics_collection).to eq ems.metrics_collector_queue_name

expect(container.my_zone).to eq ems.my_zone
expect(group.my_zone).to eq ems.my_zone
expect(project.my_zone).to eq ems.my_zone
end
end
end