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

Collect node custom attributes from hawkular during refresh #12924

Merged
merged 2 commits into from
Jan 26, 2017
Merged

Collect node custom attributes from hawkular during refresh #12924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbc58da
Collect node custom attributes from hawkular during refresh
zgalor Dec 14, 2016
b5f2f37
reuse save_custom_attribute_attribute_inventory
zgalor Jan 25, 2017
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
1 change: 1 addition & 0 deletions app/models/container_node.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class ContainerNode < ApplicationRecord
has_many :container_routes, -> { distinct }, :through => :container_services
has_many :container_replicators, -> { distinct }, :through => :container_groups
has_many :labels, -> { where(:section => "labels") }, :class_name => "CustomAttribute", :as => :resource, :dependent => :destroy
has_many :additional_attributes, -> { where(:section => "additional_attributes") }, :class_name => "CustomAttribute", :as => :resource, :dependent => :destroy
has_one :computer_system, :as => :managed_entity, :dependent => :destroy
belongs_to :lives_on, :polymorphic => true
has_one :hardware, :through => :computer_system
Expand Down
7 changes: 6 additions & 1 deletion app/models/ems_refresh/save_inventory_container.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ def save_container_nodes_inventory(ems, hashes, target = nil)
end

save_inventory_multi(ems.container_nodes, hashes, deletes, [:ems_ref],
[:labels, :tags, :computer_system, :container_conditions], [:namespace])
[:labels, :tags, :computer_system, :container_conditions,
:additional_attributes], [:namespace])
store_ids_for_new_records(ems.container_nodes, hashes, :ems_ref)
end

Expand Down Expand Up @@ -395,6 +396,10 @@ def save_container_volumes_inventory(container_group, hashes, target = nil)
store_ids_for_new_records(container_group.container_volumes, hashes, :name)
end

def save_additional_attributes_inventory(entity, hashes, target = nil)
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see this merge with the save_custom_attributes_inventory method since additional_attributes is really just an alias for custom_attributes. But, this has two major differences:

  1. additional_attributes vs. custom_attributes in the call to save_inventory_multi
  2. this version calls store_ids_for_new_records whereas save_custom_attributes_inventory does not ... and I'm not sure why.

Copy link
Member

Choose a reason for hiding this comment

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

I agree...the model should just be has_many :custom_attributes instead of trying to create some special naming. If store_ids_for_new_records is needed, there's no reason it can't be added to the existing save_custom_attributes_inventory

Copy link
Member

Choose a reason for hiding this comment

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

On that note though...is custom_attributes the right place for this or should this be in advanced_settings? It depends on the what is going to be store here in the refresh.

The difference is that custom_attributes are things that people manipulate and change, particularly through manageiq. advanced_settings are extended metadata that is mostly for informational purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fryguy this is data that we collect daily from the openshift cluster such as rpm version of openshift in each node, and then create a policy that verifies the attributed against the expected version. Could also be certificate fingerprint. Not intended to be manipulated through manageIQ, but to be pushed and changed externally.

@blomquisg I initially created these as regular custom attributes, but as @zeari noted custom_attributes are very generic, and we might want to separate those specific attributes (like we do for labels\selectors\annotations)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fryguy will advanced_settings show on the Node/Provider page? Will I be able to use them in conditions?

Copy link
Member

Choose a reason for hiding this comment

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

Hrm, looking at the two tables, I think @Fryguy is backwards on this. advanced_settings has default_value, min, max, and read_only attributes. Which to me looks like something that might be used to build a UI for manipulating in ManageIQ.

Whereas custom_attributes has just section, name, value, and description. Which, to me means it's data we simply collect and display.

So, it looks like custom_attributes is where this should stay.

However, I still think it would be better to reuse the custom_attributes directly (so you could reuse the existing save_custom_attributes_inventory method, maybe with a the small change of adding store_ids_for_new_records to the current implementation). Then, you could always group these Hawkular based settings with the section attribute of custom_attributes. And, if needed, you could have a special method called additional_attributes that grabs these Hawkular settings.

That seems better than trying to circumvent the current inventory just to have a collection attribute new name on container_node.

@zeari, @zgalor, @Fryguy thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the code to reuse existing save_custom_attribute_attribute_inventory.
I kept the additional_attributes alias same way we do for labels, for easier access and management and to differentiate those custom_attributes from others.

@blomquisg PTAL

Copy link
Member

Choose a reason for hiding this comment

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

advanced_settings has default_value, min, max, and read_only attributes.

that's because it was built from vmware which gave us those values. advanced_settings there were modifiable, but not common to modify and definitely not modified by us. custom_attributes was closer to like amazon tags or openshift labels, where it was quite common to change/add/delete them. So if this is what these hawkular settings are, then it belongs in custom_attributes.

Copy link
Member

Choose a reason for hiding this comment

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

👍 I like this new change.

Copy link
Member

Choose a reason for hiding this comment

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

@Fryguy @zgalor Custom attributes are available in reporting and expressions but, currently, advanced settings are not. Well, they are but not in the way custom attributes are where the CustomAttribute#name appears as a reportable column. It's on our todo list, though.

