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

Fix cached query for total unregistered vms #16577

Merged

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Dec 1, 2017

Storage#total_unregistered_vms and Storage#unregistered_vms are using different queries.

Query here is not same as it is here (method registered? on VM: Vm#registered? )

@kbrock can you please review? (you touched this code last time )

🎁 @ZitaNemeckova

@miq-bot add_label bug, core

@miq-bot assign @gtanzillo

@miq-bot
Copy link
Member

miq-bot commented Dec 1, 2017

@lpichler Cannot apply the following label because they are not recognized: gapridashivili/yes

@lpichler
Copy link
Contributor Author

lpichler commented Dec 1, 2017

@miq-bot add_label gaprindashivili/yes

@miq-bot
Copy link
Member

miq-bot commented Dec 1, 2017

@lpichler Cannot apply the following label because they are not recognized: gaprindashivili/yes

@ZitaNemeckova
Copy link
Contributor

@miq-bot add_label gaprindashvili/yes

@miq-bot
Copy link
Member

miq-bot commented Dec 2, 2017

@lpichler Cannot apply the following label because they are not recognized: gaprindashivili/yes

@kbrock
Copy link
Member

kbrock commented Dec 4, 2017

Good eye.

  1. I even wonder if registered? is right
  2. Do we mean to be running from the Vm Or should we be pointing to VmOrTemplate? If we want just Vm, then we know template? == false.

The logic: Templates must have an EMS, but all others just need a host.
So if you are looking at VMs, then you only need to ask if a host_id is present.

class VmOrTemplate
  def registered?
    return false if self.template? && ext_management_system_id.nil?
    host_id.present?
  end
end

If we are querying Vm then we know that template? == false.
So the clause becomes:

class VmOrTemplate
  def registered?
    host_id.present?
  end
end

So unregistered becomes:

cache_with_timeout(:unregistered_vm_counts_by_storage_id, 15.seconds) do
  Vm.not(:host_id => nil)
  .group("storage_id").pluck("storage_id, COUNT(*) AS vm_count")
  .each_with_object(Hash.new(0)) { |(storage_id, count), h| h[storage_id] = count.to_i }
end

Also fun. (sorry) Think the pluck can just be a count:

cache_with_timeout(:unregistered_vm_counts_by_storage_id, 15.seconds) do
  Vm.not(:host_id => nil).group("storage_id").count
  .each_with_object(Hash.new(0)) { |(storage_id, count), h| h[storage_id] = count.to_i }
end

@kbrock kbrock self-requested a review December 4, 2017 14:48
we have definition of registered:

class VmOrTemplate
  def registered?
    return false if self.template? && ext_management_system_id.nil?
    host_id.present?
  end
end

this query is using only Vm, we can remove part
template = true and according to the rule upper
we are not taking into account ems_id when it is Vm.

so we can remove also part vms.ems_id IS NULL
When we are using this method, we always going thru existing Storage object
@lpichler
Copy link
Contributor Author

lpichler commented Dec 5, 2017

yes, that's right, awesome, thanks ! I filtered out also vms without storages because to use this method we need always existing storage - so it was useless.

@miq-bot
Copy link
Member

miq-bot commented Dec 5, 2017

Checked commits lpichler/manageiq@aef94e9~...9863192 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@kbrock kbrock added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 5, 2017
@kbrock kbrock merged commit 3ebe6cc into ManageIQ:master Dec 5, 2017
@kbrock kbrock assigned kbrock and unassigned gtanzillo Dec 5, 2017
simaishi pushed a commit that referenced this pull request Dec 11, 2017
…ered_vms

Fix cached query for total unregistered vms
(cherry picked from commit 3ebe6cc)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 38b2460ae95c62108988bc6453294f1e18d39e94
Author: Keenan Brock <[email protected]>
Date:   Tue Dec 5 13:25:21 2017 -0500

    Merge pull request #16577 from lpichler/fix_cached_query_total_registered_vms
    
    Fix cached query for total unregistered vms
    (cherry picked from commit 3ebe6ccd94262f3d05a43a05aded8254622bfd09)

@lpichler lpichler deleted the fix_cached_query_total_registered_vms branch December 12, 2017 10:18
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