VMware provider IP discovery fix #221

slemrmartin · 2018-03-26T14:30:26Z

Recognition of vSphere product based on API info instead ports scanning
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1559347

Steps to reproduce:
Have a VMWare Virtual Center (vSphere) on known IP

Navigate to Compute -> Infrastructure -> Providers
Configuration -> Discover Infrastructure Providers
Input valid IP range and choose valid provider, click Start

Cc @agrare, @Ladas
@miq-bot add_label bug

Recognition of vSphere product based on API info instead ports scanning Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1559347

Ladas · 2018-03-26T22:00:02Z

app/models/manageiq/providers/vmware/discovery.rb

          end
+        rescue StandardError => err


rescue StandardError => err is the same as rescue => err

Yes, it was as you write before rubocop -a

Ladas

Looks good code wise, @agrare will need to review correctness, since that is outside of my area of expertise. :-)

agrare · 2018-03-28T14:46:28Z

app/models/manageiq/providers/vmware/discovery.rb

-          #   a network than VC servers.
+        # Obtains info about IP address from vSphere Web Service API
+        # @param ost [OpenStruct]
+        def self.access_webservice(ost)


Since you're only using ost.ipaddr can you just pass in the ipaddress? The whole openstruct is leaking too much info from the caller to here

agrare · 2018-03-28T14:47:07Z

app/models/manageiq/providers/vmware/discovery.rb

+                         _log&.debug("Vmware::Discovery: ip = #{ost.ipaddr}, Machine is an VMWare Server product.")
+                         :vmwareserver
+                       else
+                         _log&.error("Vmware::Discovery: ip: #{ost.ipaddr}, Unknown product: #{info&.productLineId}")


Should be ip = ... to stay consistent with the rest of these

Why consistency of sentence in log (between different log levels) has bigger priority than given information? Isn't it valuable to know, which unexpected value is inside?

ahhh, sorry, you mean = instead of : ...I noticed after I submitted comment above :)

Oh yeah sorry I meant the : part, not the rest of the info :)

agrare · 2018-03-28T14:49:51Z

app/models/manageiq/providers/vmware/discovery.rb


-          # Check if we have ESX ports open
-          if !found_vc && ost.discover_types.include?(:esx) && ManageIQ::NetworkDiscovery::Port.any_open?(ost, ESX_PORTS)
+          ost.hypervisor << hypervisor if hypervisor


Don't you still need to check if the hypervisor type is in the ost.discover_types?

agrare · 2018-03-28T14:52:39Z

app/models/manageiq/providers/vmware/discovery.rb

-            return
+          # Get Info from VMware (vSphere) Web Service API
+          access_webservice(ost) do |info|
+            if add_hypervisor(ost, info)


I feel like add_hypervisor and add_os are doing too much.
I would rather see something like:

hvisor_type = hypervisor_type(info) if ost.discover_types.include?(hvisor_type) ost.hypervisor << hvisor_type ost.os << hvisor_os_type(info) end

agrare · 2018-03-28T14:53:52Z

app/models/manageiq/providers/vmware/discovery.rb

-          # Check if we have VC ports
-          if ost.discover_types.include?(:virtualcenter)
-            checked_vc = true
+          yield info


I don't think yielding the info is needed when there's no cleanup here. How about just info = retrieve_about_info(ost.ipaddr) ?

please explain, why is this better - do you mean that code is more readable without yield?

agrare · 2018-03-28T14:58:05Z

app/models/manageiq/providers/vmware/discovery.rb


-          checked_vc = false
-          found_vc = false
+          _log&.debug("Vmware::Discovery: ip = #{ost.ipaddr}, Connected to VMware webservice.")


I don't think _log&. is needed since as long as you include VMDB::Logging _log will always be defined

slemrmartin · 2018-03-29T09:55:49Z

Thanks for review!
Additionally I made related PR: ManageIQ/manageiq#17229 with small fix
Doesn't block this

agrare · 2018-03-29T12:45:23Z

app/models/manageiq/providers/vmware/discovery.rb

