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

Fixes #36745 - VM is not associated upon host registration #9833

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions app/controllers/api/v2/registration_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ def host
ActiveRecord::Base.transaction do
find_host
prepare_host
associate_vm
setup_host_params
host_setup_extension
@template = @host.initial_configuration_template
Expand Down Expand Up @@ -121,6 +122,19 @@ def prepare_host
@host.save!
end

def associate_vm
if @host.compute_resource
associator = ComputeResourceHostAssociator.new(@host.compute_resource)
associator.associate_host(@host)
else
Copy link
Contributor

Choose a reason for hiding this comment

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

The compute resource is set from hostgroup. We should not loop throw all available compute resources.

ComputeResource.all.each do |compute_resource|
associator = ComputeResourceHostAssociator.new(compute_resource)
associator.associate_host(@host)
break if @host.uuid
end
end
end

# Extension point for plugins
def host_setup_extension
end
Expand Down
13 changes: 13 additions & 0 deletions app/models/compute_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,19 @@ def associate_by(name, attributes)
first
end

def associated_vm(host)
vms(:eager_loading => true).find { |vm| associate_by_host("mac", vm.interfaces.map(&:mac), host) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how this will behave - do we need/want eager loading here? I understand that we might not want to have a query for every single vm 🤔

end

def associate_by_host(name, attributes, host)
attributes = Array.wrap(attributes).map { |mac| Net::Validations.normalize_mac(mac) } if name == 'mac'
Host.authorized(:view_hosts, Host).where(:id => host.id).joins(:primary_interface).
where(:nics => {:primary => true}).
where(ActiveRecord::Base.sanitize_sql("nics.#{name}") => attributes).
readonly(false).
first
end

private

def set_vm_volumes_attributes(vm, vm_attrs)
Expand Down
4 changes: 4 additions & 0 deletions app/models/compute_resources/foreman/model/ec2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,10 @@ def associated_host(vm)
associate_by("ip", [vm.public_ip_address, vm.private_ip_address])
end

def associated_vm(host)
sbernhard marked this conversation as resolved.
Show resolved Hide resolved
vms(:eager_loading => true).find { |vm| associate_by_host("ip", [vm.public_ip_address, vm.private_ip_address], host) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Each VM has a "uuid". The host does normally also has a uuid which is e.g. sent during host registration from sub-man to foreman. Can these 2 uuids used to match foreman host with the associated compute resource -> vm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it the SAME uuid for host and vm?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that the host does NOT have a uuid after it is registered. It gets the uuid after it is associated to a VM - this is exactly the step that needs to be done for this PR.

end

def user_data_supported?
true
end
Expand Down
4 changes: 4 additions & 0 deletions app/models/compute_resources/foreman/model/openstack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,10 @@ def associated_host(vm)
associate_by("ip", [vm.floating_ip_address, vm.private_ip_address].compact)
end

def associated_vm(host)
vms(:eager_loading => true).find { |vm| associate_by_host("ip", [vm.floating_ip_address, vm.private_ip_address].compact, host) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Above in app/models/compute_resources/foreman/model/ec2.rb line 151, we have the same line without compact keyword. Is there a particular reason for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same query as in associated_host

Copy link
Contributor

@nadjaheitmann nadjaheitmann Oct 16, 2024

Choose a reason for hiding this comment

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

What I found so far is that compute resources (at least in vmware) use the InstanceUuid as uuid internally which we don't get as a host fact as far as I could see. What we get from vmware is the uuid and not even always and this is dependent on the fact source as well.

end

def flavor_name(flavor_ref)
client.flavors.get(flavor_ref).try(:name)
end
Expand Down
11 changes: 11 additions & 0 deletions app/services/compute_resource_host_associator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@ def associate_hosts(vms = compute_resource.vms(:eager_loading => true))
end
end

def associate_host(host)
vm = compute_resource.associated_vm(host)
if vm.present?
host.associate!(compute_resource, vm)
@hosts << host
end
rescue StandardError => e
@fail_count += 1
Foreman::Logging.exception("Could not associate host #{host}", e)
end

private

def associate_vm(vm)
Expand Down
Loading