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

Fix MiqWorker service_base_name to match unit file prefix #21149

Merged
merged 4 commits into from
Apr 6, 2021
Merged

Fix MiqWorker service_base_name to match unit file prefix #21149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c1176ce
Fix service_base_name for renamed unit files
agrare Apr 6, 2021
1909685
Add a vmdb::plugins.systemd_units method
agrare Apr 6, 2021
2498547
Add spec test for workers <=> systemd units
agrare Apr 6, 2021
8e186ca
Use match array for expected units
agrare Apr 6, 2021
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
2 changes: 1 addition & 1 deletion app/models/miq_worker/systemd_common.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module SystemdCommon

class_methods do
def service_base_name
minimal_class_name.underscore.tr("/", "_")
"manageiq-#{minimal_class_name.underscore.tr("/", "_")}"
end

def systemd_unit_dir
Expand Down
4 changes: 4 additions & 0 deletions app/models/mixins/provider_worker_mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,9 @@ def all_valid_ems_in_zone
def any_valid_ems_in_zone?
all_valid_ems_in_zone.any?
end

def service_base_name
"manageiq-providers-#{minimal_class_name.underscore.tr("/", "_")}"
end
end
end
6 changes: 6 additions & 0 deletions lib/vmdb/plugins.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ def asset_paths
end
end

def systemd_units
@systemd_units ||= begin
flat_map { |engine| engine.root.join("systemd").glob("*.*") }
end
end

def load_inflections
each do |engine|
file = engine.root.join("config", "initializers", "inflections.rb")
Expand Down
18 changes: 9 additions & 9 deletions spec/models/miq_server/worker_management/monitor/systemd_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

context "#cleanup_failed_systemd_services" do
context "with no failed services" do
let(:units) { [{:name => "generic@68400a7e-1747-4f10-be2a-d0fc91b705ca.service", :description => "ManageIQ Generic Worker", :load_state => "loaded", :active_state => "active", :sub_state => "plugged", :job_id => 0, :job_type => "", :job_object_path => "/"}] }
let(:units) { [{:name => "manageiq-generic@68400a7e-1747-4f10-be2a-d0fc91b705ca.service", :description => "ManageIQ Generic Worker", :load_state => "loaded", :active_state => "active", :sub_state => "plugged", :job_id => 0, :job_type => "", :job_object_path => "/"}] }

it "doesn't call DisableUnitFiles" do
expect(systemd_manager).not_to receive(:DisableUnitFiles)
Expand All @@ -20,7 +20,7 @@
end

context "with failed services" do
let(:service_name) { "generic@68400a7e-1747-4f10-be2a-d0fc91b705ca.service" }
let(:service_name) { "manageiq-generic@68400a7e-1747-4f10-be2a-d0fc91b705ca.service" }
let(:units) { [{:name => service_name, :description => "ManageIQ Generic Worker", :load_state => "loaded", :active_state => "failed", :sub_state => "plugged", :job_id => 0, :job_type => "", :job_object_path => "/"}] }

it "calls DisableUnitFiles with the service name" do
Expand All @@ -36,8 +36,8 @@
context "#systemd_all_miq_services" do
let(:units) do
[
{:name => "generic@68400a7e-1747-4f10-be2a-d0fc91b705ca.service", :active_state => "failed"},
{:name => "ui@cfe2c489-5c93-4b77-8620-cf6b1d3ec595.service", :active_state => "active"},
{:name => "manageiq-generic@68400a7e-1747-4f10-be2a-d0fc91b705ca.service", :active_state => "failed"},
{:name => "manageiq-ui@cfe2c489-5c93-4b77-8620-cf6b1d3ec595.service", :active_state => "active"},
{:name => "ssh.service", :active_state => "active"}
]
end
Expand All @@ -50,8 +50,8 @@
context "#systemd_failed_miq_services" do
let(:units) do
[
{:name => "generic@68400a7e-1747-4f10-be2a-d0fc91b705ca.service", :active_state => "failed"},
{:name => "ui@cfe2c489-5c93-4b77-8620-cf6b1d3ec595.service", :active_state => "active"}
{:name => "manageiq-generic@68400a7e-1747-4f10-be2a-d0fc91b705ca.service", :active_state => "failed"},
{:name => "manageiq-ui@cfe2c489-5c93-4b77-8620-cf6b1d3ec595.service", :active_state => "active"}
]
end

Expand All @@ -62,15 +62,15 @@

context "#systemd_miq_service_base_names (private)" do
it "returns the minimal_class_name" do
expect(server.send(:systemd_miq_service_base_names)).to include("generic", "ui")
expect(server.send(:systemd_miq_service_base_names)).to include("manageiq-generic", "manageiq-ui")
end
end

context "#systemd_services (private)" do
let(:units) do
[
{:name => "generic@68400a7e-1747-4f10-be2a-d0fc91b705ca.service"},
{:name => "miq.slice"}
{:name => "manageiq-generic@68400a7e-1747-4f10-be2a-d0fc91b705ca.service"},
{:name => "manageiq-miq.slice"}
]
end

Expand Down
23 changes: 23 additions & 0 deletions spec/models/miq_worker/systemd_common_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
RSpec.describe MiqWorker::SystemdCommon do
describe ".service_base_name" do
before { MiqWorkerType.seed }

it "every worker has a matching systemd target and service file" do
Copy link
Member Author

Choose a reason for hiding this comment

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

Really want to make this a providers_common spec but we'll need a good way to determine which workers come from which engine.

This is how the container_common_spec is testing all worker deployment name lengths

Copy link
Member

Choose a reason for hiding this comment

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

Do we? It can't hurt to check all workers in all engines. If someone changes a particular engine, then the probability they break their own engine is higher than breaking others, especially since we won't merge on red.

expected_units = (Vmdb::Plugins.systemd_units + Rails.root.join("systemd").glob("*.*")).map(&:basename).map(&:to_s)

expected_units.delete("manageiq.target")

found_units = MiqWorkerType.worker_class_names.flat_map do |klass_name|
klass = klass_name.constantize
service_base_name = klass.service_base_name

service_file = "#{service_base_name}@.service"
target_file = "#{service_base_name}.target"

[service_file, target_file]
end

expect(expected_units).to match_array(found_units)
Copy link
Member

Choose a reason for hiding this comment

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

Just realized this should have been the other way around...

      expect(found_units).to match_array(expected_units)

but it's fine.

Copy link
Member Author

@agrare agrare Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@Fryguy I think this was actually correct originally. I swapped this in the follow-up PR but now that I'm running one switching this around prints missing unit files as "extra" rather than "missing". Nevermind there were issues with the worker seed for subclassed workers, thought it was the missing unit files but was actually missing worker classes

end
end
end