Adaptations to have a working refresh for Hawkular's Inventory.next #10

israel-hdez · 2017-04-17T19:44:55Z

Hawkular's inventory is being rewritten to store its inventory as metrics. The hawkular-client-ruby is being modified to make the inventory's change as transparent as possible for users of the ruby client, but this is not fully possible. This PR deals with changes required in MiQ side to work correctly with new's Hawkular inventory.

~~Note: This PR was worked on top of code changes of #6. To view the relevant differences, look only at the last commit.~~ #6 has been merged. Now, it this is rebased from master.

https://bugzilla.redhat.com/show_bug.cgi?id=1446286

pilhuhn · 2017-04-18T08:53:24Z

CC @abonas

abonas · 2017-04-18T10:07:35Z

@israel-hdez thanks for this! is this necessary to rely here on PR #6?

abonas · 2017-04-18T10:18:02Z

cc @moolitayer @simon3z

cfcosta

There are some stuff that I think we need to change related to the mocks, the same as discussed on #6, but besides that it looks 🔥.

cfcosta · 2017-04-18T12:55:59Z

app/models/manageiq/providers/hawkular/inventory/collector/middleware_manager.rb

@@ -45,8 +45,11 @@ def config_data_for_resource(resource_path)
      connection.inventory.get_config_data_for_resource(resource_path)
    end

-    def metrics_for_metric_type(metric_path)
-      connection.inventory.list_metrics_for_metric_type(metric_path)
+    def metrics_for_metric_type(feed, metric_type_id)


I'd call this metrics_for_type, the second metric sounds redundant.

Right now, it sounds redundant but, if needed, we may add metrics_for_resource_type or metrics_for_resource, turning ambiguous a metrics_for_type method. Could metrics_with_type be a better name?

cfcosta · 2017-04-18T13:00:32Z

spec/models/manageiq/providers/hawkular/inventory/parser/middleware_manager_spec.rb

+      # - Feed 1 has two deployments with ids: d1, d2
+      # - Feed 2 has three deployments with ids: d1, d2, d3, d4
+      parser.persister = double("Mocked persister")
+      deployments = []


You can have this as only one array declaration.

cfcosta · 2017-04-18T13:01:17Z

spec/models/manageiq/providers/hawkular/inventory/parser/middleware_manager_spec.rb

+      # - Feed 1 has one server with id: s1
+      # - Feed 2 has two servers ids: s1, s2
+      parser.persister = double("Mocked persister")
+      servers = []


Same here, just one array declaration.

cfcosta · 2017-04-18T13:05:16Z

app/models/manageiq/providers/hawkular/inventory/parser/middleware_manager.rb

+        collection.each do |item|
+          yield item, nil
+
+          path = CGI.unescape(item.model_class.try(:resource_path_for_metrics, item) || item.manager_uuid)


One detail that might be important is that CGI is not exactly related (meaning-wise) to what the PR is doing, and I think URI.unescape or URI.decode would be better. There are some tokens that parse differently on both methods, but I think it is worth a try to make the code less all-over-the-place.

Right. Since hawkular api is web-based, I'll change those CGI usages with URI.

abonas · 2017-04-18T13:06:31Z

@israel-hdez the above are the only changes needed accross all miq repos? no vcr cassettes need to be rerecorded? (tests of this PR pass so I wonder regarding cassettes)

israel-hdez · 2017-04-18T13:37:18Z

@abonas

is this necessary to rely here on PR #6?

It depends on which PR gets merged first. If done separately, both PRs will have conflicting changes. I'm assuming that #6 will get merged first.

no vcr cassettes need to be rerecorded?

Yes! VCR cassettes need to be recorded again. Tests are passing, because travis is using the latest released hawkular-client gem, which works against current inventory. I will need some help here to know what was the setup used when VCR cassettes were first recorded, to recreate it and rerecord everything.

simon3z · 2017-04-18T21:17:40Z

@abonas the official hawkular-client gem version bump will happen on manageiq-gems-pending.gemspec, right?

