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.

Sign up for GitHub

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

Jump to bottom

add quick_stats support for RHEVM host #10505

Merged

blomquisg merged 1 commit into ManageIQ:master from borod108:rfe/rhevm_quick_stats

Nov 2, 2016

borod108 commented Aug 16, 2016 •

edited by miq-bot

Loading

Add host quickStats support for RHEVM providers so the auto provisioning of a VM will consider which host has more free memory when deciding on a host for the new VM.
(Until now this was only supported for VMWare)

see: https://bugzilla.redhat.com/show_bug.cgi?id=1323145

chessbyte added enhancement lifecycle/provisioning providers/rhevm labels

chessbyte assigned oourfali

borod108 force-pushed the rfe/rhevm_quick_stats branch 2 times, most recently from 6d63406 to 48fd252 Compare

August 16, 2016 13:56

Author

borod108 commented Aug 18, 2016

This change: https://bugzilla.redhat.com/show_bug.cgi?id=1331388 should be reverted before this will have any effect.

Author

borod108 commented Aug 22, 2016

@masayag can you please review?

borod108 force-pushed the rfe/rhevm_quick_stats branch 2 times, most recently from e528d51 to 791aed9 Compare

August 29, 2016 12:32

Contributor

masayag commented Aug 29, 2016

looks good to me

Author

borod108 commented Aug 30, 2016

@blomquisg, can this be merged?

blomquisg assigned blomquisg and unassigned oourfali

blomquisg reviewed

View reviewed changes

app/models/manageiq/providers/redhat/infra_manager.rb

    
                def host_quick_stats(host)

                  qs = {}

                  with_provider_connection(:version => 4) do |connection|

                    l = connection.system_service.hosts_service.host_service(host.uid_ems)

Member

blomquisg Sep 6, 2016 •

edited

Loading

@borod108 I think it's likely that the meaning of l is going to get lost. Can you change this to stats_list, or something similar?

Also, below, the l almost looks like the numeral 1, and I was hugely confused as to what 1.detect meant. 😄

Member

blomquisg commented Sep 6, 2016

@borod108 nice use of the Support Feature Mixin. Thanks for refactoring the hard coded vmware logic, as well 😄

blomquisg assigned borod108 and unassigned blomquisg

Member

blomquisg commented Sep 8, 2016

@borod108 just assign back to me when you've addressed my nitpick comments 😄

Member

miq-bot commented Sep 23, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

borod108 force-pushed the rfe/rhevm_quick_stats branch from 791aed9 to fd6698b Compare

October 5, 2016 12:15

Author

borod108 commented Oct 5, 2016

this is rebased over #11418

miq-bot added the dependencies label

Member

miq-bot commented Oct 5, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

borod108 force-pushed the rfe/rhevm_quick_stats branch from fd6698b to 32f1aab Compare

October 5, 2016 14:43

Member

miq-bot commented Oct 5, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

1 similar comment

Member

miq-bot commented Oct 5, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

borod108 force-pushed the rfe/rhevm_quick_stats branch from 32f1aab to 1d2df6b Compare

October 6, 2016 09:21

Member

miq-bot commented Oct 12, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

borod108 force-pushed the rfe/rhevm_quick_stats branch from 1d2df6b to 1900a4c Compare

October 13, 2016 07:01

durandom suggested changes

View reviewed changes

app/models/manageiq/providers/vmware/infra_manager/host.rb

    
            @@ -66,4 +66,8 @@ def reserve_next_available_vnc_port
          
                    port

                  end

                end

                def supports_quick_stats?

Member

durandom Oct 26, 2016

could you change this

borod108 force-pushed the rfe/rhevm_quick_stats branch from e4f91a1 to 502d80a Compare

October 27, 2016 10:05

masayag reviewed

View reviewed changes

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

    
            @@ -211,7 +211,7 @@ def api3_supported_features
          
                  end

                  def api4_supported_features

                    [:snapshots]

                    [:snapshots, :quick_stats]

Contributor

masayag Oct 27, 2016

Let's keep this list sorted, perhaps even have a single feature per line

Author

borod108 Oct 27, 2016

done


          add quick_stats support for RHEVM host

6789f44

borod108 force-pushed the rfe/rhevm_quick_stats branch from 502d80a to 6789f44 Compare

October 27, 2016 12:10

Member

miq-bot commented Oct 27, 2016

<github_pr_commenter_batch />Some comments on commit borod108@6789f44

spec/models/manageiq/providers/redhat/infra_manager/host_spec.rb

⚠️ - 15 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 7 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

Member

miq-bot commented Oct 27, 2016

Checked commit borod108@6789f44 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
8 files checked, 0 offenses detected
Everything looks good. 👍

Author

borod108 commented Oct 27, 2016

@durandom I made the changes you requested. can this be mereged
@miq-bot assign @durandom

miq-bot assigned durandom and unassigned blomquisg

Author

borod108 commented Oct 27, 2016

@miq-bot add-label euwe/yes

miq-bot added the euwe/yes label

durandom reviewed

View reviewed changes

Member

durandom left a comment

🏆 from me @borod108 nice

@agrare can you have a look, because vmware is also concerned? And merge if ok

Member

agrare commented Oct 27, 2016

This isn't really related to your PR but do we actually use vm_quick_stats anywhere? We collect summary.quickStats.* for hosts but filter it out for VMs. I'm not even sure it would work since def quick_stats is only defined in gems/pending/VMwareWebService/MiqVimHost.rb

Member

agrare commented Oct 27, 2016

The VMware part looks good, euwe/yes is supposed to be bugs only now... @blomquisg ?

oourfali commented Oct 27, 2016

@blomquisg this is a small and non risky one, as far as I see. what do you think?

masayag mentioned this pull request

Enable VM reconfigure disks for supported rhevm version #11971

Merged

Member

durandom commented Oct 31, 2016

@miq-bot assign blomquisg

@blomquisg please merge and have a quick look at the last 2 comments about euwe/yes of this

miq-bot assigned blomquisg and unassigned durandom

Member

blomquisg commented Nov 2, 2016

I'll merge it to master and let @chessbyte make the call on whether this gets backported to Euwe.

blomquisg merged commit e44dbd2 into ManageIQ:master

blomquisg added this to the Sprint 49 Ending Nov 14, 2016 milestone

chrispy1 commented Nov 7, 2016

@miq-bot add_label blocker

miq-bot added the blocker label

chessbyte pushed a commit that referenced this pull request


          Merge pull request #10505 from borod108/rfe/rhevm_quick_stats

27afd0f

add quick_stats support for RHEVM host
(cherry picked from commit e44dbd2)

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

Member

chessbyte commented Nov 7, 2016

Euwe Backport details:

$ git log -1
commit 27afd0f94632d9042a797fe6fb3c78b7c8a44cae
Author: Greg Blomquist <blomquisg@gmail.com>
Date:   Wed Nov 2 17:44:05 2016 -0400

    Merge pull request #10505 from borod108/rfe/rhevm_quick_stats

    add quick_stats support for RHEVM host
    (cherry picked from commit e44dbd2693a4cd2a19a5aa3a87c05d6320ce9f4a)

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

borod108 added euwe/backported and removed euwe/yes labels

Fryguy added the providers/infrastructure label

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

Labels

blocker dependencies enhancement euwe/backported lifecycle/provisioning providers/infrastructure