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.

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

Change the way active_provisions are calculated. #16831

Merged
merged 1 commit into from
Jan 19, 2018
Merged

Change the way active_provisions are calculated. #16831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 7 additions & 19 deletions app/models/mixins/miq_provision_quota_mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,25 +275,13 @@ def quota_find_active_prov_request_by_tenant(options)
end

def quota_find_active_prov_request(_options)
prov_req_ids = MiqQueue.where(
:class_name => %w(MiqProvisionRequest ServiceTemplateProvisionRequest),
:method_name => 'create_request_tasks',
:state => 'dequeue'
).pluck(:instance_id)

prov_ids = []
MiqQueue
.where(:method_name => 'deliver', :state => %w(ready dequeue), :class_name => 'MiqAeEngine')
.where("tracking_label like ?", '%_provision_%')
.each do |q|
if q.args
args = q.args.first
prov_ids << args[:object_id] if args[:object_type].include?('Provision') && !args[:object_id].blank?
end
end
prov_req_ids += MiqRequestTask.where(:id => prov_ids).pluck("miq_request_id")

MiqRequest.where(:id => prov_req_ids.compact.uniq)
MiqRequest.where(
:approval_state => 'approved',
:type => %w(MiqProvisionRequest ServiceTemplateProvisionRequest),
:request_state => %w(active queued pending),
:status => 'Ok',
:process => true
).where.not(:id => id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tinaafitz This is a much clearer solution and almost completely gets the quota logic away from doing lookups in MiqQueue. (One call left in this file)

My one concern is the possibility for false-positive MiqRequest instances being returned. We had that issue before, but it is slightly higher now because in the past clearing a queue would have removed the request from being returned from this method. Now clearing the queue or some other error could leave the request in what looks like a valid state.

Wondering if it makes sense to log a count and list of IDs that are being returned here. Or maybe we create a small script in the tools directory to help users identify what MiqRequest objects are in play for quota.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is a much clearer solution than the previous MiqQueue lookup logic. Your false-positive concern is valid. While I was testing this changes, I did exactly that and added logging for the count and request ID's being included in the active counts. I'm in favor of the tools/script approach to help users identify the MiqRequest objects in play for quota.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A major difference in this approach is that requests will be more likely to be denied based on the active requests, whereas before, the requests would be much more likely to be approved and cause quota to go negative.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sound good. Please open a git issue for creating that tools script with a reference back to this PR. I think that is a task we can assign to @billfitzgerald0120.

end

def vm_quota_values(pr, result)
Expand Down
82 changes: 20 additions & 62 deletions spec/models/miq_provision_request_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,49 +142,20 @@
end

context "for cloud and infra providers," do
def create_task(user, request)
FactoryGirl.create(:miq_request_task, :miq_request => request, :miq_request_id => request.id, :type => 'MiqProvision', :description => "task", :tenant => user.current_tenant)
end

def create_request(user, vm_template, prov_options)
FactoryGirl.create(:miq_provision_request, :requester => user,
:description => "request",
:tenant => user.current_tenant,
:source => vm_template,
:src_vm_id => vm_template.id,
:options => prov_options.merge(:owner_email => user.email, :requester_group => user.miq_groups.first.description))
end

def request_queue_entry(request)
FactoryGirl.create(:miq_queue,
:state => MiqQueue::STATE_DEQUEUE,
:instance_id => request.id,
:class_name => 'MiqProvisionRequest',
:method_name => 'create_request_tasks')
end

def task_queue_entry(task)
FactoryGirl.create(:miq_queue,
:state => MiqQueue::STATE_DEQUEUE,
:args => [{:object_type => "Provision", :object_id => task.id}],
:tracking_label => 'miq_provision_task',
:class_name => 'MiqAeEngine',
:method_name => 'deliver')
FactoryGirl.create(:miq_provision_request, :requester => user,
:description => "request",
:tenant => user.current_tenant,
:source => vm_template,
:status => 'Ok',
:process => true,
:request_state => 'active',
:approval_state => 'approved',
:src_vm_id => vm_template.id,
:options => prov_options.merge(:owner_email => user.email, :requester_group => user.miq_groups.first.description))
end

def create_test_task(user, template)
request = create_request(user, template, {})
create_task(user, request)
request
end

def queue(requests)
requests.each do |request|
request.miq_request_tasks.empty? ? request_queue_entry(request) : task_queue_entry(request.miq_request_tasks.first)
end
end

let(:vmware_tasks) do
let(:create_requests) do
ems = FactoryGirl.create(:ems_vmware)
vmware_tenant = FactoryGirl.create(:tenant)
group = FactoryGirl.create(:miq_group, :tenant => vmware_tenant)
Expand All @@ -195,16 +166,10 @@ def queue(requests)
:ext_management_system => ems,
:hardware => hardware)
prov_options = {:number_of_vms => [2, '2'], :vm_memory => [1024, '1024'], :number_of_cpus => [2, '2']}
requests = []
2.times { requests << create_request(@vmware_user1, @vmware_template, prov_options) }
create_task(@vmware_user1, requests.first)
2.times { create_request(@vmware_user1, @vmware_template, prov_options) }

