Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use only hypervisor hostname to match infra host with cloud vm #186

Merged
merged 1 commit into from
Jan 8, 2018
Merged

Use only hypervisor hostname to match infra host with cloud vm #186

merged 1 commit into from
Jan 8, 2018

Conversation

petrblaho
Copy link

@petrblaho petrblaho commented Jan 4, 2018

Hosts are stored in db with only hostname part of hostname.domain which
we can see in OpenStack.
That caused issue with matching VMs with Hosts when Cloud OpenStack had
Infra OpenStack related to it.

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

screenshot-2018-1-4 manageiq infrastructure providers
screenshot-2018-1-4 manageiq nodes

@aufi
Copy link
Member

aufi commented Jan 5, 2018

The fix looks reasonable to me 👍 Waiting till tests - IP/hostname issue is fixed.

@@ -286,7 +286,7 @@ def vms

collector.vms.each do |s|
if hosts && !s.os_ext_srv_attr_host.blank?
parent_host = hosts.find_by('lower(hypervisor_hostname) = ?', s.os_ext_srv_attr_host.downcase)
parent_host = hosts.find_by('lower(hypervisor_hostname) = ?', s.os_ext_srv_attr_host.split('.').first.downcase)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe split(/\.[^0-9]/)? (and IPv6 doesn't have dots)..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use os_ext_srv_attr_hypervisor_hostname? Is that field available on the OSP server model from the API?

https://ask.openstack.org/en/question/50587/os-ext-srv-attrhypervisor_hostname-and-os-ext-srv-attrhost-difference/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os_ext_srv_attr_hypervisor_hostname should be available in OSP running on top of libvirt/kvm. I am not sure we should generally suppose that this field is available when running OS on other hypervisor (as answer you linked states).

Docs https://developer.openstack.org/api-ref/compute/#show-server-details says that host is The name of the compute host on which this instance is running. and hypervisor_hostname is The hypervisor host name provided by the Nova virt driver. For the Ironic driver, it is the Ironic node uuid. That seems like it should be populated by other hypervisors than libvirt/kvm. Anyway in TripleO installed OSP using libvirt/kvm both os_ext_srv_attr_hypervisor_hostname and os_ext_srv_attr_host have the same value.

Hosts in ManageIQ got their hypervisor_hostname attribute from name of server in Nova from Undercloud (

indexed_servers.fetch_path(host.instance_uuid).try(:name)
) and this name is defined by TripleO Heat Templates used to deploy OSP.

So in the end we trust that TripleO (THT) sets the hostnames for compute nodes to the same value (almost - hostname vs fqdn) as their names in Undercloud Nova.

@blomquisg
Copy link
Member

The test that's failing seems to have nothing to do with the changes here, but something in the OpenStack Refresher is broken, so maybe related??

Failures:

  1. ManageIQ::Providers::Openstack::CloudManager::Refresher when paired with a infrastructure provider user instance should inherit cpu_speed of compute host
    Failure/Error: expect(vm.hardware.cpu_speed).to eq(@cpu_speed) if vm.name !='EmsRefreshSpec-Shelved'

    expected: 2800
    got: nil

    (compared using ==)

    # ./spec/models/manageiq/providers/openstack/cloud_manager/refresher_rhos_juno_spec.rb:103:in `block (4 levels) in <top (required)>'
    # ./spec/models/manageiq/providers/openstack/cloud_manager/refresher_rhos_juno_spec.rb:102:in `block (3 levels) in <top (required)>'

Additionally, I think you should add a test for this change. The trick is since the host is probably coming from a different provider (undercloud), you might not be able to do this in the basic refresh test. Maybe mock an undercloud provider with a host that matches one of the VMs in the VCR cassette?

@petrblaho
Copy link
Author

@blomquisg in fact it is connected - in tests we have IP addresses instead hostnames and fqdns so my change with split('.').first causes failing test. During test run these VMs are not matched with Hosts the same way as in BZ this solves and so the VMs do not have matching hardware association and so no cpu_speed.

I will enhance that matching code to probably match both split('.').first and full value.
Still investigating possible values there and implications.

Hosts are stored in db with only hostname part of hostname.domain which
we can see in OpenStack.
That caused issue with matching VMs with Hosts when Cloud OpenStack had
Infra OpenStack related to it.

Solves https://bugzilla.redhat.com/show_bug.cgi?id=1528162
@miq-bot
Copy link
Member

miq-bot commented Jan 5, 2018

Checked commit https://github.com/petrblaho/manageiq-providers-openstack/commit/e579e0d933103028f587c1b620d794979088de4e with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@petrblaho
Copy link
Author

I updated code with new 'compound' matching for both splitted and full string.

@aufi
Copy link
Member

aufi commented Jan 8, 2018

Looks good to me, merging!

Agree on test should be added later.

@aufi aufi merged commit a662f25 into ManageIQ:master Jan 8, 2018
@aufi aufi added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 8, 2018
simaishi pushed a commit that referenced this pull request Jan 8, 2018
…heck-in-inventory-parser-cloud-manager-bz1528162

Use only hypervisor hostname to match infra host with cloud vm
(cherry picked from commit a662f25)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1532347
@simaishi
Copy link
Contributor

simaishi commented Jan 8, 2018

Gaprindashvili backport details:

$ git log -1
commit 42bf9cedab8b408cbee1226e73521845606247c8
Author: Marek Aufart <[email protected]>
Date:   Mon Jan 8 09:59:44 2018 +0100

    Merge pull request #186 from petrblaho/add-hostname-domain-split-to-check-in-inventory-parser-cloud-manager-bz1528162
    
    Use only hypervisor hostname to match infra host with cloud vm
    (cherry picked from commit a662f258223fd79cf5de305ee9c00dc802e023c1)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1532347

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants