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

Cleanup constantize of worker_class_names #21470

Merged
merged 2 commits into from
Oct 5, 2021
Merged

Cleanup constantize of worker_class_names #21470

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86ac151
Cleanup constantize of worker_class_names
agrare Sep 29, 2021
f30b447
Add providers_common spec testing worker class names
agrare Sep 30, 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
21 changes: 7 additions & 14 deletions app/models/miq_server/worker_management/monitor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,14 @@ def worker_not_responding(w)
end

def sync_workers
result = {}
MiqWorkerType.worker_class_names.each do |class_name|
begin
c = class_name.constantize
raise NameError, "Constant problem: expected: #{class_name}, constantized: #{c.name}" unless c.name == class_name
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this line was lost in the refactoring. It was probably added for better diagnostics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well removing this was actually the point of this change. We have control over what makes it into miq_worker_types so this shouldn't happen. I believe this was from before we had the miq_worker_types table back when it was self.class.monitor_class_names

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the explanation

Copy link
Member

Choose a reason for hiding this comment

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

No, this was added specifically because of that weird issue with Ruby where you can ask for a particular constant and get back the wrong one if someone happened to defined one in your module ancestry. See the commit message in afe1f02

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but how would one of those "incorrect" class names make it in to MiqWorkerType ?

Copy link
Member

@Fryguy Fryguy Sep 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That being said, afe1f02 was added 2019-10-25, and the refactor into the database table was added in 2947aac on 2019-11-19

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was thinking if we prevent bad class names from hitting MiqWorkerType in the first place then we don't need to check this at runtime, and everyone else who does constantize on a worker_class_name doesn't have to duplicate this check.

Copy link
Member

Choose a reason for hiding this comment

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

@agrare Oh I'm not disagreeing with you ...just clarifying what the purpose of this line actually was and showing that yes, it was from before the refactor to a table

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree. You're both right. 👍

Copy link
Member

Choose a reason for hiding this comment

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

✂️ 🔥 ✂️ 🔥 ✂️ 🔥

🍰 🍪 👏 🙇 😍 🎉


result[c.name] = c.sync_workers
result[c.name][:adds].each { |pid| worker_add(pid) unless pid.nil? }
rescue => error
agrare marked this conversation as resolved.
Show resolved Hide resolved
_log.error("Failed to sync_workers for class: #{class_name}")
_log.log_backtrace(error)
next
end
MiqWorkerType.worker_class_names.map(&:constantize).each_with_object({}) do |klass, result|
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if MiqWorkerType.worker_class_names.map(&:constantize) should be extracted to a method like MiqWorkerType.worker_classes

Copy link
Member Author

Choose a reason for hiding this comment

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

💯 may or may not already have that branch as a follow-up to this :)

Copy link
Member

Choose a reason for hiding this comment

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

doing a search in the core repo shows

$ git grep worker_class_names
app/models/miq_server/worker_management/monitor.rb:44:    MiqWorkerType.worker_class_names.each do |class_name|
app/models/miq_server/worker_management/monitor.rb:163:    MiqWorkerType.worker_class_names_in_kill_order.each do |class_name|
app/models/miq_server/worker_management/monitor/settings.rb:11:    MiqWorkerType.worker_class_names.each do |class_name|
app/models/miq_server/worker_management/monitor/start.rb:13:      starting = MiqWorker.find_starting.find_all { |w| MiqWorkerType.worker_class_names.include?(w.class.name)
app/models/miq_server/worker_management/monitor/systemd.rb:51:      MiqWorkerType.worker_class_names.map(&:constantize).map(&:service_base_name)
app/models/miq_worker_type.rb:27:  def self.worker_class_names
app/models/miq_worker_type.rb:31:  def self.worker_class_names_in_kill_order
spec/models/miq_server/worker_management/monitor_spec.rb:99:        allow(MiqWorkerType).to receive(:worker_class_names).and_return(%w[ManageIQ::Providers::Foreman::Provis
spec/models/miq_server/worker_management/monitor_spec.rb:107:        allow(MiqWorkerType).to receive(:worker_class_names).and_return(%w(MiqGenericWorker MiqPriorityWorker)
spec/models/miq_server/worker_management/monitor_spec.rb:114:        allow(MiqWorkerType).to receive(:worker_class_names).and_return([])
spec/models/miq_worker/systemd_common_spec.rb:10:      found_units = MiqWorkerType.worker_class_names.flat_map do |klass_name|

I would bet that each of those callers is doing a constantize to get the actual classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes not all of them are but the majority

Copy link
Member

@jrafanie jrafanie Sep 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Note, worker_classes could add that constantize check on the class name matching the worker type name if that is really still needed. Looking at my original PR, it looks like the problem was in const_get doing the wrong thing. I guess that could still happen if rails autoload still uses it. 🤷

I can look to see if that's still an issue. But here's more context: #19400 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right AFAIK constantizing classes which don't exist won't always fail (could resolve to the non-namespaced base class), but constantizing classes which do exist will always resolve to the right thing?

Copy link
Member

Choose a reason for hiding this comment

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

@agrare I believe that's correct.

Copy link
Member

Choose a reason for hiding this comment

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

Note that newer versions of Ruby / Rails may have eliminated this problem entirely as well, but in this case you know all the values will exist (because they were derived from existing values), so you are safe.

result[klass.name] = klass.sync_workers
result[klass.name][:adds].each { |pid| worker_add(pid) unless pid.nil? }
rescue => error
_log.error("Failed to sync_workers for class: #{klass.name}: #{error}")
_log.log_backtrace(error)
next
end
result
end

def sync_from_system
Expand Down
13 changes: 0 additions & 13 deletions spec/models/miq_server/worker_management/monitor_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,19 +90,6 @@
context "#sync_workers" do
let(:server) { EvmSpecHelper.local_miq_server }

it "ensures only expected worker classes are constantized" do
# Autoload abstract base class for the event catcher
ManageIQ::Providers::BaseManager::EventCatcher

# We'll try to constantize a non-existing EventCatcher class in an existing namespace,
# which incorrectly resolves to the base manager event catcher.
allow(MiqWorkerType).to receive(:worker_class_names).and_return(%w[ManageIQ::Providers::Foreman::ProvisioningManager::EventCatcher MiqGenericWorker])

expect(ManageIQ::Providers::BaseManager::EventCatcher).not_to receive(:sync_workers)
expect(MiqGenericWorker).to receive(:sync_workers).and_return(:adds => [111])
expect(server.sync_workers).to eq("MiqGenericWorker"=>{:adds=>[111]})
end

it "rescues exceptions and moves on" do
allow(MiqWorkerType).to receive(:worker_class_names).and_return(%w(MiqGenericWorker MiqPriorityWorker))
allow(MiqGenericWorker).to receive(:sync_workers).and_raise
Expand Down