Quota - Calculate quota values for active provisions. #15466

tinaafitz · 2017-06-28T15:21:33Z

Modified provision quota mixin to calculate active or inflight provisions.
The changes are meant only as a temporary measure for quota calculations.

Updated quota mixin to reflect changes in method and object names.
Added methods for tenant and group active provisions and for quota item calculations.
Added tests.

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

tinaafitz · 2017-06-28T15:21:52Z

@mkanoor Please review.

tinaafitz · 2017-06-28T15:23:27Z

@miq-bot add_label bug, fine/yes, assign @gmcculloug

miq-bot · 2017-06-28T15:24:05Z

@tinaafitz Cannot apply the following label because they are not recognized: assign @gmcculloug

mkanoor · 2017-06-29T18:20:14Z

app/models/mixins/miq_provision_quota_mixin.rb

+ def number_of_vms(request)
+ num_vms_for_request = request.get_option(:number_of_vms).to_i
+ if options[:nil_vm_id_only] == true && request.miq_request_tasks.length == num_vms_for_request
+ no_vm = request.miq_request_tasks.find_all { |p| p.destination_id.nil? && p.state != 'finished' }


@tinaafitz Is this a count or an array of objects?

@mkanoor It's an array of objects.

I believe request.miq_request_tasks is an ActiveRecord Collection, correct?
If so, prefer where over find_all.
request.miq_request_tasks.where(:destination_id => nil).where.not(:state => 'finished')

Wait... this method returns an integer or an array. Based on the name, I would expect it to return an Integer, so you need a .count on the suggestion above.

bdunne · 2017-08-01T20:49:38Z

app/models/mixins/miq_provision_quota_mixin.rb

+ prov_requests = quota_find_active_prov_request(options).select do |p|
+ prov_request_group == p.miq_request.requester.current_group
+ end
+ prov_requests


prov_requests here and on line 272 are not needed, the select is the last thing run in method, so the method will return that by default.

bdunne · 2017-08-01T21:08:12Z

app/models/mixins/miq_provision_quota_mixin.rb

- end
+ prov_request_group = miq_request.requester.current_group
+ prov_requests = quota_find_active_prov_request(options).select do |p|
+ prov_request_group == p.miq_request.requester.current_group


Do you need the .miq_request? Isn't p already an instance of MiqRequest?

bdunne · 2017-08-01T21:10:04Z

app/models/mixins/miq_provision_quota_mixin.rb

@@ -250,40 +261,88 @@ def quota_find_active_prov_request_by_owner(options)
 quota_find_active_prov_request(options).select { |p| email.casecmp(p.get_option(:owner_email).to_s.strip) == 0 }
 end

+ def quota_find_active_prov_request_by_user(options)
+ quota_find_active_prov_request(options).select do |p|


Why accept and forward options if they're ultimately ignored?

@bdunne The method needs to have the options argument because it's called by the quota_provision_stats method which calls all of the provision methods with options:
def quota_provision_stats(prov_method, options)
....
send(prov_method, options).each do |pr|

Why not do all of this in the database?

quota_find_active_prov_request(options).where(:userid => miq_request.requester.userid)

And tack on a .all if you need to.

bdunne · 2017-08-01T21:28:02Z

app/models/mixins/miq_provision_quota_mixin.rb

+ def number_of_vms(request)
+ num_vms_for_request = request.get_option(:number_of_vms).to_i
+ if options[:nil_vm_id_only] == true && request.miq_request_tasks.length == num_vms_for_request
+ no_vm = request.miq_request_tasks.find_all { |p| p.destination_id.nil? && p.state != 'finished' }


I believe request.miq_request_tasks is an ActiveRecord Collection, correct?
If so, prefer where over find_all.
request.miq_request_tasks.where(:destination_id => nil).where.not(:state => 'finished')

bdunne · 2017-08-01T21:30:59Z

app/models/mixins/miq_provision_quota_mixin.rb

+ def number_of_vms(request)
+ num_vms_for_request = request.get_option(:number_of_vms).to_i
+ if options[:nil_vm_id_only] == true && request.miq_request_tasks.length == num_vms_for_request
+ no_vm = request.miq_request_tasks.find_all { |p| p.destination_id.nil? && p.state != 'finished' }


Wait... this method returns an integer or an array. Based on the name, I would expect it to return an Integer, so you need a .count on the suggestion above.

bdunne · 2017-08-01T21:36:57Z

app/models/mixins/miq_provision_quota_mixin.rb

+
+ def memory(prov, cloud, vendor, flavor_obj = nil)
+ if cloud
+ return nil unless flavor_obj


flavor_obj.try(:memory) would one-line and remove the return from this side of the if

bdunne · 2017-08-01T21:41:56Z

app/models/mixins/miq_provision_quota_mixin.rb