save_custom_attribute_attribute_inventory(entity, :additional_attributes, hashes, target)
end

def save_custom_attribute_attribute_inventory(entity, attribute_name, hashes, target = nil)
return if hashes.nil?

Expand Down
1 change: 1 addition & 0 deletions ...models/manageiq/providers/kubernetes/container_manager/metrics_capture/hawkular_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ def initialize(ext_management_system, tenant = '_system')
end

delegate :gauges, :to => :hawkular_client
delegate :strings, :to => :hawkular_client
end
28 changes: 28 additions & 0 deletions app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ def initialize
end

def ems_inv_to_hashes(inventory)
get_additional_attributes(inventory)
get_nodes(inventory)
get_namespaces(inventory)
get_resource_quotas(inventory)
Expand Down Expand Up @@ -125,6 +126,19 @@ def get_component_statuses(inventory)
end
end

def get_additional_attributes(inventory)
inventory["additional_attributes"] ||= {}
process_collection(inventory["additional_attributes"], :additional_attributes) do |aa|
parse_additional_attribute(aa)
end

@data[:additional_attributes].each do |aa|
ats = @data_index.fetch_path(:additional_attributes, :by_node, aa[:node]) || []
ats << {:name => aa[:name], :value => aa[:value], :section => "additional_attributes"}
@data_index.store_path(:additional_attributes, :by_node, aa[:node], ats)
end
end

def process_collection(collection, key, &block)
@data[key] ||= []
collection.each { |item| process_collection_item(item, key, &block) }
Expand Down Expand Up @@ -189,6 +203,18 @@ def cross_link_node(new_result)
new_result[:lives_on_type] = host_instance.try(:type)
end

def parse_additional_attribute(attribute)
# Assuming keys are in format "node/<hostname.example.com/key"
if attribute[0] && attribute[0].split("/").count == 3
{ attribute[0].split("/").first.to_sym => attribute[0].split("/").second,
:name => attribute[0].split("/").last,
:value => attribute[1],
:section => "additional_attributes"}
else
{}
end
end

def parse_node(node)
new_result = parse_base_item(node)

Expand Down Expand Up @@ -234,6 +260,8 @@ def parse_node(node)
new_result[:container_conditions] = parse_conditions(node)
cross_link_node(new_result)

new_result[:additional_attributes] = @data_index.fetch_path(:additional_attributes, :by_node, node.metadata.name)

new_result
end

Expand Down
13 changes: 13 additions & 0 deletions app/models/manageiq/providers/openshift/container_manager/refresher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,18 @@ class ContainerManager::Refresher < ManageIQ::Providers::BaseManager::Refresher
{:name => 'images'}
]

def fetch_hawk_inv(ems)
hawk = ManageIQ::Providers::Kubernetes::ContainerManager::MetricsCapture::HawkularClient.new(ems, '_ops')
keys = hawk.strings.query(:miq_metric => true)
keys.each_with_object({}) do |k, attributes|
values = hawk.strings.get_data(k.json["id"], :limit => 1, :order => "DESC")
attributes[k.json["id"]] = values.first["value"] unless values.empty?
end
rescue => err
_log.error err.message
return nil
end

def parse_legacy_inventory(ems)
kube_entities = ems.with_provider_connection(:service => KUBERNETES_EMS_TYPE) do |kubeclient|
fetch_entities(kubeclient, KUBERNETES_ENTITIES)
Expand All @@ -20,6 +32,7 @@ def parse_legacy_inventory(ems)
fetch_entities(openshift_client, OPENSHIFT_ENTITIES)
end
entities = openshift_entities.merge(kube_entities)
entities["additional_attributes"] = fetch_hawk_inv(ems) || {}
EmsRefresh.log_inv_debug_trace(entities, "inv_hash:")
ManageIQ::Providers::Openshift::ContainerManager::RefreshParser.ems_inv_to_hashes(entities)
end
Expand Down
5 changes: 3 additions & 2 deletions ...s/manageiq/providers/kubernetes/container_manager/metrics_capture/capture_context_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
allow(MiqServer).to receive(:my_zone).and_return("default")
hostname = 'capture.context.com'
token = 'theToken'
@start_time = 1_468_235_843
@end_time = 1_468_235_843

@start_time = 1_482_306_073
@end_time = 1_482_306_073
@interval = 20

@ems = FactoryGirl.create(
Expand Down
202 changes: 197 additions & 5 deletions spec/models/manageiq/providers/kubernetes/container_manager/refresh_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,8 @@
},
:namespace => nil,
:resource_version => '369104',
:type => 'ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode'
:type => 'ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode',
:additional_attributes => nil
})
end

Expand Down Expand Up @@ -823,7 +824,8 @@
},
:namespace => nil,
:resource_version => '3691041',
:type => 'ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode'
:type => 'ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode',
:additional_attributes => nil
})
end

Expand Down Expand Up @@ -866,11 +868,201 @@
:kernel_version => nil
}
},
:namespace => nil,
:resource_version => '369104',
:type => 'ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode'
:namespace => nil,
:resource_version => '369104',
:type => 'ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode',
:additional_attributes => nil
})
end
it "handles node with single custom attribute" do
parser.get_additional_attributes(
"additional_attributes" => { "node/test-node/key" => "val" }
)