-          # Check if VMware Server
-          ost.hypervisor << :vmwareserver if ManageIQ::NetworkDiscovery::Port.scan_open(ost, SERVER_CONSOLE_PORTS).length == 2
+          # Get Info from VMware (vSphere) Web Service API
+          info = retrieve_webservice_info(ost.ipaddr)



How about return if info.nil? here and not need to do info&.productLineId and info&.osType below?

I saw codeclimate only few times, but doesn't it evaluate as higher cognitive complexity when using return in middle of method?

If the code makes more sense I wouldn't worry about what codeclimate says. If you'd rather one return you can raise in retrieve_webservice_info if vim.about is nil

agrare · 2018-03-29T12:46:11Z

app/models/manageiq/providers/vmware/discovery.rb

-          $log&.debug("Vmware::Discovery: ip = #{ost.ipaddr}, Connected to VMware webservice. Machine is either ESX or VirtualCenter.")
+          if ost.discover_types&.include?(hvisor_type)
+            ost.hypervisor << hvisor_type
+            ost.os << hypervisor_os_type(info&.osType)


Probably would be good to add unless info.osType.nil? here so we don't potentially have ost.os = [nil] and cause the caller issues

ost.os should result to be :mswin or :linux only

I think it is better option than result ost.hypervisor.size != ost.os.size

That's fair, osType is a required field in AboutInfo so it shouldn't be nil

I still don't see where is the problem - how should be possible to have ost.os = [nil] with this code?

If osType will be nil, then hypervisor_os_type returns :linux

Ah was getting ahead of myself, I was thinking hypervisor_os_type should check if osType is nil instead of just defaulting to :linux but then ost.os would be [nil] so thought better to check if nil before calling it so ost.os = [nil] was an intermediate step in my head that I didn't write down haha

Ok, now I'm confused what is your recommendation :))
If I check if osType isn't nil instead of defaulting to :linux, then ost.hypervisor and ost.os arrays can have different sizes. It doesn't cause error, but I don't know if next code is ok with that

It looks like it just won't save the operating_system automatically https://github.com/ManageIQ/manageiq/blob/master/app/models/host.rb#L994
If you want to just have it default to linux that's fine

agrare · 2018-03-29T12:49:49Z

app/models/manageiq/providers/vmware/discovery.rb


-          # Next check for ESX or VC. Since VC shares some port numbers with ESX, we check VC before ESX
+        rescue => err
+          _log.warn("Vmware::Discovery: ip = #{ost.ipaddr}, Failed to connect to VMware webservice: #{err}.")


Since this will be the case for every IP that isn't a VCenter on the subnet being scanned this should still be debug

agrare · 2018-03-29T12:51:00Z

app/models/manageiq/providers/vmware/discovery.rb

+        def self.retrieve_webservice_info(ip)
+          require 'VMwareWebService/MiqVimClientBase'
+          vim = MiqVimClientBase.new(ip, "test", "test")
+          info = vim&.sic&.about


I just checked again and :about is an attr_reader on VimService so you can skip the .sic in the middle

agrare · 2018-03-29T12:51:50Z

@slemrmartin looking really good, just a couple of small things

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

miq-bot · 2018-03-29T13:37:54Z

Checked commits slemrmartin/manageiq-providers-vmware@abe3151~...88277fc with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

app/models/manageiq/providers/vmware/discovery.rb

❗ - Line 19, Col 9 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

agrare

👍 will merge when green

VMware provider IP discovery fix

simaishi · 2018-04-12T02:49:09Z

@slemrmartin Can this be gaprindashvili/yes ?

slemrmartin · 2018-04-12T07:23:53Z

Not sure - @agrare ?

agrare · 2018-04-12T13:04:20Z

@simaishi yes it can, #181 was backported so it should be a clean cherry-pick. Any further back and @slemrmartin will need to create a backport PR.

VMware provider IP discovery fix (cherry picked from commit 56b62a7) https://bugzilla.redhat.com/show_bug.cgi?id=1566557

simaishi · 2018-04-12T14:07:12Z

