Merge retirement checks #15645
Merge retirement checks #15645
Conversation
By default we run retirement_check for Vms, Services, LoadBalancers and OrchestrationStacks every 10 minutes. Most of the time, retirement_check results into no-op. However, it has fixed costs attached. - it inserts, selects updates, and deletes from MiqQueue - it locks miq_queue_lock - it synces log messages (they grow) - it takes worker's time to process Every 10 minutes. Moreover, the code base is very similar -> no point of having it so many times. Every method defined increases our footprint. Every penny counts. Even in cases, your set-up uses retirement dates heavily, each run will go through really quickly -> so there is not reason for separation. The work is already too small, to divide into multiple jobs.
| FactoryGirl.create(:vm) | ||
| FactoryGirl.create(:service, :retires_on => Time.zone.today + 1.day) | ||
| FactoryGirl.create(:service, :retired => true) | ||
| allow(MiqServer).to receive(:my_server).and_return(my_server) |
| let(:my_server) { FactoryGirl.create(:miq_server, :zone => zone) } | ||
|
|
||
| it "check runs" do | ||
| allow(MiqServer).to receive(:my_server).and_return(my_server) |
There was a problem hiding this comment.
Why allow rather than expect?
When I read rspec, my brain translates:
allow - This may or may not happen, I'm either not sure or can't find a better way to write this test.
expect - I expect this thing to happen (or not to happen)
| describe "#check" do | ||
| let(:ems) { FactoryGirl.create(:ext_management_system) } | ||
| let(:zone) { FactoryGirl.create(:zone, :name => "api_zone", :ext_management_systems => [ems]) } | ||
| let(:my_server) { FactoryGirl.create(:miq_server, :zone => zone) } |
There was a problem hiding this comment.
I think you can use EvmSpecHelper.local_miq_server and remove the let(:zone)
| FactoryGirl.create(:service, :retired => true) | ||
| allow(MiqServer).to receive(:my_server).and_return(my_server) | ||
|
|
||
| expect_any_instance_of(LoadBalancer).to receive(:retirement_check).once.and_return(true) |
There was a problem hiding this comment.
I think these checks should expect a specific instance to receive the retirement_check. Also, the other instances should expect not to receive retirement_check, right?
app/models/retirement_manager.rb
Outdated
| ems_ids = MiqServer.my_server.zone.ext_management_system_ids | ||
| [LoadBalancer, OrchestrationStack, Vm, Service].each do |model| | ||
| table = model.arel_table | ||
| arel = table[:retires_on].not_eq(nil).or(table[:retired].not_eq(true)) |
There was a problem hiding this comment.
Any reason not to use regular ActiveRecord .where and .where.not?
d-m-u
force-pushed
the
merge-retirement-checks
branch
3 times, most recently
from
8cf5267 to
a59fe9c
Compare
d-m-u
force-pushed
the
merge-retirement-checks
branch
from
a59fe9c to
6b1c86d
Compare
| describe RetirementManager do | ||
| describe "#check" do | ||
| it "with retirement date, runs retirement checks" do | ||
| _, _server, zone = EvmSpecHelper.local_guid_miq_server_zone |
There was a problem hiding this comment.
For consistence, use either _guid, _server, zone or _, _, zone
|
|
||
| load_balancer = FactoryGirl.create(:load_balancer, :retires_on => Time.zone.today + 1.day, :ext_management_system => ems) | ||
| FactoryGirl.create(:load_balancer, :retired => true) | ||
| orch_stack = FactoryGirl.create(:orchestration_stack, :retires_on => Time.zone.today + 1.day, :ext_management_system => ems) |
Define the scope in the RetirementMixin
Create a scope for items not scheduled for retirement
app/models/retirement_manager.rb
Outdated
| end | ||
| end | ||
|
|
||
| private_class_method def self.not_retired_with_ems(model, ems_ids) |
There was a problem hiding this comment.
Prefer we use the syntax of placing private_class_method on the line below the method with the method name passed as a symbol to be consistent with the rest of the project.
Example:
manageiq/app/models/metric/capture.rb
Line 234 in 470292c
d-m-u
force-pushed
the
merge-retirement-checks
branch
from
a815663 to
228fbdd
Compare
d-m-u
force-pushed
the
merge-retirement-checks
branch
from
228fbdd to
9e36f18
Compare
|
Checked commits d-m-u/manageiq@17412ff~...9e36f18 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/models/miq_schedule_worker/runner.rb
|
Tests retirement checks bundled by @isimluk here. Vms, Services, OrchestrationStacks, and LoadBalancers run basically the same code for the regular retirement_checks which is now a single queue job thanks to Šimon. The tests create two of each model, only one of which passes the arel conditions of having a retirement date in the future, not being retired already, and having an ems_id present in the list of ids from MiqServer (with the exception of Service, which isn't attached to a zone). The arel is broken out into a separate method to avoid the issue of expect_any_instance_of running on the wrong instance.