Use inventory v4 #75

josejulio · 2017-10-12T16:15:53Z

Switching from inventory v3 to v4.

Pending:

Operations ([WIP] Updates operations to work with inventory v4 #110)
~~Live metrics (the maps that are in manageiq core)~~ No longer needed
Gem release

Depends on ManageIQ/manageiq-providers-kubernetes#173

josejulio · 2017-10-12T16:17:57Z

@miq-bot add_label wip, refactoring

josejulio · 2017-10-17T19:38:26Z

The fetching part is mostly done, need to finish with fetch_availability and metrics.

I plan to squash into a single commit all this fetching stuff, clean up a bit (remove no longer used methods) and update tests.

Inventory no longer uses Canonical Paths, they do have an id (which is opaque) but the name attribute is used instead of parsing the CPs.
The inventory API allows us to get a whole tree (e.g. the whole server for parsing), calls no longer require to specify the feed.

miq-bot · 2017-10-20T04:53:53Z

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

miq-bot · 2017-10-23T15:54:07Z

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

miq-bot · 2017-10-24T19:34:35Z

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

israel-hdez · 2017-11-07T22:05:00Z

@miq-bot add_label gaprindashvili/no

josejulio · 2017-11-15T16:38:07Z

**

 - Linter/Yaml - missing config files

I don't really know what it means.
Any idea?

lucasponce · 2017-11-16T16:54:40Z

Thx @josejulio as for HAWKULAR-1275 I'm going to base my changes on top of your PR.

israel-hdez

Uff... Dropped some comments.

israel-hdez · 2017-10-24T21:06:35Z

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

+            server.properties[feature] = 'true' if agent_config.try(:[], feature) == true
+          end
+          if server.properties['In Container'] == 'true'
+            container_id = collector.container_id(eap.feed)['Container Id']


How is this working without the container_id method in the collector?

I'm sure its not working :)
I didn't notice that, not sure why is not failing. I suppose I should use server.config instead

Updated to fix that.

israel-hdez · 2017-11-16T18:21:57Z

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

-    def raw_availability_data(*args)
-      connection.metrics.avail.raw_data(*args)
+    def server_groups_from_host_controller(host_controller)
+      host_controller.children_by_type('Domain Server Group')


As an improvement.... Probably we could monkey patch the Resources class of the client gem. Although, I don't know if that would really be an "improvement". I mean... this isn't making any API calls, so it doesn't make much sense to be here in the collector.

I agree, we could monkey patch the Resource class and remove these methods from the collector.

israel-hdez · 2017-11-16T19:08:33Z

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

        collection = persister.middleware_deployments
-        fetch_availabilities_for(feeds, collection, collection.model_class::AVAIL_TYPE_ID) do |deployment, availability|
+        fetch_availabilities_for(collector.deployments, collection, collection.model_class::AVAIL_TYPE_ID) do |deployment, availability|


And this is very similar regarding hitting HServices twice to get again the deployments.

See my other comment, I think we should refactor it to fetch each top resource only once.

israel-hdez · 2017-11-16T19:12:50Z

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

          deployment.status = process_deployment_availability(availability.try(:[], 'data').try(:first))
        end
+        subdeployments_by_deployment_id = collector.subdeployments.group_by(&:parent_id)


I find this a little inefficient in terms of performance. It is hitting Hservices for each available deployment, fetching always the same data and, thereafter, filtering it.

I think is only hitting Hservices once, grouping by their parent_id after fetching.

Fetch all the deployments, process them all (see end on line 195)

Fetch all the subdeployments, group then by their parent_id and process them.

Yes, you are right. My eyes saw it inside the fetch_availabilities_for block

israel-hdez · 2017-11-16T19:15:32Z

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

-        fetch_domain_availabilities(feeds_of_interest)
+        fetch_deployment_availabilities
+        fetch_server_availabilities(collector.eaps)
+        fetch_server_availabilities(collector.domain_servers)


These two lines are hitting HServices but servers were previously obtained in fetch_middleware_servers and fetch_domain_servers. Perhaps you can store servers in instance variables to save another hit to HServices.

I could do that, but I don't know if is OK (memory-wise) to store all the servers.
Maybe we could refactor this to process each top resource in a single go?
e.g.

Fetch server A

Process server A

Process deployments of server A

Process datasources of server A

etc..

You already have them in memory. I was thinking on just not discarding the data by using instance variables :) (I think it's easier than the refactor)

They are lazy fetched from the server see the resource method [1]. So i shouldn't have them all at once, unless I'm using a non-lazy enumeration method, which is also possible.

That said, I can do as you say and we can later revisit that if there is any performance issue.

[1] https://github.com/hawkular/hawkular-client-ruby/blob/master/lib/hawkular/inventory/inventory_api.rb#L75

Oh! So, there is some underlying magic!
Well... In that case, I'll leave to you the decision :)

israel-hdez · 2017-11-16T19:35:33Z

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

-        end
-      end
-
-      def parse_deployment_name(name)


Great! No more parsing of names :)

Praise the new inventory!

israel-hdez · 2017-11-16T19:47:24Z

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

        'Running Mode'         => 'NORMAL',
        'Version'              => '9.0.2.Final',
        'Product Name'         => 'WildFly Full',
        'Host State'           => 'running',
        'Is Domain Controller' => 'true',
        'Name'                 => 'master',
      }
+      type_id = 'Host Controller'
      feed = 'master.Unnamed%20Domain'


Is this the right feed? In other places you replaced this with wf-domain.

I suppose the feed doesn't matter here, as we are using the id and never the feed.
I'm only using the feed to know which agent (and operative system) a server belongs to.
I'm going to update it to wf-domain, to remain consistent.

@jotak Is it a good way to know that? (using feed id to link server with agent and operative system)

I'm not sure to see where you link servers with agents, but to answer, yes the feed id is more or less the agent id and could be used in that purpose.

I fetch the agents and store their config on a map of feeds for using later.

https://github.com/ManageIQ/manageiq-providers-hawkular/pull/75/files#diff-a6822e8aa662958bb0ab11a0ec7cc558R23
https://github.com/ManageIQ/manageiq-providers-hawkular/pull/75/files#diff-a6822e8aa662958bb0ab11a0ec7cc558R111

israel-hdez · 2017-11-16T20:07:53Z

spec/models/manageiq/providers/hawkular/middleware_manager/event_catcher/stream_spec.rb

@@ -125,7 +198,7 @@
    let!(:db_server) do
      ems_hawkular.middleware_servers.create(
        :feed       => 'f1',
-        :ems_ref    => '/t;hawkular/f;f1/r;resource1',


ems_ref is the primary key in MiQ (isolated to each provider)... What will happen if two independent servers (aka, two agents) happen to have the same server id??? Having the feed in the ems_ref worked, to some extent, to avoid collisions. But, now, is it just a bare ID???

Previously, potential collisions happened for the domain case, where ems_ref included the server name instead of a random guid like in the case of standalone servers. So, if you have two domains each one with a server named server-one a collision in MiQ was being avoided by having the feed id in ems_ref. How would that be handled now?... May collisions like this never happen?

I haven't seen how the IDs look like, so I'm unsure.

Ids are opaque (kind of), and unique, so it must be fine. In the inventory server, it's the primary key for resources, there's no composed key. And in its implementation it contains the feedid.

You can assume that IDs are unique. Currently implementation uses the old "CanonicalPath" as their "id".

edit: Whoops, wrong thread

Ok, I'm happy if IDs are unique :)

israel-hdez · 2017-11-16T20:17:35Z

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

@@ -97,10 +96,10 @@ def assert_specific_domain_server
  it 'will perform a full refresh on 127.0.0.1 even though the os type is not there yet' do
    # using different cassette that represents the hawkular inventory without the operating system resource type
    # TODO: Make this work with live tests
+    # Perhaps having a separate MW without OS configuration on its config


Did you make it work? (asking because you removed :record => :none)

Whoops, didn't make it work for live tests. Only updated the yaml to remove all domain and OS interaction from the server. WIll add back the :record => :none.

lucasponce · 2017-11-20T09:08:06Z

@josejulio minor bug from my side (as I am using this branch for hawkular-1275)

I changed locally the list_feeds by root_resources in the verify_credentials

    def verify_credentials(_auth_type = nil, options = {})
      begin
        # As the connect will only give a handle
        # we verify the credentials via an actual operation
        connect(options).inventory.root_resources

I saw that meanwhile setting up my environment but I think you would like to fix it on this one.

josejulio · 2017-11-21T15:58:10Z

@miq-bot remove_label wip

…ion to success "will perform a full refresh on 127.0.0.1 even though the os type is not there yet"

…need to query services

josejulio · 2017-11-21T16:02:10Z

@israel-hdez
Updated the code, can you take a second look?

israel-hdez

I think it looks good.

israel-hdez · 2017-11-21T20:57:34Z

app/models/manageiq/providers/hawkular/middleware_manager/event_catcher/stream.rb

@@ -58,15 +57,15 @@ def fetch_availabilities
  def fetch_server_availabilities(parser)


We'll have to reconsider how these methods work.
Didn't you put some events regarding the availabilities? I guess we should now be using them. But that's for another PR.

There are some events wrt availability, but aren't enabled for every resource.

israel-hdez · 2017-11-23T17:58:22Z

I tried these changes and I'm curious about why we can no longer identify which server group a server belongs when it's not running.
With the WF example domain config, there is the server-thee that is created in a stopped state. It belongs IIRC, to other-server-group, but apparently its Server Group property comes unset from Hawkular and, thus, MiQ is unable to identify and set the belonging group.

josejulio · 2017-11-23T21:37:55Z

I tried these changes and I'm curious about why we can no longer identify which server group a server belongs when it's not running.
With the WF example domain config, there is the server-thee that is created in a stopped state. It belongs IIRC, to other-server-group, but apparently its Server Group property comes unset from Hawkular and, thus, MiQ is unable to identify and set the belonging group.

I'm not sure, but i suppose Hawkular is no longer telling us that information.
I'll check, maybe i'm missing something.

josejulio · 2017-11-23T21:44:45Z

So, after checking, we don't have the server-group on the config when the server is not running:

"config": {
    "Server State": "STOPPED"
  }

If this did before, maybe we should report a bug on the agent :)

