-
Notifications
You must be signed in to change notification settings - Fork 993
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?
Conversation
Issues: #36745 |
Can one of the admins verify this patch? |
2 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
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.
I worry this may have a serious performance impact so perhaps this should be opt in, or at least provide an opt out.
@stejskalleos could you take a look?
ok to test |
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.
Hi,
the PR is about Associating VMs when registering the host, but it also does some refactoring of associated_host
and associated_vm
methods, moving them to the compute_resource
, if I understand that correctly.
I don't think it should be a part of this PR. Refactoring is always tricky, and it requires a lot of testing across all the compute resources, like Azure and Google, to make sure that it's not breaking anything.
I suggest splitting the PR into two, plus fixing rubocop and unit tests
app/models/compute_resource.rb
Outdated
@@ -448,9 +460,13 @@ def nested_attributes_for(type, opts) | |||
end.compact | |||
end | |||
|
|||
def associate_by(name, attributes) | |||
def associate_by(name, attributes, host = nil) |
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.
How about creating a new method associate_by_host
?
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.
Hey @stejskalleos,
this is exactly what my is supposed to do. And thanks for your suggestion with the new associate_by_host
method. I adjusted my changes accordlingly and found out that this approach makes it all in all really simple.
IMO no refactoring is needed anymore. What do you think?
@@ -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 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?
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.
It's the same query as in associated_host
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.
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.
@@ -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 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 🤔
if @host.compute_resource | ||
associator = ComputeResourceHostAssociator.new(@host.compute_resource) | ||
associator.associate_host(@host) | ||
else |
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.
@@ -147,6 +147,10 @@ def associated_host(vm) | |||
associate_by("ip", [vm.public_ip_address, vm.private_ip_address]) | |||
end | |||
|
|||
def associated_vm(host) | |||
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 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?
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.
Is it the SAME uuid for host and vm?
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 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.
Upon host registration no VM is associated to the newly registered host. Therefore, no VM information is visible in the host details page (Legacy UI). IMO association of the VM should happen automatically upon registration.
With this PR I want to add some code to search for the VM that belongs to a certain host by the already present search criteria in the compute resources. If a corresponding VM is found the host will be associated to it.