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

JobProxyDispatcher should use all container image classes #15519

Merged
merged 3 commits into from Jul 26, 2017
Merged

JobProxyDispatcher should use all container image classes #15519

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c8a16c6
job proxy dispatcher should use all container image classes
Jul 6, 2017
cccdfe4
use Job class STI
Jul 13, 2017
3bbab0d
using all EXISTING container image classes
Jul 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 9 additions & 6 deletions app/models/job_proxy_dispatcher.rb
View file
Open in desktop
@@ -1,5 +1,6 @@
class JobProxyDispatcher
include Vmdb::Logging

def self.dispatch
new.dispatch
end
Expand Down Expand Up @@ -93,6 +94,10 @@ def dispatch
_log.info "Complete - Timings: #{t.inspect}"
end

def container_image_classes
ContainerImage.descendants.append(ContainerImage)
Copy link
Member

Choose a reason for hiding this comment

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

I know the current pattern does this but it feels like the dispatcher shouldn't have this knowledge. Additionally, it would cause us to load each of these classes and any requires they do in the schedule worker... a process that probably shouldn't load container dependencies. I'm not sure of a better solution but if you have an idea, do let us know.

Copy link

Choose a reason for hiding this comment

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

  1. We can check for jobs with target != VmOrTemplate.
    It is a hack, maybe if we add a comment that states that if we ever get a target that isn't one of the two we should rethink.
  2. Maybe we can look at job.type instead

Copy link
Member

Choose a reason for hiding this comment

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

hi @moolitayer, thanks... One thing that still confuses me is it feels like the vm scans are really VmOrTemplateScanJob things (hiding as the Job class) and the container image scans are really ContainerImageScanJob things (hiding as a list of descendent classes + self here)... I don't know why we didn't use single table inheritance on this table in the past but I'm wondering if we should do it now. Perhaps fix this without a migration like you are here for any backports... but then, fix it right on master with STI. What do you think? cc @Fryguy

The nice thing is then you can do ContainerImageScanJob.where... or VmOrTemplateScanJob.where as you would expect. This might break the UI in many places where they're expecting Job classes by name, but it feels like we're doing it the hard way and fixing this might help maintenance in the future.

Copy link
Member

Choose a reason for hiding this comment

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

@gtanzillo ^

Copy link
Member

Choose a reason for hiding this comment

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

I don't know why we didn't use single table inheritance on this table in the past but I'm wondering if we should do it now.

We already do have STI on the jobs table.

end

def dispatch_container_scan_jobs
jobs_by_ems, = Benchmark.realtime_block(:pending_container_jobs) { pending_container_jobs }
Benchmark.current_realtime[:container_jobs_to_dispatch_count] = jobs_by_ems.values.reduce(0) { |m, o| m + o.length }
Expand Down Expand Up @@ -193,12 +198,11 @@ def self.waiting?
end

def pending_jobs(target_class = VmOrTemplate)
class_name = target_class.base_class.name
Copy link

Choose a reason for hiding this comment

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

any idea what was the result of this line?

Copy link
Author

Choose a reason for hiding this comment

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

For both ContainerImage and VmOrTemplate the result was their class name ("ContainerImage", "VmOrTemplate" as strings).
#15519 (comment)