israel-hdez · 2017-11-28T20:20:13Z

@josejulio I remember that it worked fine previously. So, yes! A bug on the agent should be reported :)

* Refresher class is now inheriting from ManagerRefresher. This forces to include a builder class, which creates the required parser, collector and persister objects when the refresh is happening. * In the settings file, a new property is added. Because now the refresher is inheriting from ManagerRefresher, targeted refresh must be explicitly enabled. * The parser has been broken into multiple parsers. This is in preparation to implement targeted refresh.

…files The specs changed its form. Now, the specs aren't using the protected nor private methods to check behaviours. The old parser/middleware_manager_spec.rb file is now removed since it is now replaced by the new files.

israel-hdez · 2017-11-29T00:17:40Z

@josejulio BTW.... I saw some specs failing, even with the operations fixes. Are you also having some errors in the specs?

josejulio · 2017-11-29T00:41:08Z

All specs are passing on my side.

Break parser into entity focused parsers

josejulio · 2017-11-29T16:05:35Z

@lucasponce @jshaughn
Heads up!
@israel-hdez refactored some of the code and is already on this PR.
josejulio#1

Secondary references in inventory collection are experimental for now. Stop using them and use an index in the persister to link servers with server groups.

Fix rubocop warnings and stop using inventory collection secondary refs

lucasponce · 2017-11-29T16:39:25Z

@lucasponce @jshaughn
Heads up!
@israel-hdez refactored some of the code and is already on this PR.
josejulio#1

Thanks for the update.
I wonder if due the status we are if it makes sense to push a whole Hawkular-1275 instead to split with inventory v4 only and then our work.
I don't know :-) still finishing the changes locally here
https://github.com/lucasponce/manageiq-providers-hawkular/tree/hawkular-1275
Where @jshaughn is also PR his changes.
I don't know a lot of pieces moving on the same code, but yes, I guess first @josejulio needs to consolidate his changes, then mine, and last Jay as alerting is the user of most of the changes.

jshaughn · 2017-11-29T16:50:12Z

I wonder if due the status we are if it makes sense to push a whole Hawkular-1275 instead to split with inventory v4 only and then our work.

+1, at this point we need to consolidate into one branch (per repo). For the hawkular-client-ruby and and manageiq-provider-hawkular I'd suggest hawkular-1275, hosted by lucas or josejulio. The 1275 branches would later be merged to master. But for manageiq and manageiq-ui-classic I'd maybe suggest hawkular-ga, which would likely be held from master until a later date?

So, I think @josejulio @israel-hdez and @lucasponce should probably coordinate to decide who will own the consolidated branch, and then how best to consoldiate. Let's meet if that would help.

Re-record VCR cassettes for failing tests

miq-bot · 2017-11-29T17:24:46Z

Checked commits josejulio/manageiq-providers-hawkular@4ee81f8~...4035d76 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
42 files checked, 1 offense detected

**

💣 💥 🔥 🚒 - Linter/Yaml - missing config files

miq-bot changed the title ~~WIP - Use inventory v4~~ [WIP] WIP - Use inventory v4 Oct 12, 2017

miq-bot added refactoring wip labels Oct 12, 2017

josejulio changed the title ~~[WIP] WIP - Use inventory v4~~ [WIP] Use inventory v4 Oct 12, 2017

josejulio mentioned this pull request Oct 18, 2017