Gaprindashvili backport details:

$ git log -1
commit b3fe8b34b664cbf06e78c5c2d2cb7feb818a47ed
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Mar 29 10:01:54 2018 -0400

    Merge pull request #221 from slemrmartin/infra-provider-discovery
    
    VMware provider IP discovery fix
    (cherry picked from commit 56b62a704e62c3e9aa7b64968cd6b75c984ea26f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1566557

VMware provider IP discovery fix

abe3151

Recognition of vSphere product based on API info instead ports scanning Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1559347

miq-bot added the bug label Mar 26, 2018

agrare self-assigned this Mar 26, 2018

Ladas reviewed Mar 26, 2018

View reviewed changes

Ladas approved these changes Mar 27, 2018

View reviewed changes

agrare reviewed Mar 28, 2018

View reviewed changes

slemrmartin force-pushed the infra-provider-discovery branch from 0628add to a5bc7b4 Compare March 29, 2018 09:39

slemrmartin mentioned this pull request Mar 29, 2018

Providers discovery without unspecified discovery types (FIX) ManageIQ/manageiq#17229

Merged

agrare reviewed Mar 29, 2018

View reviewed changes

VMWare IP Discovery refactoring

88277fc

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

slemrmartin force-pushed the infra-provider-discovery branch from a5bc7b4 to 88277fc Compare March 29, 2018 13:34

agrare approved these changes Mar 29, 2018

View reviewed changes

agrare merged commit 88277fc into ManageIQ:master Mar 29, 2018

agrare added a commit that referenced this pull request Mar 29, 2018

Merge pull request #221 from slemrmartin/infra-provider-discovery

56b62a7

VMware provider IP discovery fix

agrare added this to the Sprint 83 Ending Apr 9, 2018 milestone Mar 29, 2018

slemrmartin deleted the infra-provider-discovery branch March 29, 2018 14:04

simaishi added blocker gaprindashvili/yes labels Apr 12, 2018

simaishi pushed a commit that referenced this pull request Apr 12, 2018

Merge pull request #221 from slemrmartin/infra-provider-discovery

b3fe8b3

VMware provider IP discovery fix (cherry picked from commit 56b62a7) https://bugzilla.redhat.com/show_bug.cgi?id=1566557

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VMware provider IP discovery fix #221

VMware provider IP discovery fix #221

slemrmartin commented Mar 26, 2018

Ladas Mar 26, 2018

slemrmartin Mar 27, 2018

Ladas left a comment

agrare Mar 28, 2018

agrare Mar 28, 2018

slemrmartin Mar 29, 2018

slemrmartin Mar 29, 2018

agrare Mar 29, 2018

agrare Mar 28, 2018

agrare Mar 28, 2018

agrare Mar 28, 2018

slemrmartin Mar 29, 2018

agrare Mar 28, 2018

slemrmartin commented Mar 29, 2018

agrare Mar 29, 2018

slemrmartin Mar 29, 2018

agrare Mar 29, 2018

agrare Mar 29, 2018

slemrmartin Mar 29, 2018

agrare Mar 29, 2018

slemrmartin Mar 29, 2018

agrare Mar 29, 2018

slemrmartin Mar 29, 2018

agrare Mar 29, 2018

agrare Mar 29, 2018

agrare Mar 29, 2018

agrare commented Mar 29, 2018

miq-bot commented Mar 29, 2018

agrare left a comment

simaishi commented Apr 12, 2018

slemrmartin commented Apr 12, 2018

agrare commented Apr 12, 2018

simaishi commented Apr 12, 2018

VMware provider IP discovery fix #221

VMware provider IP discovery fix #221

Conversation

slemrmartin commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas 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

slemrmartin commented Mar 29, 2018

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

agrare commented Mar 29, 2018

miq-bot commented Mar 29, 2018

agrare left a comment

Choose a reason for hiding this comment

simaishi commented Apr 12, 2018

slemrmartin commented Apr 12, 2018

agrare commented Apr 12, 2018

simaishi commented Apr 12, 2018