Cross-linking Middleware server model with containers. #14043

rubenvp8510 · 2017-02-23T14:09:09Z

No description provided.

rubenvp8510 · 2017-02-24T18:36:43Z

@miq-bot add_label providers/hawkular

rubenvp8510 · 2017-02-24T18:37:22Z

@pilhuhn Could you please review this? Thank you!

pilhuhn · 2017-02-27T14:26:26Z

app/models/manageiq/providers/hawkular/middleware_manager/refresh_parser.rb

-        alternate << swap_part(machine_id[12, 4])
-        alternate << machine_id[16, 4]
-        alternate << machine_id[20, 12]
+        alternate << swap_part(machine_id[0, 8]) unless machine_id[0, 8].nil?


Isn't there a way to have those nil checks around the whole block? Or can parts of the machine_id be nil and others not?

pilhuhn · 2017-02-27T14:27:02Z

app/models/manageiq/providers/hawkular/middleware_manager/refresh_parser.rb

+        alternate << swap_part(machine_id[8, 4]) unless machine_id[8, 4].nil?
+        alternate << swap_part(machine_id[12, 4]) unless machine_id[12, 4].nil?
+        alternate << machine_id[16, 4] unless machine_id[16, 4].nil?
+        alternate << machine_id[20, 12] unless machine_id[20, 4].nil?


the array in the nil check does not match the one in front of it.

pilhuhn · 2017-02-28T08:26:17Z

LGTM
@miq-bot assign @bronaghs

bronaghs · 2017-02-28T15:20:30Z

@miq-bot assign @agrare

agrare

Couple of comments, also can you add a description explaining in a bit more detail about what you're doing here?
Also this is going to need some spec tests, if you have vcr cassettes you should re-record with some container cross linking and that alternate_machine_id method needs some specific tests where the machine_id is invalid.

agrare · 2017-02-28T15:50:52Z

app/models/manageiq/providers/hawkular/middleware_manager/refresh_parser.rb

@@ -34,7 +34,13 @@ def fetch_middleware_servers
            host_instance = find_host_by_bios_uuid(machine_id) ||
                            find_host_by_bios_uuid(alternate_machine_id(machine_id))

-            if host_instance
+            if server[:properties].try { |prop| prop['In Container'] } == 'true'


Looks like a good place to use fetch_path, try server.fetch_path(:properties, 'In Container') == true
Using a method could help readability as well

agrare · 2017-02-28T15:55:35Z

app/models/manageiq/providers/hawkular/middleware_manager/refresh_parser.rb

@@ -34,7 +34,13 @@ def fetch_middleware_servers
            host_instance = find_host_by_bios_uuid(machine_id) ||
                            find_host_by_bios_uuid(alternate_machine_id(machine_id))

-            if host_instance
+            if server[:properties].try { |prop| prop['In Container'] } == 'true'
+              container = Container.find_by(:name => machine_id)


Is the name guaranteed to be unique? You should at least add :ems_id to your find_by, and searching by :ems_id and :ems_ref is the generally accepted way to find an item.

I'm not sure about this, but this is the only way I have to x-linking the container with the hawkular agent, but it seems like we need to change this, WDYT @pilhuhn

agrare · 2017-02-28T15:57:02Z

app/models/manageiq/providers/hawkular/middleware_manager/refresh_parser.rb

@@ -101,6 +107,9 @@ def alternate_machine_id(machine_id)
        # providers store it in downcase, so, we let the upcase/downcase logic to other methods with more
        # business knowledge.
        # 1 - https://bugzilla.redhat.com/show_bug.cgi?id=1294461
+        return if machine_id[0, 8].nil? || machine_id[8, 4].nil? || machine_id[12, 4].nil? || machine_id[16, 4].nil? ||
+                  machine_id[20, 12].nil?


This is pretty gross, if you're trying to validate a UUID check out MiqUUID instead of doing anything on the string directly
If you do need to do something like this at least a method that is descriptive like return unless machine_id_valid?(machine_id)

I don't think we an detect from within the container what the env outside is and/or in which OpenShift/K8s cluster it is running.

pilhuhn · 2017-03-27T12:37:23Z

I think the issue is that the code on agent side that provides the container id makes/made an invalid assumption.

I wrote it the way that it reuses the machine-id property by just passing in -Dhawkular.agent.machine-id=.
My thought was that this way it ends up in the right place in ManageIQ, where ManageIQ code then just looks up a container instead of a (virtual) machine with that code.

Now there is code in MiQ that makes some trickery with the machine id in refresh_parser/alternate_machine_id that is not compatible with the container id.
While in fact the container id is the machine id of a container, there is apparently a real thing "machine id", which is normally baked into the hardware (or at least derived from hardware).

So we have several options that I can think of now:

we make the agent not set machine id, but a new property "container id", which is then forwarded all along to MiQ and the refresh parser then takes it and uses it for x-linking
we leave agent as is, but in the refresh parser we check before dorking with alernate-machine-id if the "in container" flag is set and then don't branch into the real-machine-id code, but do the x-linking with that id and the container provider.

I guess the 2nd is something we can do quickly, while the former needs more work (but is cleaner). I guess the former is something we can do post 4.5 - as we already know the logic on the MiQ side for x-linking to containers. We would then just not read the machine-id and branch, but the container-id.
The UI code could even stay the same.

rubenvp8510 · 2017-03-27T16:19:15Z

@pilhuhn

I agree on both solutions that you proposes here, but in the case of OpenShift containers, the machine-id is set to the pod name, which is unique for one OpenShift provider, but we could have two or more providers, in each of them we could have a pod with the same name. That doesn't happens with "hardware" machine-id I think because it's a UUID.

pilhuhn · 2017-03-27T18:36:19Z

@rubenvp8510 It is correct that this is no machine-id, but machine-id is not available for the containers, so we need something else.

abonas · 2017-03-28T14:01:35Z

I'm linking this here as it is the solution for option 1 which was originally planned for a later time
hawkular/hawkular-agent#306
So please sync so we will not end up with the agent solution merged for option 1, but with miq code that fits option 2 :)
@pilhuhn @rubenvp8510 @jmazzitelli

jmazzitelli · 2017-03-28T14:03:47Z

Yes, I need someone to peer review hawkular/hawkular-agent#306 to confirm it provides what the MiQ code needs.

rubenvp8510 · 2017-04-04T15:32:33Z

Now I'm getting the container id from the agent and doing the cross-linking using that is, also I addressed some code style comments.

rubenvp8510 · 2017-04-04T15:33:23Z

Could someone on this thread of comments review it please? Thank you!

abonas · 2017-04-04T19:56:49Z

app/models/manageiq/providers/hawkular/middleware_manager/refresh_parser.rb

-              server[:lives_on_id] = host_instance.id
-              server[:lives_on_type] = host_instance.type
+            if server.fetch_path(:properties, 'In Container') == 'true'
+              container_id = @ems.container_id(eap.feed)


is it possible that container_id will be empty? (and then the backing_ref will be corrupted)

Looks like travis is failing on exactly this case ref

Failure/Error: backing_ref = 'docker://' + container_id TypeError: no implicit conversion of nil into String

In theory, this shouldn't happen, if the hawkular agent is running into a container, it should have a valid container-id, but I'll validate this just in case.

abonas · 2017-04-04T19:59:17Z

app/models/manageiq/providers/hawkular/middleware_manager/refresh_parser.rb

+              backing_ref = 'docker://' + container_id
+              container = Container.find_by(:backing_ref => backing_ref)
+              if container
+                server[:lives_on_id] = container.id


can't lines 38-39 and 46-47 be unified for code reuse? they are essentially the same, just a different object is passed

abonas · 2017-04-04T20:03:06Z

app/models/manageiq/providers/hawkular/middleware_manager/refresh_parser.rb

@@ -101,6 +110,7 @@ def alternate_machine_id(machine_id)
        # providers store it in downcase, so, we let the upcase/downcase logic to other methods with more
        # business knowledge.
        # 1 - https://bugzilla.redhat.com/show_bug.cgi?id=1294461
+        return if machine_id.length < 31


isn't this addition causing alternate_machine_id method to sometimes return empty? then find_host_by bios_uuid will search with an empty var, no?

What changed in this PR that is causing a different machine_id to be passed to this method that is making this extra check necessary?

If the method find_host_by_bios_uuid received nil as a parameter it returns nil immediately, it doesn't try to find anything, so if the machine_id is not valid (line 43), host_instance will be nil.

@rubenvp8510 I don't see any change to how this method is being called so I'm not sure why this is in here, if there is an issue with this method can it be addressed in another PR? As far as I can see the calling code is just moved but the argument is still just eap.feed

abonas · 2017-04-04T20:05:04Z

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

@@ -107,6 +107,10 @@ def machine_id(feed)
      os_resource_for(feed).try(:properties).try { |prop| prop['Machine Id'] }
    end

+    def container_id(feed)


I wonder if this class a proper place for such methods (although this addition repeats another method that has been already here)

@cfcosta wdyt? Rails wise, where is the "right" place to put such methods?

I'd extract all resource-related stuff into it's own class, but I'd also keep it on providers/hawkular. There's no need to put it further away from where it is actually being used.

Could this be done on a separate PR?

The machine_id method is already here so if we are going to move it, best to do in a follow-up PR

abonas · 2017-04-05T13:05:24Z

Could this be done on a separate PR?

sure

agrare · 2017-04-05T13:20:11Z

Outside of the scope of this PR but why not normalize the UUID used for the :uid_ems in the refresh_parser before saving it to the DB? That way you don't have to try to look it up twice here you can always assume it will be a properly formatted UUID

pilhuhn · 2017-04-05T16:04:10Z

@agrare The "agare requested changes" flag is from over a month ago. Could you tell what stands between todays code and being merged (except for the test failure)?
If it is only "cosmetics" - could they be addressed in a subsequent PR?

