External Logging link for container nodes #13704

enoodle · 2017-01-31T12:22:21Z

Adding the common logging link support for container nodes.

enoodle · 2017-01-31T12:22:45Z

This depends on #13319 (I pulled over the commit from there, will rebase once its merged)

Fryguy · 2017-01-31T17:15:26Z

app/models/container_node.rb

@@ -79,4 +80,19 @@ def ipaddress
  def cockpit_url
    URI::HTTP.build(:host => ipaddress, :port => 9090)
  end
+
+  supports :common_logging do


Not sure I like the phrase "common logging" as that is a Red Hat idiom. cc @blomquisg

I'll answer on #13319

Fryguy · 2017-01-31T17:16:39Z

config/settings.yml

@@ -80,6 +80,8 @@
  :scan_via_host: true
  :use_vim_broker: true
  :use_vim_broker_ems: true
+:container_logging:
+  :common_logging_route: logging-kibana-ops


This feels wrong as a setting in the general settings...feels like it should a per-provider settings or perhaps a special endpoint on each provider.

Ill answer on #13319

enoodle · 2017-02-01T14:16:18Z

I removed the commit from the parent PR so this will only be green after it is merged.

miq-bot · 2017-02-14T14:31:55Z

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

simon3z · 2017-02-14T23:02:23Z

@enoodle can you handle the rebase?

simon3z · 2017-02-24T11:06:50Z

@enoodle remind me, I think there was no real conclusion on what the kibana search should be for nodes. Is it reliable enough in this PR? cc @portante

portante · 2017-02-25T16:49:50Z

@enoodle, this looks like it tries to catch both hostnames and ManageIQ names. Ideally we'd have all logs from a given cluster tagged with a certain value in the "tags" field so that we can query like so to using the shortened hostname, tags:clusterA AND (hostname:${hostname} OR hostname:$(basename ${hostname}) (syntax here using bash for example purposes).

enoodle · 2017-02-28T10:55:00Z

@portante I changed to try the hostname from the kubernetes label first. I also changed to make the query itself easier to read.
About the tag: currently it is not available to me, so my only suggestion is that we can add it later when it is more commonly available.

portante · 2017-03-01T01:24:12Z

app/models/container_node.rb

+  def external_logging_path
+    hostname = (ipaddress || name || "").split('.').first
+    query = "bool:(filter:(or:!((term:(hostname:#{hostname})),(term:(hostname:#{name})))))"
+    "/#/discover?_g=()&_a=(columns:!(_source),filters:!((meta:(disabled:!f,index:'.operations.*',key:hostname,negate:!f),#{query})),index:'.operations.*',interval:auto,query:(query_string:(analyze_wildcard:!t,query:'*')),sort:!(time,desc))"


Can you factor out the .operations.* index name as a variable? It appears in two places in the string. Also, I tried to use this URL with a few adjustments for names in my Kibana instance, but I get an error. Are you sure this is right?

Additionally, could we add some fields by default so that it is not just the _source field, which is large blob? Perhaps adding hostname, level, pod_name, and message would be sufficient initially.

I am not sure that this URL is right, but it is working for me (I have added a gif, made it for the UI Patch but it fits here too).
To get this URL I opened Kibana and told it to filter by hostname. I then changed the query to catch the both fqdn and hostname and trimmed it a little. This is where the _source came from. To my understanding after playing with this a little this changes the columns that are displayed next to each message in the list. I will set this to the columns you suggested.
The field level is empty for me on all messages but I assume that future improvments with the data model will change that.

portante · 2017-03-01T01:25:09Z

app/models/container_node.rb

+
+  supports :external_logging_support do
+    unless ext_management_system.respond_to?(:external_logging_route_name)
+      unsupported_reason_add(:external_logging_support, _('This provider type does not support external_logging_support'))


Is this message from the department of the redundancy department? ;)

I will un-robotize this. This message is not for the end user though.

portante · 2017-03-01T13:44:42Z

@enoodle, I'll try to get this URL working for me in my kibana instance and report back.

portante · 2017-03-01T14:30:45Z

I made a simple python script to verify the base string works now. Looks good!

#!/usr/bin/env python