@zone = MiqServer.my_zone
Job.order(:id)
.where(:state => "waiting_to_start")
.where(:dispatch_status => "pending")
.where(:target_class => class_name)
.where(:target_class => target_class)
.where("zone is null or zone = ?", @zone)
.where("sync_key is NULL or
sync_key not in (
Expand All @@ -210,7 +214,7 @@ def pending_jobs(target_class = VmOrTemplate)
end

def pending_container_jobs
pending_jobs(ContainerImage).each_with_object(Hash.new { |h, k| h[k] = [] }) do |job, h|
pending_jobs(container_image_classes).each_with_object(Hash.new { |h, k| h[k] = [] }) do |job, h|
h[job.options[:ems_id]] << job
end
end
Expand Down Expand Up @@ -254,9 +258,8 @@ def busy_proxies
end

def active_scans_by_zone(target_class, count = true)
class_name = target_class.base_class.name
actives = Hash.new(0)
jobs = Job.where(:zone => @zone, :dispatch_status => "active", :target_class => class_name)
jobs = Job.where(:zone => @zone, :dispatch_status => "active", :target_class => target_class)
.where.not(:state => "finished")
actives[@zone] = count ? jobs.count : jobs
actives
Expand All @@ -271,7 +274,7 @@ def active_container_scans_by_zone_and_ems
memo = Hash.new do |h, k|
h[k] = Hash.new(0)
end
active_scans_by_zone(ContainerImage, false)[@zone].each do |job|
active_scans_by_zone(container_image_classes, false)[@zone].each do |job|
memo[@zone][job.options[:ems_id]] += 1
end
memo
Expand Down
17 changes: 9 additions & 8 deletions spec/models/job_proxy_dispatcher_spec.rb
View file
Open in desktop
Expand Up @@ -174,6 +174,7 @@
end

context "with container and vms jobs" do
let (:container_image_classes) { ContainerImage.descendants.collect(&:name).append('ContainerImage') }
Copy link

Choose a reason for hiding this comment

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

please test for a list as well

Copy link
Author

Choose a reason for hiding this comment

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

👍 here: https://github.com/ManageIQ/manageiq/pull/15519/files#diff-b36aacfb241b305bc73f3a934af1ea53R195

before(:each) do
@jobs = (@vms + @repo_vms).collect(&:raw_scan)
User.current_user = FactoryGirl.create(:user)
Expand All @@ -191,10 +192,10 @@
end

it "returns only container images jobs when requested" do
jobs = dispatcher.pending_jobs(ContainerImage)
jobs = dispatcher.pending_jobs(dispatcher.container_image_classes)
expect(jobs.count).to eq(@container_images.count)
jobs.each do |x|
expect(x.target_class).to eq 'ContainerImage'
expect(container_image_classes).to include x.target_class
end
expect(jobs.count).to be > 0 # in case something unexpected goes wrong
end
Expand All @@ -212,7 +213,7 @@

describe "#active_container_scans_by_zone_and_ems" do
it "returns active container acans for zone" do
job = @jobs.find { |j| j.target_class == ContainerImage.name }
job = @jobs.find { |j| container_image_classes.include?(j.target_class) }
job.update(:dispatch_status => "active")
provider = ExtManagementSystem.find(job.options[:ems_id])
dispatcher.instance_variable_set(:@zone, MiqServer.my_zone) # memoized during pending_jobs call
Expand All @@ -226,29 +227,29 @@
it "dispatches jobs until reaching limit" do
stub_settings(:container_scanning => {:concurrent_per_ems => 1})
dispatcher.dispatch_container_scan_jobs
expect(Job.where(:target_class => ContainerImage, :dispatch_status => "pending").count).to eq(1)
expect(Job.where(:target_class => container_image_classes, :dispatch_status => "pending").count).to eq(1)
# 1 per ems, one ems has 1 job and the other 2
end

it "does not dispach if limit is already reached" do
stub_settings(:container_scanning => {:concurrent_per_ems => 1})
dispatcher.dispatch_container_scan_jobs
expect(Job.where(:target_class => ContainerImage, :dispatch_status => "pending").count).to eq(1)
expect(Job.where(:target_class => container_image_classes, :dispatch_status => "pending").count).to eq(1)
dispatcher.dispatch_container_scan_jobs
expect(Job.where(:target_class => ContainerImage, :dispatch_status => "pending").count).to eq(1)
expect(Job.where(:target_class => container_image_classes, :dispatch_status => "pending").count).to eq(1)
end

it "does not apply limit when concurrent_per_ems is 0" do
stub_settings(:container_scanning => {:concurrent_per_ems => 0})
dispatcher.dispatch_container_scan_jobs
expect(Job.where(:target_class => ContainerImage, :dispatch_status => "pending").count).to eq(0)
expect(Job.where(:target_class => container_image_classes, :dispatch_status => "pending").count).to eq(0)
# 1 per ems, one ems has 1 job and the other 2
end

it "does not apply limit when concurrent_per_ems is -1" do
stub_settings(:container_scanning => {:concurrent_per_ems => -1})
dispatcher.dispatch_container_scan_jobs
expect(Job.where(:target_class => ContainerImage, :dispatch_status => "pending").count).to eq(0)
expect(Job.where(:target_class => container_image_classes, :dispatch_status => "pending").count).to eq(0)
# 1 per ems, one ems has 1 job and the other 2
end
end
Expand Down
9 changes: 6 additions & 3 deletions spec/support/job_proxy_dispatcher_helper.rb
View file
Open in desktop
Expand Up @@ -48,10 +48,13 @@ def build_entities(options = {})

container_providers = []
options[:container_providers].each_with_index do |images_count, i|
ems_kubernetes = FactoryGirl.create(:ems_kubernetes, :name => "test_container_provider_#{i}")
container_providers << ems_kubernetes
ems_openshift = FactoryGirl.create(:ems_openshift, :name => "test_container_provider_#{i}")
container_providers << ems_openshift
images_count.times do |idx|
FactoryGirl.create(:container_image, :name => "test_container_images_#{idx}", :ems_id => ems_kubernetes.id)
FactoryGirl.create(:container_image,
:name => "test_container_images_#{idx}",
:ems_id => ems_openshift.id,
:type => 'ManageIQ::Providers::Openshift::ContainerManager::ContainerImage')
end
end
return hosts, proxies, storages, vms, repo_vms, container_providers
Expand Down