israel-hdez · 2017-04-18T23:30:52Z

@simon3z @abonas Is hawkular-client gem being used in another repo? Else, we may want to remove it from manageiq-gems-pending.gemspec and add it to manageiq-providers-hawkular.gemspec

abonas · 2017-04-19T03:44:45Z

@simon3z @abonas Is hawkular-client gem being used in another repo? Else, we may want to remove it from manageiq-gems-pending.gemspec and add it to manageiq-providers-hawkular.gemspec

@israel-hdez yes, it is used also by the containers provider which code currently resides in the main manageiq repo.
And regarding gem location - I also recently raised the topic of the gem declaration location - that it should not be in the gems pending gemfile). I 100% agree it's better to move it where you suggested (in a separate PR). @Fryguy @durandom ack?

Fryguy · 2017-04-19T15:16:06Z

@abonas Yes. The goal of manageiq-gems-pending is to not exist. Everything in there should be ripped out into it's own repo or, if provider specific, into the provider repo.

durandom · 2017-04-21T13:47:45Z

it is used also by the containers provider which code currently resides in the main manageiq repo.

I suggest both, openshift, kubernetes and hawkular add it to their dependency in .gemspec then?!

moolitayer · 2017-04-23T10:11:54Z

I expect the gem to be used in:

manageiq-providers-kubernetes
manageiq-providers-openshift
manageiq-providers-hawkular
manageiq-providers-datawarehouse (does not exist but I understood it might exist in the future)

I suggest both, openshift, kubernetes and hawkular add it to their dependency in .gemspec then?!

if one repo bumps a version in a way that is not compatible with another (both cannot be satisfied)
wont we know about it in the main mangeiq repo only after the bump PR merge?

miq-bot · 2017-04-24T09:33:31Z

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

durandom · 2017-04-24T09:38:46Z

if one repo bumps a version in a way that is not compatible with another (both cannot be satisfied)
wont we know about it in the main mangeiq repo only after the bump PR merge?

yes, it's the same with any other gem dependency

…rics - Messaging types are now retrieved from its "type path" because the resources do not anymore include its resource-types. - When collecting availability metrics, the "paths" are unescaped rather than escaped for string comparisons. This is because the paths received from hawkular are percent-encoded with arbitrary case and string comparisons were failing because some paths are using uppercase while others are using lowercase (e.g. '%2f' rather than '%2F').

jkremser · 2017-04-26T12:46:12Z

@israel-hdez ~~I can't run the refresh successfully.~~
~~I had to change the~~
/app/models/manageiq/providers/hawkular/inventory/parser/middleware_manager.rb:257 to use the nil? instead of .empty? because nil.empty? was causing exceptions and
in /app/models/manageiq/providers/hawkular/inventory/collector/middleware_manager.rb:52
there is still a call to method that has to be removed. Do you have any newer version that works?

EDIT: ignore my comment ^ I was using old version of hawkular-client gem :)

Disabling expectations testing the underlying container because they require a more complex setup to work correctly. Will be re-enabled in a follow-up commit. Cassettes were recorded using the domain-mode example configuration for WildFly 10.1.0.Final docker image.

miq-bot · 2017-04-26T14:23:37Z

Checked commits israel-hdez/manageiq-providers-hawkular@bed7634~...969f964 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. ⭐

israel-hdez · 2017-04-26T15:16:39Z

This is working fine with v2.9.0 gem. So, I will un-wip it and let to you the decision to merge before or at the same time of ManageIQ/manageiq-gems-pending#138 is merged. It should not be merged "after" gems-pending PR because the refresh won't work. (Although, if merged "before", travis will stay red until the gems-pending PR is merged).

@cfcosta You may wand to do a quick re-review in the meanwhile.
@miq-bot remove_label wip

israel-hdez · 2017-04-26T16:25:23Z

@miq-bot assign abonas

israel-hdez · 2017-04-26T16:28:33Z

spec/models/manageiq/providers/hawkular/middleware_manager/refresher_spec.rb