2.times { requests << create_request(@vmware_user2, @vmware_template, prov_options) }
create_task(@vmware_user2, requests.last)
requests
end
2.times { create_request(@vmware_user2, @vmware_template, prov_options) }

let(:google_tasks) do
ems = FactoryGirl.create(:ems_google_with_authentication,
:availability_zones => [FactoryGirl.create(:availability_zone_google)])
google_tenant = FactoryGirl.create(:tenant)
Expand All @@ -216,26 +181,21 @@ def queue(requests)
:cpus => 4, :cpu_cores => 1, :memory => 1024)
prov_options = {:number_of_vms => 1, :src_vm_id => vm_template.id, :boot_disk_size => ["10.GB", "10 GB"],
:placement_auto => [true, 1], :instance_type => [flavor.id, flavor.name]}
requests = []
2.times { requests << create_request(@google_user1, @google_template, prov_options) }
create_task(@google_user1, requests.first)
2.times { create_request(@google_user1, @google_template, prov_options) }

2.times { requests << create_request(@google_user2, @google_template, prov_options) }
create_task(@google_user2, requests.last)
requests
2.times { create_request(@google_user2, @google_template, prov_options) }
end

shared_examples_for "check_quota" do
it "check" do
load_queue
create_requests
stats = request.check_quota(quota_method)
expect(stats).to include(counts_hash)
end
end

