Collect node custom attributes from hawkular during refresh #12924

zgalor · 2016-11-30T14:03:19Z

Reading metrics from hawkular tagged with 'miq_metric=true' and saving them as custom attributes in the node they were collected from. This is done in ems refresh process for kubernetes and openshift.

cc @simon3z
@miq-bot add_label providers/containers

zgalor · 2016-11-30T14:09:38Z

@yaacov PTAL

simon3z · 2016-11-30T15:30:23Z

@moolitayer @zeari @cben can you review?

simon3z · 2016-11-30T15:32:45Z

@zgalor I would expected this to be part of providers/kubernetes/container_manager/refresh_parser.rb and not metrics collection.

What client you use to gather information shouldn't affect where you do the collection.

moolitayer · 2016-11-30T16:20:40Z

@simon3z There is some sense in doing this collection under the new provider. That way we would not interfere with upstream customers out of the box.

simon3z · 2016-12-01T09:09:05Z

@moolitayer there are few things that we want to do in the hawkular provider but this one seems to really belong to inventory refresh on kubernetes/openshift.

@miq-bot assign zgalor

zgalor · 2016-12-06T12:32:30Z

@miq-bot add_label wip

miq-bot · 2016-12-06T22:56:43Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

zgalor · 2016-12-14T14:16:47Z

app/models/manageiq/providers/kubernetes/container_manager/refresher_mixin.rb

@@ -21,6 +21,18 @@ def fetch_entities(client, entities)
            end
          end
        end
+
+        def fetch_ems_metrics(ems)
+          hawk = ManageIQ::Providers::Kubernetes::ContainerManager::MetricsCapture::HawkularClient.new(ems)


@yaacov here I am creating a new HawkularClient for the ems, even though the ems already has a hawkular client for metrics. This is done once in every refresh for each provider.
Is that a good practice? Do you see a better way to do it?

zgalor · 2016-12-14T14:24:24Z

I removed the previous commit - collecting string metrics as part of metric collection and re-implemented as part of refresh.
I also renamed the PR and updated the comment.
It's still a wip because I need to add specs, but I'd like to get some reviews.

@moolitayer @enoodle @cben @yaacov

enoodle · 2016-12-14T14:28:34Z

app/models/ems_refresh/save_inventory_container.rb

@@ -395,6 +395,19 @@ def save_container_volumes_inventory(container_group, hashes, target = nil)
    store_ids_for_new_records(container_group.container_volumes, hashes, :name)
  end

+  def save_custom_attributes_inventory(entity, hashes, target = nil)


maybe we can change save_custom_attribute_attribute_inventory to use this function

I suggest you refactor this in a different PR

Yes. I will rephrase: What a cool function! Lets rewrite save_custom_attribute_attribute_inventory with it in another PR!

enoodle · 2016-12-14T14:34:05Z

app/models/manageiq/providers/kubernetes/container_manager/refresher_mixin.rb

+          keys = hawk.strings.query(:miq_metric => true)
+
+          metrics = {}
+          keys.each do |k|


maybe you can use each_with_objecct here? instead of defining the metrcis hash and then returning it:

keys.each_with_object({|) do |k, metrics| ..... end

enoodle · 2016-12-14T14:34:17Z

app/models/manageiq/providers/openshift/container_manager/refresher.rb

@@ -20,6 +20,7 @@ def parse_legacy_inventory(ems)
          fetch_entities(openshift_client, OPENSHIFT_ENTITIES)
        end
        entities = openshift_entities.merge(kube_entities)
+        entities[:metrics] = fetch_ems_metrics(ems)


Is this only collecting metrics for nodes or also for other entities?

currently we are only sending node metrics, but it will fetch every metric tagged with "miq_metric=true" on this tenant

simon3z · 2017-01-03T13:58:12Z

Not really, can these just be lumped under labels with the kubernetes labels?
Not sure. These aren't labels. labels might have a different purpose - like for tagging and grouping.
@simon3z ?

@zgalor you need a naming suggestion for the section of the nodes custom attributes?
Maybe something like nodes_attributes ? or node_extra_attributes.

zgalor · 2017-01-04T12:33:50Z

@zeari @simon3z I used the generic name additional_attributes so the same code can be later used for other entities (much like labels).

I also added a screenshot in the description.
Please make final reviews/approval so this can be merged.

simon3z · 2017-01-04T14:07:35Z

@zeari @simon3z I used the generic name additional_attributes so the same code can be later used for other entities (much like labels).

@zgalor I am not sure additional is descriptive enough. I have the feeling we can find something better... (we won't block on this but it would be great to improve the naming).

zgalor · 2017-01-04T14:22:35Z

@zgalor I am not sure additional is descriptive enough. I have the feeling we can find something better... (we won't block on this but it would be great to improve the naming).

@simon3z I wanted the inventory parsing to be transparent instead of specific to node so it can be used for other entities. Does "dynamic_attributes" sound better?

simon3z · 2017-01-11T14:03:28Z

LGTM 👍 cc @chessbyte
Assigning to @blomquisg for review/merge.

@miq-bot assign blomquisg

miq-bot · 2017-01-16T21:06:53Z

This pull request is not mergeable. Please rebase and repush.

blomquisg · 2017-01-23T20:39:48Z

app/models/ems_refresh/save_inventory_container.rb

@@ -395,6 +396,19 @@ 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)


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:

additional_attributes vs. custom_attributes in the call to save_inventory_multi

this version calls store_ids_for_new_records whereas save_custom_attributes_inventory does not ... and I'm not sure why.

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

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.

@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)

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

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?

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

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.

👍 I like this new change.

@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.

Use Hawkular client to fetch metrics with miq_metric tag and save each metric as additional attribute in the relevant node Use store_path and fetch_path to parse additional attributes Add hawk collected custom attribute test in refresher_spec.rb Change metrics_capture/capture_context_spec.rb and related vcr cassetes to work with new refresh.

miq-bot · 2017-01-25T13:02:14Z

Checked commits zgalor/manageiq@dbc58da~...b5f2f37 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
8 files checked, 0 offenses detected
Everything looks good. 🍪

blomquisg · 2017-01-26T22:07:52Z

This is good @zgalor ... thanks for working with us on it!

miq-bot added the providers/containers label Nov 30, 2016

chessbyte assigned simon3z Nov 30, 2016

chessbyte added the enhancement label Nov 30, 2016

miq-bot assigned zgalor and unassigned simon3z Dec 1, 2016

zgalor changed the title ~~Collect string metrics from hawkular~~ [WIP] Collect string metrics from hawkular Dec 6, 2016

miq-bot added the wip label Dec 6, 2016

miq-bot added the unmergeable label Dec 6, 2016

zgalor force-pushed the string_metric_collection branch 2 times, most recently from 57226ed to 1e50fd9 Compare December 14, 2016 12:16

miq-bot removed the unmergeable label Dec 14, 2016

zgalor force-pushed the string_metric_collection branch 3 times, most recently from 72968d2 to 9d2f188 Compare December 14, 2016 12:40

zgalor closed this Dec 14, 2016

zgalor reopened this Dec 14, 2016

zgalor commented Dec 14, 2016

View reviewed changes

zgalor changed the title ~~[WIP] Collect string metrics from hawkular~~ [WIP] Update node custom attributes during refresh Dec 14, 2016

enoodle reviewed Dec 14, 2016

View reviewed changes

zgalor force-pushed the string_metric_collection branch 3 times, most recently from 3bce917 to f35c0d3 Compare December 15, 2016 10:22

zgalor force-pushed the string_metric_collection branch from e73b103 to 0533c24 Compare January 4, 2017 12:28

zgalor force-pushed the string_metric_collection branch 2 times, most recently from 9bc4f9c to ff1deed Compare January 4, 2017 13:55

miq-bot assigned blomquisg and unassigned zgalor Jan 11, 2017

miq-bot added the unmergeable label Jan 16, 2017

zgalor force-pushed the string_metric_collection branch from ff1deed to 5767c20 Compare January 17, 2017 12:12

miq-bot removed the unmergeable label Jan 17, 2017

zgalor force-pushed the string_metric_collection branch 3 times, most recently from 7959e41 to b8e41ea Compare January 17, 2017 12:51

zgalor closed this Jan 17, 2017

zgalor reopened this Jan 17, 2017

zgalor closed this Jan 18, 2017

zgalor reopened this Jan 18, 2017

blomquisg reviewed Jan 23, 2017

View reviewed changes

zgalor added 2 commits January 25, 2017 14:28

reuse save_custom_attribute_attribute_inventory

b5f2f37

zgalor force-pushed the string_metric_collection branch from b8e41ea to b5f2f37 Compare January 25, 2017 12:56

blomquisg merged commit 50d2941 into ManageIQ:master Jan 26, 2017

blomquisg added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 26, 2017

zgalor mentioned this pull request Jan 29, 2017

[WIP] Adding Elasticsearch provider with DH manager #13320

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect node custom attributes from hawkular during refresh #12924

Collect node custom attributes from hawkular during refresh #12924

zgalor commented Nov 30, 2016 •

edited

Loading

zgalor commented Nov 30, 2016

simon3z commented Nov 30, 2016

simon3z commented Nov 30, 2016

moolitayer commented Nov 30, 2016

simon3z commented Dec 1, 2016

zgalor commented Dec 6, 2016

miq-bot commented Dec 6, 2016

zgalor Dec 14, 2016

zgalor commented Dec 14, 2016

enoodle Dec 14, 2016

zgalor Dec 14, 2016

enoodle Dec 14, 2016

enoodle Dec 14, 2016

zgalor Dec 14, 2016

enoodle Dec 14, 2016

zgalor Dec 14, 2016

simon3z commented Jan 3, 2017

zgalor commented Jan 4, 2017

simon3z commented Jan 4, 2017

zgalor commented Jan 4, 2017 •

edited

Loading

simon3z commented Jan 11, 2017

miq-bot commented Jan 16, 2017

blomquisg Jan 23, 2017

Fryguy Jan 23, 2017

Fryguy Jan 23, 2017

zgalor Jan 23, 2017

zgalor Jan 23, 2017

blomquisg Jan 24, 2017

zgalor Jan 25, 2017

Fryguy Jan 25, 2017

Fryguy Jan 25, 2017

gtanzillo Jan 25, 2017

miq-bot commented Jan 25, 2017

blomquisg commented Jan 26, 2017

Collect node custom attributes from hawkular during refresh #12924

Collect node custom attributes from hawkular during refresh #12924

Conversation

zgalor commented Nov 30, 2016 • edited Loading

zgalor commented Nov 30, 2016

simon3z commented Nov 30, 2016

simon3z commented Nov 30, 2016

moolitayer commented Nov 30, 2016

simon3z commented Dec 1, 2016

zgalor commented Dec 6, 2016

miq-bot commented Dec 6, 2016

Choose a reason for hiding this comment

zgalor commented Dec 14, 2016

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

simon3z commented Jan 3, 2017

zgalor commented Jan 4, 2017

simon3z commented Jan 4, 2017

zgalor commented Jan 4, 2017 • edited Loading

simon3z commented Jan 11, 2017

miq-bot commented Jan 16, 2017

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

Choose a reason for hiding this comment

miq-bot commented Jan 25, 2017

blomquisg commented Jan 26, 2017

zgalor commented Nov 30, 2016 •

edited

Loading

zgalor commented Jan 4, 2017 •

edited

Loading