@@ -30,19 +30,20 @@

    # check whether the server was associated with the vm
    server = @ems_hawkular.middleware_servers.first
-    expect(server.lives_on_id).to eql(@vm.id)
-    expect(server.lives_on_type).to eql(@vm.type)
+    # TODO: Restore these expectations


@Jiri-Kremser I will appreciate if you can instruct me how you did your setup for these expectations, so I can bring them back to life in a follow-up PR.

They should appear if the hawkular-services are deployed on a virtual machine managed by RHEV provider and this provider is also added to MiQ. The cross-linking should just happen automatically. If I recall correctly the https://hawkular.torii.gva.redhat.com is running on RHEV and both the hawkular and RHEV can be added as providers to MiQ (juca can give you the access). Although, it will probably not work for the containerized hawular-services, because we no longer have the

sudo echo $HOSTNAME > /etc/machine-id

..in the start script, because the docker image must run without the root on openshift.

@jpkrohling As the author, could you pls check that I am not lying here :] Also, do you have any idea how to workaround the ^ for containers? Perhaps we can change the permissions of /etc/machine-id so that it's writable by non-root guys, right? That would work. If you are not against it I am willing to add it back.

They should appear if the hawkular-services are deployed on a virtual machine managed by RHEV provider and this provider is also added to MiQ.

That's correct.

The cross-linking should just happen automatically.

Depending on the definition of automatically, that's also correct :) @abonas can confirm how it works on the UI, but on this side, all we need to do is to set the lives_on field. Something else will compare these values and show that accordingly on the UI.

If I recall correctly the https://hawkular.torii.gva.redhat.com is running on RHEV and both the hawkular and RHEV can be added as providers to MiQ (juca can give you the access).

Also correct.

Also, do you have any idea how to workaround the ^ for containers? Perhaps we can change the permissions of /etc/machine-id so that it's writable by non-root guys, right? That would work. If you are not against it I am willing to add it back.

I have no idea about that, though, as it probably depends on the outcome of the discussions about the machine ID for containers. In my opinion, /etc/machine-id should not contain the hostname, or anything that doesn't conform with the spec that rules this file: https://www.freedesktop.org/software/systemd/man/machine-id.html

@Jiri-Kremser @jpkrohling

If I recall correctly the https://hawkular.torii.gva.redhat.com is running on RHEV