def external_logging_path(index, hostname_l):
    hostname_s = (hostname_l or "*").split('.')[0]
    query = "bool:(filter:(or:!((term:(hostname:'%(hostname_s)s')),(term:(hostname:'%(hostname_l)s')))))" % { 'hostname_l': hostname_l, 'hostname_s': hostname_s }
    return  "#/discover?_g=()&_a=(columns:!(hostname,level,kubernetes.pod_name,message),filters:!((meta:(disabled:!f,index:%(index)s,key:hostname,negate:!f),%(query)s)),index:%(index)s,interval:auto,query:(query_string:(analyze_wildcard:!t,query:'*')),sort:!('@timestamp',desc))" % { 'index': index, 'query': query }

print external_logging_path("[logstash-]YYYY.MM.DD", "perf44.example.com")
print external_logging_path("[logstash-]YYYY.MM.DD", "gprfs003.example.com")
print external_logging_path("[logstash-]YYYY.MM.DD", "172.17.40.144")

enoodle · 2017-03-01T22:12:12Z

@simon3z I verified this with the latest update to the UI PR. updated the GIF.

simon3z · 2017-03-01T22:20:53Z

app/models/container_node.rb

+
+  def external_logging_path
+    fqdn = (ipaddress || name || "")
+    hostname = fqdn.split('.').first


@enoodle @portante if fqdn is an ipaddress... wouldn't hostname be the first octet?

Also shouldn't you protect against hostname possibly being nil?

If neither ipaddress or name are defined why do we continue with ""? Wouldn't it generate a meaningless query?

@simon3z The name of the method was misleading so I changed it from ipaddress to kubernetes_hostname. If it is indeed returning an IP address instead of an hostname then we will add a search for the first octect (OR the full IP). It will not be optimal but won't crush anything and considering that none of our options gave a valid hostname it seems like an acceptable option.

I fixed hostname to have a default value of ""

simon3z · 2017-03-03T16:13:44Z

app/models/container_node.rb

+  def external_logging_path
+    fqdn = (kubernetes_hostname || name || "")
+    hostname = fqdn.split('.').first || ""
+    query = "bool:(filter:(or:!((term:(hostname:'#{hostname}')),(term:(hostname:'#{fqdn}')))))"


@enoodle I don't understand, why do you still use the hostname here even if it's empty.
I have the feeling we should be smarter on how to build this query.

Implicitly I believe that what you're saying here is:

if fqdn is empty (and therefore hostname is empty), return all logs

if hostname is empty, just use fqdn

use both fqdn and hostname if available

Although I assume that the above relies on Kibana treating an empty string "" as a search term not to consider.

I'd prefer to see all the logic expressed in code rather than relying on Kibana behavior.

@simon3z, unfortunately, the logging subsystem does not return a consistent set of hostnames for the same entity. So some parts of the logging subsystem gather the FQDN, other parts might use the shortened name. I believe this query is trying to account for that. We have to file bugs with the logging subsystem to fix the collectors to ensure an FQDN.

Having said that, data collection is never going to be perfect, so it is likely we'll need to keep this around.

@simon3z I want a query to get all the message with hostname that equals to the fqdn of the node OR the hostname of the node. This is derived from looking at the values of this field in the logs that are currently being collected.
I have tested that if either fqdn or hostname will be empty string "" it will not add results to the search query results.

I have tested that if either fqdn or hostname will be empty string "" it will not add results to the search query results.

@enoodle as mentioned above I think this should be explicit in the logic that builds the query rather than implicit in the empty string handling of kibana.

enoodle · 2017-03-07T10:24:44Z

@simon3z PTAL

simon3z · 2017-03-08T13:50:39Z

app/models/container_node.rb

+    query = "#{query}," if !hostname.blank? && !fqdn.blank?
+    query = "#{query}(term:(hostname:'#{hostname}'))" if !hostname.blank?
+    query = "#{query})))"
+    index = ".operations.*"


@enoodle mmmm... logic is still incomplete because fqdn.split('.').first could generate an hostname that is == fqdn:

fqdn = "node_short_hostname" hostname = fqdn.split('.').first || "" => "node_short_hostname"

