-
Notifications
You must be signed in to change notification settings - Fork 990
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
base: develop
Are you sure you want to change the base?
Changes from all commits
25127a8
8d9869d
6bdd117
8dbfff0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it the SAME uuid for host and vm? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Above in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the same query as in associated_host There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment.
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.