It outputs a "bad gateway" error to me :(

israel-hdez · 2017-04-27T14:54:57Z

@miq-bot add_label fine/yes, blocker

Please see ManageIQ/manageiq-gems-pending#138 (comment) for backport notes.

abonas · 2017-04-27T15:55:21Z

clicking on code climate shows green status, so I'll merge

israel-hdez · 2017-04-27T17:40:26Z

If this cannot be backported seamlessly to fine, please merge ManageIQ/manageiq#14927.

simaishi · 2017-04-27T20:23:23Z

Backported to Fine via ManageIQ/manageiq#14927

Enable rubocop in CI for pull requests

miq-bot added the wip label Apr 17, 2017

abonas requested a review from cfcosta April 18, 2017 10:17

cfcosta reviewed Apr 18, 2017

View reviewed changes

abonas added the enhancement label Apr 19, 2017

miq-bot added the unmergeable label Apr 24, 2017

israel-hdez force-pushed the inventory-on-metrics branch from 3668d3b to fdff37b Compare April 24, 2017 19:53

miq-bot removed the unmergeable label Apr 24, 2017

israel-hdez added 2 commits April 26, 2017 09:22

Re-recording vcr cassettes related to metric collection specs

a15c0ea

israel-hdez force-pushed the inventory-on-metrics branch from 344a119 to 969f964 Compare April 26, 2017 14:23

miq-bot changed the title ~~[WIP] Adaptations to have a working refresh for Hawkular's Inventory.next~~ Adaptations to have a working refresh for Hawkular's Inventory.next Apr 26, 2017

miq-bot removed the wip label Apr 26, 2017

miq-bot assigned abonas Apr 26, 2017

israel-hdez commented Apr 26, 2017

View reviewed changes

abonas added the inventory label Apr 27, 2017

israel-hdez mentioned this pull request Apr 27, 2017

Bump version of hawkular-client gem to 3.0.1 ManageIQ/manageiq-gems-pending#138

Merged

miq-bot added blocker fine/yes labels Apr 27, 2017

israel-hdez closed this Apr 27, 2017

israel-hdez reopened this Apr 27, 2017

abonas merged commit 9903c18 into ManageIQ:master Apr 27, 2017

israel-hdez mentioned this pull request Apr 27, 2017

[FINE] Adaptations to have a working refresh for Hawkular's Inventory.v3 ManageIQ/manageiq#14927

Merged

simaishi added fine/backported and removed fine/yes labels Apr 27, 2017

israel-hdez deleted the inventory-on-metrics branch May 4, 2017 18:01

abonas added this to the Sprint 60 Ending May 8, 2017 milestone May 8, 2017

josejulio added a commit to josejulio/manageiq-providers-hawkular that referenced this pull request Dec 8, 2017

Merge pull request ManageIQ#10 from israel-hdez/h-1328-rubocop

d204481

Enable rubocop in CI for pull requests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adaptations to have a working refresh for Hawkular's Inventory.next #10

Adaptations to have a working refresh for Hawkular's Inventory.next #10

israel-hdez commented Apr 17, 2017 •

edited

pilhuhn commented Apr 18, 2017

abonas commented Apr 18, 2017

abonas commented Apr 18, 2017

cfcosta left a comment

cfcosta Apr 18, 2017

israel-hdez Apr 18, 2017

cfcosta Apr 18, 2017

cfcosta Apr 18, 2017

cfcosta Apr 18, 2017

israel-hdez Apr 18, 2017

abonas commented Apr 18, 2017

israel-hdez commented Apr 18, 2017 •

edited

simon3z commented Apr 18, 2017

israel-hdez commented Apr 18, 2017

abonas commented Apr 19, 2017

Fryguy commented Apr 19, 2017

durandom commented Apr 21, 2017

moolitayer commented Apr 23, 2017

miq-bot commented Apr 24, 2017

durandom commented Apr 24, 2017

jkremser commented Apr 26, 2017 •

edited

miq-bot commented Apr 26, 2017

israel-hdez commented Apr 26, 2017

israel-hdez commented Apr 26, 2017

israel-hdez Apr 26, 2017

jkremser Apr 27, 2017

jpkrohling Apr 27, 2017

israel-hdez Apr 27, 2017

israel-hdez commented Apr 27, 2017

abonas commented Apr 27, 2017

israel-hdez commented Apr 27, 2017 •

edited

simaishi commented Apr 27, 2017

Adaptations to have a working refresh for Hawkular's Inventory.next #10

Adaptations to have a working refresh for Hawkular's Inventory.next #10

Conversation

israel-hdez commented Apr 17, 2017 • edited

pilhuhn commented Apr 18, 2017

abonas commented Apr 18, 2017

abonas commented Apr 18, 2017

cfcosta left a 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

abonas commented Apr 18, 2017

israel-hdez commented Apr 18, 2017 • edited

simon3z commented Apr 18, 2017

israel-hdez commented Apr 18, 2017

abonas commented Apr 19, 2017

Fryguy commented Apr 19, 2017

durandom commented Apr 21, 2017

moolitayer commented Apr 23, 2017

miq-bot commented Apr 24, 2017

durandom commented Apr 24, 2017

jkremser commented Apr 26, 2017 • edited

miq-bot commented Apr 26, 2017

israel-hdez commented Apr 26, 2017

israel-hdez commented Apr 26, 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

israel-hdez commented Apr 27, 2017

abonas commented Apr 27, 2017

israel-hdez commented Apr 27, 2017 • edited

simaishi commented Apr 27, 2017

israel-hdez commented Apr 17, 2017 •

edited

israel-hdez commented Apr 18, 2017 •

edited

jkremser commented Apr 26, 2017 •

edited

israel-hdez commented Apr 27, 2017 •

edited