Also you check multiple times (at least 4) for !fqdn.blank? and !hostname.blank?

Since this search is term-based couldn't you just do something like:

node_hostnames = [kubernetes_hostname || name] # node name cannot be empty, it's an ID node_hostnames.push(node_hostnames.first.split('.').first) node_hostnames_query = node_hostnames.uniq.map{|x| "(term:(hostname:'#{x}'))"}.join(",") ...

Which for test.example.com would generate a node_hostnames_query = "(term:(hostname:'test.example.com')),(term:(hostname:'test'))"

Also it seems that the URL may deserve a constant.

simon3z · 2017-03-08T14:30:14Z

spec/models/manageiq/providers/kubernetes/container_manager/container_node_spec.rb

+    it "queries only for the name/fqdn when hostname can't be parsed" do
+      node = FactoryGirl.create(:container_node, :name => "hello")
+      query = get_query(node.external_logging_path)
+      expect(query).to eq("bool:(filter:(or:!((term:(hostname:'hello')),(term:(hostname:'hello')))))")


@enoodle shouldn't this fail now?

@simon3z Yes, I am on it. This is not working as intended yet.

enoodle · 2017-03-08T16:05:36Z

@simon3z PTAL

simon3z · 2017-03-08T16:07:58Z

@enoodle please add a test that takes advantage of kubernetes.io/hostname

simon3z

LGTM 👍

@miq-bot assign Fryguy

simon3z · 2017-03-08T22:05:28Z

@miq-bot assign Fryguy

Fryguy · 2017-03-10T21:54:08Z

app/models/container_node.rb

  end

  def evaluate_alert(_alert_id, _event)
    # currently only EmsEvents from hawkular are tested for node alerts,
    # and these should automaticaly be translated to alerts.
    true
  end
+
+  supports :external_logging_support do


This name is funny because it duplicates the word support in certain caller paths. I think you just want supports :external_logging.

Right, do you want me to change this name in this PR? This will involve changing it in "app/models/mixins/support_feature_mixin.rb" and "app/models/manageiq/providers/container_manager.rb" too.

I opt for changing it 👍 But in it's own PR - and check if the UI depends on it

Oh I didn't realize it wasn't introduced in this PR. Yeah, then separate PR for sure.

Fryguy · 2017-03-10T21:54:35Z

app/models/container_node.rb

+
+  supports :external_logging_support do
+    unless ext_management_system.respond_to?(:external_logging_route_name)
+      unsupported_reason_add(:external_logging_support, _('This provider type does not support Extrenal Logging'))


Typo Extrenal -> External

Does it make sense to make this a dynamic supports method? Why not just supports_not :external_logging. @durandom Thoughts here?

@Fryguy This is for the Kubernetes container provider. It doesn't use routes so it can't support this feature.

Fryguy · 2017-03-10T21:57:45Z

spec/models/manageiq/providers/kubernetes/container_manager/container_node_spec.rb

+  describe "#external_logging_path" do
+    def get_query(path)
+      index = ".operations.*"
+      prefix_len = "/#/discover?_g=()&_a=(columns:!(hostname,level,kubernetes.pod_name,message),filters:!((meta:(disabled:!f,index:'#{index}',key:hostname,negate:!f),".length


It feels wrong to duplicate this string in the test. Is there a way to keep it in the model? Perhaps a pair of methods for constructing and desconstructing the query string in the model?

container node ipaddress rename to kubernetes_hostname the previous name, "ipaddress" is confusing and does not reflect well on the return value.

miq-bot · 2017-03-12T15:57:23Z

Checked commit enoodle@c025dcd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🏆

enoodle mentioned this pull request Jan 31, 2017

container provider: add External Logging Support SupportFeature #13319

Merged

chessbyte added enhancement providers/containers labels Jan 31, 2017

chessbyte assigned simon3z Jan 31, 2017

Fryguy reviewed Jan 31, 2017

View reviewed changes

enoodle force-pushed the kibana_link_for_container_nodes branch from 67db9e5 to 0144a88 Compare February 1, 2017 14:14

enoodle closed this Feb 7, 2017

enoodle reopened this Feb 7, 2017

miq-bot added the unmergeable label Feb 14, 2017

