search

ManageIQ / manageiq-automation_engine

Automation engine for ManageIQ
Apache License 2.0
11 stars 73 forks source link

Use load when loading static service models #536

Closed kbrock closed 8 months ago

kbrock commented 9 months ago

Before

When development reloads classes, MiqAeService models run into an issue.

  1. reference an unknown automate service model.
  2. calls MiqAeMethodService.const_missing
  3. MiqAeServiceModelBase.create_service_model_from_name
  4. MiqAeServiceModelBase.create_service_model
  5. if file containing service model is found require file_name HERE
  6. reference same service model
  7. since we required file, it is found and all is well

If rails reloads a class, it unloads the service models, but since the file is still marked as required, the require does nothing. So the workflow changes as follows:

  1. the file was already required, so requre is a no-op
  2. reference same constant (still unknown) - step 1
  3. calls MiqAeMethodService.const_missing
  4. infinite loop

To display the dialog for provisioning a Vm, automate is called inline in the webserver, so the webserver starts crashing with stack overflow errors.

irb(main):001:0> MiqAeMethodService::MiqAeServiceVm
=> MiqAeMethodService::MiqAeServiceVm
irb(main):002:0> reload!
Reloading...
=> true
irb(main):003:0> MiqAeMethodService::MiqAeServiceVm
/Users/kbrock/.gem/ruby/3.0.6/gems/activesupport-6.1.7.6/lib/active_support/core_ext/string/inflections.rb:87:in `safe_constantize': stack level too deep (SystemStackError)
    from manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:159:in `service_model_name_to_model'
    from manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:117:in `create_service_model_from_name'

After

irb(main):001:0> MiqAeMethodService::MiqAeServiceVm
=> MiqAeMethodService::MiqAeServiceVm
irb(main):002:0> reload!
Reloading...
=> true
irb(main):003:0> MiqAeMethodService::MiqAeServiceVm
=> MiqAeMethodService::MiqAeServiceVm
irb(main):004:0> quit
kbrock commented 8 months ago

So the caching issue is an existing issue. Not sure if it will be an issue but want to wait until I get a reproducer.

The load is the only part I need. I'll change this refactoring to do that.

kbrock commented 8 months ago

update:

We can probably fix this changing the way zeitwerk is implemented, but I was hoping to get this minimal change in first

miq-bot commented 8 months ago

Checked commit https://github.com/kbrock/manageiq-automation_engine/commit/aea5b21acacc7e80001c0d9a7b201443a7a92371 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 1 file checked, 0 offenses detected Everything looks fine. :+1:

Fryguy commented 8 months ago

I'm good with merging this as is to get rid of the stack overflow.

Fryguy commented 8 months ago

Backported to quinteros in commit 5f47ecbf645fa57224aad614ae651a901e5b6492.

commit 5f47ecbf645fa57224aad614ae651a901e5b6492
Author: Jason Frey <fryguy9@gmail.com>
Date:   Fri Dec 8 16:02:15 2023 -0500

    Merge pull request #536 from kbrock/const_get

    Const get

    (cherry picked from commit 9e3f656946fa5995fc77705ef7609363a77b4f48)