expect(
parser.send(
:parse_node,
RecursiveOpenStruct.new(
:metadata => {
:name => 'test-node',
:uid => 'f0c1fe7e-9c09-11e5-bb22-28d2447dcefe',
:resourceVersion => '369104',
:creationTimestamp => '2016-01-01T11:10:21Z'
},
:spec => {
:providerID => 'aws:///zone/aws-id'
},
:status => {
:capacity => {}
}
),
)
).to eq(
:name => 'test-node',
:ems_ref => 'f0c1fe7e-9c09-11e5-bb22-28d2447dcefe',
:ems_created_on => '2016-01-01T11:10:21Z',
:container_conditions => [],
:identity_infra => 'aws:///zone/aws-id',
:labels => [],
:tags => [],
:lives_on_id => nil,
:lives_on_type => nil,
:max_container_groups => nil,
:computer_system => {
:hardware => {
:cpu_total_cores => nil,
:memory_mb => nil
},
:operating_system => {
:distribution => nil,
:kernel_version => nil
}
},
:namespace => nil,
:resource_version => '369104',
:type => 'ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode',
:additional_attributes => [{ :name => "key", :value => "val", :section => "additional_attributes" }]
)
end
it "handles node with multiple custom attributes" do
parser.get_additional_attributes(
"additional_attributes" => { "node/test-node/key1" => "val1",
"node/test-node/key2" => "val2"}
)

expect(
parser.send(
:parse_node,
RecursiveOpenStruct.new(
:metadata => {
:name => 'test-node',
:uid => 'f0c1fe7e-9c09-11e5-bb22-28d2447dcefe',
:resourceVersion => '369104',
:creationTimestamp => '2016-01-01T11:10:21Z'
},
:spec => {
:providerID => 'aws:///zone/aws-id'
},
:status => {
:capacity => {}
}
),
)
).to eq(
:name => 'test-node',
:ems_ref => 'f0c1fe7e-9c09-11e5-bb22-28d2447dcefe',
:ems_created_on => '2016-01-01T11:10:21Z',
:container_conditions => [],
:identity_infra => 'aws:///zone/aws-id',
:labels => [],
:tags => [],
:lives_on_id => nil,
:lives_on_type => nil,
:max_container_groups => nil,
:computer_system => {
:hardware => {
:cpu_total_cores => nil,
:memory_mb => nil
},
:operating_system => {
:distribution => nil,
:kernel_version => nil
}
},
:namespace => nil,
:resource_version => '369104',
:type => 'ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode',
:additional_attributes => [{ :name => "key1", :value => "val1", :section => "additional_attributes" },
{ :name => "key2", :value => "val2", :section => "additional_attributes" }]
)
end
it "ignores custom attributes of a different node" do
parser.get_additional_attributes(
"additional_attributes" => { "node/test-node1/key1" => "val1",
"node/test-node2/key2" => "val2"}
)

expect(
parser.send(
:parse_node,
RecursiveOpenStruct.new(
:metadata => {
:name => 'test-node1',
:uid => 'f0c1fe7e-9c09-11e5-bb22-28d2447dcefe',
:resourceVersion => '369104',
:creationTimestamp => '2016-01-01T11:10:21Z'
},
:spec => {
:providerID => 'aws:///zone/aws-id'
},
:status => {
:capacity => {}
}
),
)
).to eq(
:name => 'test-node1',
:ems_ref => 'f0c1fe7e-9c09-11e5-bb22-28d2447dcefe',
:ems_created_on => '2016-01-01T11:10:21Z',
:container_conditions => [],
:identity_infra => 'aws:///zone/aws-id',
:labels => [],
:tags => [],
:lives_on_id => nil,
:lives_on_type => nil,
:max_container_groups => nil,
:computer_system => {
:hardware => {
:cpu_total_cores => nil,
:memory_mb => nil
},
:operating_system => {
:distribution => nil,
:kernel_version => nil
}
},
:namespace => nil,
:resource_version => '369104',
:type => 'ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode',
:additional_attributes => [{ :name => "key1", :value => "val1", :section => "additional_attributes" }]
)
end
end

describe "parse_additional_attribute" do
it "parses node attribute" do
expect(
parser.send(
:parse_additional_attribute,
%w(node/test-node/key val)
)
).to eq(:node => "test-node", :name => "key", :value => "val", :section => "additional_attributes")
end
it "parses pod attribute" do
expect(
parser.send(
:parse_additional_attribute,
%w(pod/test-pod/key val)
)
).to eq(:pod => "test-pod", :name => "key", :value => "val", :section => "additional_attributes")
end
it "parses empty attribute" do
expect(
parser.send(
:parse_additional_attribute,
[]
)
).to eq({})
end

it "parses wrong format" do
expect(
parser.send(
:parse_additional_attribute,
%w(key1 val1)
)
).to eq({})
end
end

describe "parse_persistent_volume" do
Expand Down
Loading