Completely remove the MiqVimBrokerWorker

[agrare]

This is the complete removal of the MiqVimBrokerWorker

Depends on:

• Completely remove the VimBrokerWorker manageiq-providers-vmware#506
• With streaming refresh reset_vim_cache not needed manageiq-ui-classic#6576
• Remove all MiqVimBrokerWorker rows manageiq-schema#447

Cross Repo Tests: ManageIQ/manageiq-cross_repo-tests#44

[jrafanie]

We probably should have a migration to remove any miq_worker rows for broker workers

[chessbyte]

We probably should have a migration to remove any miq_worker rows for broker workers

@jrafanie won't the miq_worker rows be removed when the appliance gets restarted here?

[Fryguy]

We probably should have a migration to remove any miq_worker rows for broker workers

@jrafanie won't the miq_worker rows be removed when the appliance gets restarted here?

@chessbyte - The problem with the method you referenced is that it is still STI aware. That is, when the query sees an existing MiqVimBrokerWorker row, it will try to instantiate that class and blow up because it won't find the class anymore. This has bitten us before, unfortunately.

However, @jrafanie, I think @chessbyte has a point. If we make the method NOT STI aware, would that solve all of our future problems? Two ways I can think to do it...

select columns without type

MiqWorker.server_scope.select(:id, :pid, :status).each do |w|
  # w.class => MiqWorker
  ...
end

pluck columns

MiqWorker.server_scope.pluck(:id, :pid, :status).each do |w_id, w_pid, w_status|
  # however, you can't do w.is_alive?
end

---

Totally separate, we can probably make a generic without_sti scope, that is basically just

def self.without_sti
  select(column_names - ["type"])
end

I could see this being really useful in a lot of situations.

[agrare]

Okay put the schema PR in and kicked the cross-repo tests

[chessbyte]

We probably should have a migration to remove any miq_worker rows for broker workers

@jrafanie won't the miq_worker rows be removed when the appliance gets restarted here?

@chessbyte - The problem with the method you referenced is that it is still STI aware. That is, when the query sees an existing MiqVimBrokerWorker row, it will try to instantiate that class and blow up because it won't find the class anymore. This has bitten us before, unfortunately.

Thanks for the clarification.

However, @jrafanie, I think @chessbyte has a point. If we make the method NOT STI aware, would that solve all of our future problems? Two ways I can think to do it...

Agree that we do not need to instantiate a specific worker class just so we can kill the process and delete the row.

select columns without type

MiqWorker.server_scope.select(:id, :pid, :status).each do |w|
  # w.class => MiqWorker
  ...
end

pluck columns

MiqWorker.server_scope.pluck(:id, :pid, :status).each do |w_id, w_pid, w_status|
  # however, you can't do w.is_alive?
end

I prefer the select approach.

Totally separate, we can probably make a generic without_sti scope, that is basically just

def self.without_sti
  select(columns_names - ["type"])
end

I could see this being really useful in a lot of situations.

Totally agree that this would be useful in other scenarios. Where should this be defined?

[jrafanie]

We probably should have a migration to remove any miq_worker rows for broker workers

@jrafanie won't the miq_worker rows be removed when the appliance gets restarted here?

Looking at @Fryguy's cool ideas about avoiding the the STI problem with missing classes, I am also wondering if we need to be in the business of managing the server's worker processes. I think much of these process related checks were written before systemd and I don't know how likely there are to be runaway processes that don't exit or don't get killed by systemd. The processes can't be heartbeating as their server's drb port has changed once the server was restarted so I'm not sure what situation a worker could get in that would leave it running but not heartbeating and not exceeding memory... maybe a deadlock? Either way, I'm curious if we would have written this code in this way if we were to start from scratch with systemd.

[chessbyte]

Part of ManageIQ/manageiq-providers-vmware#484

[chessbyte]

@kbrock maybe you can take a look at implementing the without_sti method/scope discussed above.

[chessbyte]

@agrare what is the status of this PR? hard to figure out what the dependencies and/or issues are before this can be merged

[agrare]

Yeah sorry @chessbyte I've been working on finishing up this issue #19543 then I'm going to get back to this. Mainly just need to get the cross-repo tests green in addition to some manual testing.

[Fryguy]

Can we drop this setting altogether? Do we need a data migration to drop it?

[miq-bot]

Checked commits agrare/manageiq@201fc05~...755d219 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
15 files checked, 5 offenses detected

app/models/vm_or_template.rb

• ❗ - Line 442, Col 5 - Style/SafeNavigation - Use safe navigation (&.) instead of checking if an object exists before calling the method.

spec/models/miq_action_spec.rb

• ❗ - Line 189, Col 10 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 189, Col 15 - Layout/SpaceAroundOperators - Operator = should be surrounded by a single space.
• ❗ - Line 89, Col 10 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 89, Col 13 - Layout/SpaceAroundOperators - Operator = should be surrounded by a single space.

[Fryguy]

🎉 🎉 🎉 🎉 🌮 🎉

[jrafanie]

🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽
🔥 🗑 🚽

[chessbyte]

Wow!!!!! This has been a long time coming!! Congratulations to all involved for making this happen!