agrare · 2017-04-05T16:07:20Z

@pilhuhn it looks like the last of my issues were just addressed in e2420bc

agrare

👍 Will merge when green

agrare · 2017-04-05T16:32:49Z

@rubenvp8510 please address the failing tests

miq-bot · 2017-04-05T17:52:45Z

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

abonas · 2017-04-05T18:42:37Z

@miq-bot add_label fine/yes

agrare · 2017-04-05T18:43:47Z

~~Was this for a BZ? If so it should be in the description and if not we'll need to create one in order to backport it.~~
Scratch that I've been notified no BZ needed to backport to fine yet

Cross-linking Middleware server model with containers. (cherry picked from commit 0a3d7e4)

simaishi · 2017-04-05T22:33:44Z

Fine backport details:

$ git log -1
commit 22d25adc220d27c535a057c4c9cfb53b56cea82c
Author: Adam Grare <[email protected]>
Date:   Wed Apr 5 14:30:15 2017 -0400

    Merge pull request #14043 from rubenvp8510/xlink-container
    
    Cross-linking Middleware server model with containers.
    (cherry picked from commit 0a3d7e40ae82b13aa7d1c7d80a709b4763370ac6)

chessbyte assigned blomquisg Feb 23, 2017

rubenvp8510 force-pushed the xlink-container branch 2 times, most recently from 813d768 to cbfaab7 Compare February 23, 2017 18:53

miq-bot added the providers/hawkular label Feb 24, 2017

pilhuhn reviewed Feb 27, 2017

View reviewed changes

rubenvp8510 force-pushed the xlink-container branch 2 times, most recently from a16a8c1 to 2330a8e Compare February 27, 2017 16:20

rubenvp8510 mentioned this pull request Feb 27, 2017

Show cross linking containers links ManageIQ/manageiq-ui-classic#484

Merged

miq-bot assigned bronaghs and unassigned blomquisg Feb 28, 2017

miq-bot assigned agrare and unassigned bronaghs Feb 28, 2017

agrare requested changes Feb 28, 2017

View reviewed changes

rubenvp8510 mentioned this pull request Mar 7, 2017

Error in refresh_parser.rb when using startup params: -Dhawkular.agent.machine.id=machineA ManageIQ/manageiq-ui-classic#617

Closed

rubenvp8510 force-pushed the xlink-container branch from 2330a8e to c31a2fa Compare March 27, 2017 18:13

rubenvp8510 force-pushed the xlink-container branch 2 times, most recently from f4eccd4 to f5182ff Compare April 4, 2017 15:19

abonas reviewed Apr 4, 2017

View reviewed changes

rubenvp8510 force-pushed the xlink-container branch 3 times, most recently from ca48c4c to e2420bc Compare April 5, 2017 16:03

agrare approved these changes Apr 5, 2017

View reviewed changes

Cross-linking Middleware server model with containers.

0e59b1c

rubenvp8510 force-pushed the xlink-container branch from e2420bc to 0e59b1c Compare April 5, 2017 17:49

agrare added the enhancement label Apr 5, 2017

agrare merged commit 0a3d7e4 into ManageIQ:master Apr 5, 2017

agrare added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 5, 2017

miq-bot added the fine/yes label Apr 5, 2017

simaishi pushed a commit that referenced this pull request Apr 5, 2017

Merge pull request #14043 from rubenvp8510/xlink-container

22d25ad

Cross-linking Middleware server model with containers. (cherry picked from commit 0a3d7e4)

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

chessbyte added providers/middleware and removed providers/hawkular labels Nov 8, 2017

Cross-linking Middleware server model with containers. #14043

Cross-linking Middleware server model with containers. #14043

Conversation

rubenvp8510 commented Feb 23, 2017

rubenvp8510 commented Feb 24, 2017

rubenvp8510 commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pilhuhn commented Feb 28, 2017

bronaghs commented Feb 28, 2017

agrare 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

pilhuhn commented Mar 27, 2017 • edited Loading

rubenvp8510 commented Mar 27, 2017 • edited Loading

pilhuhn commented Mar 27, 2017

abonas commented Mar 28, 2017

jmazzitelli commented Mar 28, 2017

rubenvp8510 commented Apr 4, 2017

rubenvp8510 commented Apr 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonas commented Apr 5, 2017

agrare commented Apr 5, 2017

pilhuhn commented Apr 5, 2017

agrare commented Apr 5, 2017

agrare left a comment

Choose a reason for hiding this comment

agrare commented Apr 5, 2017

miq-bot commented Apr 5, 2017

abonas commented Apr 5, 2017

agrare commented Apr 5, 2017 • edited Loading

simaishi commented Apr 5, 2017

pilhuhn commented Mar 27, 2017 •

edited

Loading

rubenvp8510 commented Mar 27, 2017 •

edited

Loading

agrare commented Apr 5, 2017 •

edited

Loading