context "active_provisions," do
let(:load_queue) { queue(vmware_tasks | google_tasks) }
let(:request) { create_test_task(@vmware_user1, @vmware_template) }
let(:request) { create_request(@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}
Expand All @@ -244,8 +204,7 @@ def queue(requests)
end

context "infra," do
let(:load_queue) { queue(vmware_tasks | google_tasks) }
let(:request) { create_test_task(@vmware_user1, @vmware_template) }
let(:request) { create_request(@vmware_user1, @vmware_template, {}) }
let(:counts_hash) do
{:count => 8, :memory => 8.gigabytes, :cpu => 16, :storage => 4.gigabytes}
end
Expand All @@ -268,16 +227,15 @@ def queue(requests)
it_behaves_like "check_quota"

it "fails without owner_email option" do
load_queue
create_requests
request.options = {}
expect { request.check_quota(quota_method) }.to raise_error(NoMethodError)
end
end
end

context "cloud," do
let(:load_queue) { queue(vmware_tasks | google_tasks) }
let(:request) { create_test_task(@google_user1, @google_template) }
let(:request) { create_request(@google_user1, @google_template, {}) }
let(:counts_hash) do
{:count => 4, :memory => 4096, :cpu => 16, :storage => 40.gigabytes}
end
Expand Down
55 changes: 11 additions & 44 deletions spec/models/service_template_provision_request_quota_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@
let(:admin) { FactoryGirl.create(:user_admin) }
context "quota methods" do
context "for cloud and infra providers," do
def create_task(user, request)
FactoryGirl.create(:service_template_provision_task, :miq_request => request, :miq_request_id => request.id, :type => 'ServiceTemplateProvisionTask', :description => "task", :tenant => user.current_tenant)
end

def create_request(user, template, prov_options = {})
FactoryGirl.create(:service_template_provision_request, :requester => user,
:description => "request",
Expand All @@ -19,33 +15,8 @@ def create_request(user, template, prov_options = {})
:options => prov_options.merge(:owner_email => user.email))
end

def request_queue_entry(request)
FactoryGirl.create(:miq_queue,
:state => MiqQueue::STATE_DEQUEUE,
:instance_id => request.id,
:class_name => 'ServiceTemplateProvisionRequest',
:method_name => 'create_request_tasks')
end

def task_queue_entry(task)
FactoryGirl.create(:miq_queue,
:state => MiqQueue::STATE_DEQUEUE,
:args => [{:object_type => "ServiceTemplateProvisionRequest", :object_id => task.id}],
:tracking_label => 'service_template_provision_task',
:class_name => 'MiqAeEngine',
:method_name => 'deliver')
end

def create_test_task(user, service_template)
request = create_request(user, service_template)
create_task(user, request)
request
end

def queue(requests)
requests.each do |request|
request.miq_request_tasks.empty? ? request_queue_entry(request) : task_queue_entry(request.miq_request_tasks.first)
end
def create_test_request(user, service_template)
create_request(user, service_template)
end

def create_service_bundle(user, items, options = {})
Expand All @@ -59,14 +30,14 @@ def create_service_bundle(user, items, options = {})

shared_examples_for "check_quota" do
it "check" do
load_queue
load_requests
stats = request.check_quota(quota_method)
expect(stats).to include(counts_hash)
end
end

context "infra," do
let(:vmware_tasks) do
let(:vmware_requests) do
ems = FactoryGirl.create(:ems_vmware)
group = FactoryGirl.create(:miq_group, :tenant => FactoryGirl.create(:tenant))
@vmware_user1 = FactoryGirl.create(:user_with_email, :miq_groups => [group])
Expand All @@ -83,20 +54,18 @@ def create_service_bundle(user, items, options = {})
requests << build_vmware_service_item

requests << create_service_bundle(@user, [@vmware_template], @vmware_prov_options)
create_task(@user, requests.last)

@user = @vmware_user2
requests << build_vmware_service_item
create_task(@user, requests.last)

requests << create_service_bundle(@user, [@vmware_template], @vmware_prov_options)

requests.each { |r| r.update_attributes(:tenant_id => @user.current_tenant.id) }
requests
end

let(:load_queue) { queue(vmware_tasks) }
let(:request) { create_test_task(@vmware_user1, @vmware_template) }
let(:load_requests) { vmware_requests }
let(:request) { create_test_request(@vmware_user1, @vmware_template) }
let(:counts_hash) do
{:count => 6, :memory => 6.gigabytes, :cpu => 16, :storage => 3.gigabytes}
end
Expand All @@ -106,7 +75,7 @@ def create_service_bundle(user, items, options = {})
it_behaves_like "check_quota"

it "invalid service_template does not raise error" do
requests = load_queue
requests = load_requests
requests.first.update_attributes(:service_template => nil)
expect { request.check_quota(quota_method) }.not_to raise_error
end
Expand All @@ -125,7 +94,7 @@ def create_service_bundle(user, items, options = {})
it_behaves_like "check_quota"

it "fails without requester.email" do
load_queue
load_requests
@vmware_user1.update_attributes(:email => nil)
expect { request.check_quota(quota_method) }.to raise_error(NoMethodError)
end
Expand All @@ -142,7 +111,7 @@ def build_google_service_item
@service_request = build_service_template_request("google_service_item", @user, :dialog => {"test" => "dialog"})
end

let(:google_tasks) do
let(:google_requests) do
ems = FactoryGirl.create(:ems_google_with_authentication,
:availability_zones => [FactoryGirl.create(:availability_zone_google)])
group = FactoryGirl.create(:miq_group, :tenant => FactoryGirl.create(:tenant))
Expand All @@ -162,20 +131,18 @@ def build_google_service_item
requests << build_google_service_item

requests << create_service_bundle(@user, [@google_template], @google_prov_options)
create_task(@user, requests.last)

@user = @google_user2
requests << build_google_service_item
create_task(@user, requests.last)

requests << create_service_bundle(@user, [@google_template], @google_prov_options)

requests.each { |r| r.update_attributes(:tenant_id => @user.current_tenant.id) }
requests
end

let(:load_queue) { queue(google_tasks) }
let(:request) { create_test_task(@google_user1, @google_template) }
let(:load_requests) { google_requests }
let(:request) { create_test_request(@google_user1, @google_template) }
let(:counts_hash) do
{:count => 4, :memory => 4096, :cpu => 16, :storage => 40.gigabytes}
end
Expand Down
3 changes: 3 additions & 0 deletions spec/support/service_template_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ def build_service_template_request(root_st_name, user, dialog_options = {})
:type => 'ServiceTemplateProvisionRequest',
:request_type => 'clone_to_service',
:approval_state => 'approved',
:status => 'Ok',
:process => true,
:request_state => 'active',
:source_id => root.id,
:requester => user,
:options => options)
Expand Down