hawkular-1259: remove old inventory module, start updating itests hawkular/hawkular-client-ruby#239

Merged

miq-bot added the unmergeable label Oct 20, 2017

josejulio force-pushed the hawkular-1259 branch from d1259f0 to fe42c76 Compare October 20, 2017 22:41

miq-bot removed the unmergeable label Oct 20, 2017

miq-bot added the unmergeable label Oct 23, 2017

josejulio force-pushed the hawkular-1259 branch from fe42c76 to 4fcd224 Compare October 23, 2017 16:55

miq-bot removed the unmergeable label Oct 23, 2017

miq-bot added the unmergeable label Oct 24, 2017

josejulio force-pushed the hawkular-1259 branch from acd37d6 to 193086e Compare November 3, 2017 18:37

miq-bot added the gaprindashvili/no label Nov 7, 2017

josejulio force-pushed the hawkular-1259 branch from 193086e to d0d6160 Compare November 13, 2017 23:43

miq-bot removed the unmergeable label Nov 13, 2017

josejulio force-pushed the hawkular-1259 branch from d0d6160 to 16abd33 Compare November 13, 2017 23:47

israel-hdez reviewed Nov 16, 2017

View reviewed changes

josejulio added 2 commits November 21, 2017 09:59

WIP - Use inventory v4

4ee81f8

WIP - Adding domains

a32d7c5

miq-bot changed the title ~~[WIP] Use inventory v4~~ Use inventory v4 Nov 21, 2017

miq-bot removed the wip label Nov 21, 2017

josejulio added 4 commits November 21, 2017 09:59

Updated feed to be more consistent in the usages

69eb4e3

Avoid recording spec that requires more work with the live configurat…

06efeb2

…ion to success "will perform a full refresh on 127.0.0.1 even though the os type is not there yet"

Updates call to root_resources when doing provider auth verification

09d587f

Monkey patch Inventory::Resource to handle operations where we don't …

45d87c5

…need to query services

josejulio force-pushed the hawkular-1259 branch from c17a461 to 45d87c5 Compare November 21, 2017 16:00

israel-hdez reviewed Nov 21, 2017

View reviewed changes

josejulio mentioned this pull request Nov 22, 2017

[WIP] Updates operations to work with inventory v4 #110

Closed

israel-hdez added 4 commits November 28, 2017 18:12

Remove MWM fat parser and update event catcher to stop using it

3b6c3da

Fixing and moving datasources and domain parser specs to its own files

851ba3d

Merge pull request #1 from israel-hdez/targeted-refresh-prep

e340f29

Break parser into entity focused parsers

israel-hdez and others added 2 commits November 29, 2017 10:06

Fix rubocop warnings and stop using inventory collection secondary refs

b2e571c

Secondary references in inventory collection are experimental for now. Stop using them and use an index in the persister to link servers with server groups.

Merge pull request #2 from israel-hdez/targeted-refresh-prep

15e3238

Fix rubocop warnings and stop using inventory collection secondary refs

israel-hdez and others added 2 commits November 29, 2017 11:12

Re-record VCR cassettes for failing tests

677934c

Merge pull request #3 from israel-hdez/targeted-refresh-prep

4035d76

Re-record VCR cassettes for failing tests

josejulio closed this Nov 30, 2017

		@@ -58,15 +57,15 @@ def fetch_availabilities
		def fetch_server_availabilities(parser)

Use inventory v4 #75

Use inventory v4 #75

Conversation

josejulio commented Oct 12, 2017 • edited

josejulio commented Oct 12, 2017

josejulio commented Oct 17, 2017

miq-bot commented Oct 20, 2017

miq-bot commented Oct 23, 2017

miq-bot commented Oct 24, 2017

israel-hdez commented Nov 7, 2017

josejulio commented Nov 15, 2017

lucasponce commented Nov 16, 2017

israel-hdez 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josejulio Nov 17, 2017 • edited

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

josejulio Nov 17, 2017 • edited

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

lucasponce commented Nov 20, 2017

josejulio commented Nov 21, 2017

josejulio commented Nov 21, 2017

israel-hdez left a 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 Nov 23, 2017

josejulio commented Nov 23, 2017

josejulio commented Nov 23, 2017

israel-hdez commented Nov 28, 2017

israel-hdez commented Nov 29, 2017

josejulio commented Nov 29, 2017

josejulio commented Nov 29, 2017

lucasponce commented Nov 29, 2017

jshaughn commented Nov 29, 2017

miq-bot commented Nov 29, 2017

josejulio commented Oct 12, 2017 •

edited

josejulio Nov 17, 2017 •

edited

josejulio Nov 17, 2017 •

edited