+ flavor_obj.memory
+ else
+ memory = prov.kind_of?(MiqRequest) ? prov.get_option(:vm_memory).to_i : prov.miq_request.get_option(:vm_memory).to_i
+ memory.megabytes if %w(amazon openstack google).exclude?(vendor)


return nil if it's not one of those 3 vendors?
If that's the intent, then we should return before line 412 to avoid doing extra work.

@bdunne We return memory if it's not one of those 3 vendors.

Sorry, I meant it returns nil if it is one of those vendors.
It would read easier and be more efficient as:

return if %w(amazon openstack google).include?(vendor) request = prov.kind_of?(MiqRequest) ? prov : prov.miq_request request.get_option(:vm_memory).to_i.megabytes

bdunne · 2017-08-01T21:44:24Z

spec/models/miq_provision_request_spec.rb

+
+ stats = @pr.check_quota(:requests_by_owner)
+ expect(stats).to be_kind_of(Hash)
+ expect(stats[:class_name]).to eq("MiqProvisionRequest")


To reduce the number of expectations on the same object, you can:

expect(stats).to have_attributes( :class_name => "MiqProvisionRequest", :count => 2, ..... )

bdunne · 2017-08-01T21:44:52Z

spec/models/miq_provision_request_spec.rb

+ end
+
+ def create_request(user, vm_template, prov_options)
+ FactoryGirl.create(:miq_provision_request, :requester => user, :description => "request",


This alignment is awkward

bdunne · 2017-08-01T21:48:11Z

spec/models/service_template_provision_request_quota_spec.rb

+ end
+
+ def create_request(user, template, prov_options = {})
+ FactoryGirl.create(:service_template_provision_request, :requester => user, :description => "request",


Same awkward alignment

tinaafitz · 2017-08-02T17:25:42Z

spec/models/miq_provision_request_spec.rb

@@ -130,30 +130,18 @@
 expect(stats.fetch_path(:active, :class_name)).to eq("MiqProvision")
 end

- it "should return stats from quota methods" do


@bdunne This test wasn't necessary.

miq-bot · 2017-08-02T17:37:17Z

This pull request is not mergeable. Please rebase and repush.

methods. Added tests. https://bugzilla.redhat.com/show_bug.cgi?id=1456819

bdunne · 2017-08-02T19:47:10Z

app/models/mixins/miq_provision_quota_mixin.rb

@@ -250,40 +261,88 @@ def quota_find_active_prov_request_by_owner(options)
 quota_find_active_prov_request(options).select { |p| email.casecmp(p.get_option(:owner_email).to_s.strip) == 0 }
 end

+ def quota_find_active_prov_request_by_user(options)
+ quota_find_active_prov_request(options).select do |p|


Why not do all of this in the database?

quota_find_active_prov_request(options).where(:userid => miq_request.requester.userid)

And tack on a .all if you need to.

bdunne · 2017-08-02T19:48:50Z

app/models/mixins/miq_provision_quota_mixin.rb

+
+ def quota_find_active_prov_request_by_tenant(options)
+ quota_find_active_prov_request(options).select do |p|
+ miq_request.tenant == p.tenant


Same where comment here.

bdunne · 2017-08-02T20:01:48Z

app/models/mixins/miq_provision_quota_mixin.rb

+ end
+
+ def flavor(request)
+ Flavor.find_by(:id => request.get_option(:instance_type)) if cloud?(request)


You can simplify to using find since you're searching by id

bdunne · 2017-08-02T20:20:56Z

app/models/mixins/miq_provision_quota_mixin.rb

+ end
+
+ def memory(prov, cloud, vendor, flavor_obj = nil)
+ if cloud


Use guard clauses to prevent doing extra work.

return flavor_obj.try(:memory) if cloud return if %w(amazon openstack google).includes?(vendor) request = prov.kind_of?(MiqRequest) ? prov : prov.miq_request request.get_option(:vm_memory).to_i.megabytes

bdunne · 2017-08-02T20:28:26Z

spec/models/miq_provision_request_spec.rb

+ let(:request) { create_test_task(@vmware_user1, @vmware_template) }
+ let(:quota_method) { :active_provisions }
+ let(:counts_hash) do
+ { :count => 12, :memory => 8_589_938_688, :cpu => 32, :storage => 44.gigabytes }


You shouldn't have spaces inside the hash braces

bdunne · 2017-08-02T20:29:06Z

spec/models/miq_provision_request_spec.rb

+ context "active_provisions_by_user," do
+ let(:quota_method) { :active_provisions_by_user }
+ let(:counts_hash) do
+ { :count => 2, :memory => 2048, :cpu => 8, :storage => 20.gigabytes }


You shouldn't have spaces inside the hash braces

bdunne · 2017-08-02T20:30:36Z

spec/models/service_template_provision_request_quota_spec.rb

+ let(:load_queue) { queue(vmware_tasks) }
+ let(:request) { create_test_task(@vmware_user1, @vmware_template) }
+ let(:counts_hash) do
+ { :count => 6, :memory => 6.gigabytes, :cpu => 16, :storage => 3.gigabytes }


You shouldn't have spaces inside the hash braces

bdunne · 2017-08-02T20:30:44Z

spec/models/service_template_provision_request_quota_spec.rb

+ context "active_provisions_by_user," do
+ let(:quota_method) { :active_provisions_by_user }
+ let(:counts_hash) do
+ { :count => 3, :memory => 3.gigabytes, :cpu => 8, :storage => 1_610_612_736 }


You shouldn't have spaces inside the hash braces

bdunne · 2017-08-02T20:30:57Z

spec/models/service_template_provision_request_quota_spec.rb

+ let(:load_queue) { queue(google_tasks) }
+ let(:request) { create_test_task(@google_user1, @google_template) }
+ let(:counts_hash) do
+ { :count => 4, :memory => 4096, :cpu => 16, :storage => 40.gigabytes }


You shouldn't have spaces inside the hash braces

bdunne · 2017-08-02T20:31:03Z

spec/models/service_template_provision_request_quota_spec.rb

+ context "active_provisions_by_user," do
+ let(:quota_method) { :active_provisions_by_user }
+ let(:counts_hash) do
+ { :count => 2, :memory => 2048, :cpu => 8, :storage => 20.gigabytes }


You shouldn't have spaces inside the hash braces

bdunne · 2017-08-03T17:44:46Z

app/models/mixins/miq_provision_quota_mixin.rb

+ end
+
+ def number_of_cpus(prov, cloud, flavor_obj)
+ if cloud


Prefer a guard clause like return flavor_obj.try(:cpus) if cloud rather than if / else.

bdunne · 2017-08-03T17:48:01Z

app/models/mixins/miq_provision_quota_mixin.rb

+ return flavor_obj.try(:memory) if cloud
+ request = prov.kind_of?(MiqRequest) ? prov : prov.miq_request
+ memory = request.get_option(:vm_memory).to_i
+ memory.megabytes if %w(amazon openstack google).exclude?(vendor)


We seem to keep losing my comment that we should return if %w(amazon openstack google).exclude?(vendor) early.

tinaafitz · 2017-08-04T13:37:54Z

@mkanoor Please review.

gmcculloug · 2017-08-04T19:00:31Z

app/models/mixins/miq_provision_quota_mixin.rb

 :state => 'dequeue'
 ).pluck(:instance_id)

 prov_ids = []
 MiqQueue
 .where(:method_name => 'deliver', :state => %w(ready dequeue), :class_name => 'MiqAeEngine')
- .where("tracking_label like ?", '%miq_provision_%')
+ .where("task_id like ?", '%_provision_%')


I think you want to continue using the tracking_label. (See commit b609e1d from PR #15443)

cc @mkanoor

gmcculloug · 2017-08-15T12:40:00Z

app/models/mixins/miq_provision_quota_mixin.rb

- prov_req_owner = p.get_owner
- prov_req_owner && group.casecmp(prov_req_owner.ldap_group) == 0
- end
+ prov_request_group = miq_request.requester.current_group


Knowing that current_group change change in the db for the user after the request is created should we base this on miq_request.options[:requester_group]? (Note: This is the group name, not id)

gmcculloug · 2017-08-15T12:40:30Z

app/models/mixins/miq_provision_quota_mixin.rb

- end
+ prov_request_group = miq_request.requester.current_group
+ quota_find_active_prov_request(options).select do |r|
+ prov_request_group == r.requester.current_group


Same here for the current_group.

gmcculloug · 2017-08-15T12:51:34Z

app/models/mixins/miq_provision_quota_mixin.rb

+ end
+
+ def service_request?(request)
+ request.type == "ServiceTemplateProvisionRequest"


Suggest request.kind_of?(ServiceTemplateProvisionRequest).

This will future-proof the method if the ServiceTemplateProvisionRequest ever gets sub-classed. Because the name isn't already long enough. 😄

gmcculloug · 2017-08-15T22:03:11Z

@tinaafitz The tests are failing because of the miq_request.options[:requester_group] change but the options hash is not setup with that value.

miq-bot · 2017-08-16T19:47:46Z

Checked commits tinaafitz/manageiq@fa19836~...ad37766 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

Quota - Calculate quota values for active provisions. (cherry picked from commit 6d5cb11) https://bugzilla.redhat.com/show_bug.cgi?id=1489507

simaishi · 2017-09-07T18:07:12Z

Fine backport details:

$ git log -1
commit 9e33806d17e8fcb631741d19fecc37d6a8c94094
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Mon Aug 21 16:04:12 2017 -0400

    Merge pull request #15466 from tinaafitz/new_quota_requested
    
    Quota - Calculate quota values for active provisions.
    (cherry picked from commit 6d5cb119535f0ffe1b425893f831d633a5587030)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1489507

tinaafitz · 2017-10-05T17:23:59Z

@miq-bot add_label euwe/yes

simaishi · 2017-10-09T19:36:41Z

Backported to Euwe via #16141

Quota - Calculate quota values for active provisions. (cherry picked from commit 6d5cb11) https://bugzilla.redhat.com/show_bug.cgi?id=1489507

miq-bot added bug fine/yes labels Jun 28, 2017

gmcculloug requested a review from mkanoor June 28, 2017 16:01

gmcculloug self-assigned this Jun 28, 2017

mkanoor reviewed Jul 6, 2017

View reviewed changes

tinaafitz changed the title ~~Quota - Calculate quota values for active provisions.~~ [WIP]Quota - Calculate quota values for active provisions. Jul 12, 2017

miq-bot added the wip label Jul 12, 2017

tinaafitz force-pushed the new_quota_requested branch from c5a8e4a to 5ddc971 Compare July 31, 2017 21:41

tinaafitz changed the title ~~[WIP]Quota - Calculate quota values for active provisions.~~ Quota - Calculate quota values for active provisions. Aug 1, 2017

miq-bot removed the wip label Aug 1, 2017

bdunne requested changes Aug 1, 2017

View reviewed changes

tinaafitz commented Aug 2, 2017

View reviewed changes

miq-bot added the unmergeable label Aug 2, 2017

tinaafitz added 5 commits August 2, 2017 14:04

Modified active prov methods and added by_group and by_tenant active

fa19836

methods. Added tests. https://bugzilla.redhat.com/show_bug.cgi?id=1456819

Add ability to calculate in-flight Service item provisions.

d1999fb

Add ability to calculate in-flight Service Bundle provisions.

9040007

Fix rubocop issues.

b2081d7

Refactored test.

b2d7281

tinaafitz force-pushed the new_quota_requested branch from 792d42a to ec130e7 Compare August 2, 2017 18:10

miq-bot removed the unmergeable label Aug 2, 2017

bdunne requested changes Aug 2, 2017

View reviewed changes

tinaafitz force-pushed the new_quota_requested branch from a731f03 to 55111d5 Compare August 2, 2017 21:46

bdunne requested changes Aug 3, 2017

View reviewed changes

bdunne approved these changes Aug 4, 2017

View reviewed changes

gmcculloug reviewed Aug 4, 2017

View reviewed changes

tinaafitz force-pushed the new_quota_requested branch from ceafd63 to a7736f9 Compare August 11, 2017 17:44

tinaafitz closed this Aug 11, 2017

tinaafitz reopened this Aug 11, 2017

tinaafitz force-pushed the new_quota_requested branch from a7736f9 to 873340b Compare August 11, 2017 20:25

gmcculloug reviewed Aug 15, 2017

View reviewed changes

tinaafitz force-pushed the new_quota_requested branch from 873340b to eb8da72 Compare August 15, 2017 20:54

Use active by_owner methods, adjusting them for service requests.

ad37766

tinaafitz force-pushed the new_quota_requested branch from eb8da72 to ad37766 Compare August 16, 2017 19:45

bdunne merged commit 6d5cb11 into ManageIQ:master Aug 21, 2017

bdunne assigned bdunne and unassigned gmcculloug Aug 21, 2017

bdunne added the lifecycle/provisioning label Aug 21, 2017

bdunne added this to the Sprint 67 Ending Aug 21, 2017 milestone Aug 21, 2017

simaishi added the blocker label Sep 7, 2017

simaishi pushed a commit that referenced this pull request Sep 7, 2017

Merge pull request #15466 from tinaafitz/new_quota_requested

9e33806

Quota - Calculate quota values for active provisions. (cherry picked from commit 6d5cb11) https://bugzilla.redhat.com/show_bug.cgi?id=1489507

simaishi added fine/backported and removed fine/yes labels Sep 7, 2017

miq-bot added the euwe/yes label Oct 5, 2017

tinaafitz mentioned this pull request Oct 9, 2017

[EUWE] Back port quota active provision changes. #16141

Merged

simaishi added euwe/backported and removed euwe/yes labels Oct 9, 2017

Quota - Calculate quota values for active provisions. #15466

Quota - Calculate quota values for active provisions. #15466

Conversation

tinaafitz commented Jun 28, 2017

tinaafitz commented Jun 28, 2017

tinaafitz commented Jun 28, 2017

miq-bot commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinaafitz commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcculloug commented Aug 15, 2017

miq-bot commented Aug 16, 2017

simaishi commented Sep 7, 2017

tinaafitz commented Oct 5, 2017

simaishi commented Oct 9, 2017

bdunne Aug 1, 2017 •

edited

Loading