enoodle force-pushed the kibana_link_for_container_nodes branch from 0144a88 to 82b5cc3 Compare February 22, 2017 09:33

miq-bot removed the unmergeable label Feb 22, 2017

enoodle force-pushed the kibana_link_for_container_nodes branch 2 times, most recently from 6505533 to fdf76bd Compare February 23, 2017 12:38

enoodle force-pushed the kibana_link_for_container_nodes branch from fdf76bd to d9c40c4 Compare February 26, 2017 11:06

enoodle changed the title ~~Kibana link for container nodes~~ External Logging link for container nodes Feb 27, 2017

enoodle force-pushed the kibana_link_for_container_nodes branch from d9c40c4 to a2dc0bc Compare February 28, 2017 10:42

portante suggested changes Mar 1, 2017

View reviewed changes

enoodle force-pushed the kibana_link_for_container_nodes branch 2 times, most recently from ff3c930 to d4299b9 Compare March 1, 2017 10:11

enoodle force-pushed the kibana_link_for_container_nodes branch from d4299b9 to f80a208 Compare March 1, 2017 14:26

simon3z reviewed Mar 1, 2017

View reviewed changes

enoodle force-pushed the kibana_link_for_container_nodes branch from 06a2dcc to 2f2e527 Compare March 2, 2017 13:00

simon3z reviewed Mar 3, 2017

View reviewed changes

enoodle force-pushed the kibana_link_for_container_nodes branch from 2f2e527 to cda3b14 Compare March 7, 2017 10:11

simon3z reviewed Mar 8, 2017

View reviewed changes

enoodle force-pushed the kibana_link_for_container_nodes branch from cda3b14 to 9a08f62 Compare March 8, 2017 14:22

simon3z reviewed Mar 8, 2017

View reviewed changes

enoodle force-pushed the kibana_link_for_container_nodes branch from 9a08f62 to fccc487 Compare March 8, 2017 16:05

enoodle force-pushed the kibana_link_for_container_nodes branch from fccc487 to 9fcafb0 Compare March 8, 2017 16:15

simon3z approved these changes Mar 8, 2017

View reviewed changes

miq-bot assigned Fryguy and unassigned simon3z Mar 8, 2017

Fryguy reviewed Mar 10, 2017

View reviewed changes

enoodle force-pushed the kibana_link_for_container_nodes branch 2 times, most recently from 9729627 to a3bc353 Compare March 12, 2017 10:04

adding common logging link support for container nodes

c025dcd

container node ipaddress rename to kubernetes_hostname the previous name, "ipaddress" is confusing and does not reflect well on the return value.

enoodle force-pushed the kibana_link_for_container_nodes branch from a3bc353 to c025dcd Compare March 12, 2017 15:54

Fryguy merged commit 1b5655f into ManageIQ:master Mar 13, 2017

Fryguy added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 13, 2017

This was referenced Mar 13, 2017

omit 'support' from external_logging feature name #14298

Merged

renaming external_logging feature ManageIQ/manageiq-ui-classic#669

Merged

enoodle mentioned this pull request Apr 12, 2017

fix cockpit button ManageIQ/manageiq-ui-classic#1021

Merged

External Logging link for container nodes #13704

External Logging link for container nodes #13704

Conversation

enoodle commented Jan 31, 2017 • edited Loading

enoodle commented Jan 31, 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

enoodle commented Feb 1, 2017

miq-bot commented Feb 14, 2017

simon3z commented Feb 14, 2017

simon3z commented Feb 24, 2017

portante commented Feb 25, 2017

enoodle commented Feb 28, 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

portante commented Mar 1, 2017

portante commented Mar 1, 2017

enoodle commented Mar 1, 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

enoodle commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Mar 8, 2017

simon3z commented Mar 8, 2017

simon3z left a comment

Choose a reason for hiding this comment

simon3z commented Mar 8, 2017

Fryguy Mar 10, 2017 • edited Loading

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

Fryguy Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Mar 12, 2017

enoodle commented Jan 31, 2017 •

edited

Loading

Fryguy Mar 10, 2017 •

edited

Loading

Fryguy Mar 10, 2017 •

edited

Loading