Fixes #32814 - Fix working dir when doesn't exist

[adiabramovitch]

No description provided.

[ezr-ondrej]

I'd rather fail to initialize the plugin if the working dir from setting doesn't exist.
The application should not expect, that user has set non-existent directory as working directory.

[adiabramovitch]

@adamruzicka Maybe Adam can say his opinion on the matter?

[adamruzicka]

I'd rather fail to initialize the plugin if the working dir from setting doesn't exist.
The application should not expect, that user has set non-existent directory as working directory.

Agreed. Even though trying to create the directory on demand would solve the issue, I too would prefer crashing at startup

[adiabramovitch]

@ezr-ondrej After raising an exception, I see that the job is not canceled but pending, is that the expected behavior?

[adamruzicka]

Well, raising a specific exception isn't really all that different from what we had here before. It still happens on runtime when we actually try to use it. It would be better if this happened during startup

[ezr-ondrej]

Did you find out why the ansible still appears active even if the dir doesn't exists?
Should we fail before the after_activation?

What about the directory creation (if we can create it, shouldn't we)?
I think having the setting nil is valid, we use temp in that case, don't we?

[lzap]

I am not expert in the DI hell that we have in Smart Proxy, but it looks like after_activation is considered a legacy API, you should probably use a service instead. Looking in the code, I see that failures in the legacy codepath does log an error but plugins are not considered as failed but the new API via services rethrows the exception which probably leads to plugin being deactivated:

module ::Proxy::LegacyRuntimeConfigurationLoader
  def configure_plugin
    plugin.class_eval(&plugin.after_activation_blk)
    logger.info("Successfully initialized '#{plugin.plugin_name}'")
  rescue Exception => e
    logger.error "Couldn't enable '#{plugin.plugin_name}'", e
    ::Proxy::LogBuffer::Buffer.instance.failed_module(plugin.plugin_name, e.message)
  end
end

module ::Proxy::DefaultRuntimeConfigurationLoader
  def configure_plugin
    wire_up_dependencies(plugin.di_wirings, plugin.settings.marshal_dump, di_container)
    start_services(plugin.services, di_container)
    logger.info("Successfully initialized '#{plugin.plugin_name}'")
  rescue Exception => e
    logger.error "Couldn't enable '#{plugin.plugin_name}'", e
    ::Proxy::LogBuffer::Buffer.instance.failed_module(plugin.plugin_name, e.message)
    raise e
  end
 # ...
end

See ISC DHCP module for an example how to load classes dynamically or implement what's called a service:

load_classes ::Proxy::DHCP::ISC::PluginConfiguration
load_dependency_injection_wirings ::Proxy::DHCP::ISC::PluginConfiguration
start_services :leases_observer

module Proxy::DHCP::ISC
  class PluginConfiguration
    def load_dependency_injection_wirings(container, settings)
      container.dependency :memory_store, ::Proxy::MemoryStore
      container.singleton_dependency :subnet_service, (lambda do
        ::Proxy::DHCP::SubnetService.new(container.get_dependency(:memory_store),
                                         container.get_dependency(:memory_store), container.get_dependency(:memory_store),
                                         container.get_dependency(:memory_store), container.get_dependency(:memory_store))
      end)
      container.dependency :parser, -> { ::Proxy::DHCP::CommonISC::ConfigurationParser.new }
      container.dependency :service_initialization, -> { ::Proxy::DHCP::CommonISC::IscSubnetServiceInitialization.new(container.get_dependency(:               +subnet_service), container.get_dependency(:parser)) }
      container.dependency :state_changes_observer, (lambda do
        ::Proxy::DHCP::ISC::IscStateChangesObserver.new(settings[:config], settings[:leases], container.get_dependency(:subnet_service), container.            +get_dependency(:service_initialization))
      end)

      container.singleton_dependency :free_ips, -> { ::Proxy::DHCP::FreeIps.new(settings[:blacklist_duration_minutes]) }

      if settings[:leases_file_observer] == :inotify_leases_file_observer
        require 'dhcp_isc/inotify_leases_file_observer'
        container.singleton_dependency :leases_observer, (lambda do
          ::Proxy::DHCP::ISC::InotifyLeasesFileObserver.new(container.get_dependency(:state_changes_observer), settings[:leases])
        end)
      elsif settings[:leases_file_observer] == :kqueue_leases_file_observer
        require 'dhcp_isc/kqueue_leases_file_observer'
        container.singleton_dependency :leases_observer, (lambda do
          ::Proxy::DHCP::ISC::KqueueLeasesFileObserver.new(container.get_dependency(:state_changes_observer), settings[:leases])
        end)
      end

      container.dependency :dhcp_provider, (lambda do
        Proxy::DHCP::CommonISC::IscOmapiProvider.new(
          settings[:server], settings[:omapi_port], settings[:subnets], settings[:key_name], settings[:key_secret],
          container.get_dependency(:subnet_service), container.get_dependency(:free_ips))
      end)
    end

    def load_classes
      require 'dhcp_common/subnet_service'
      require 'dhcp_common/free_ips'
      require 'dhcp_common/isc/omapi_provider'
      require 'dhcp_common/isc/configuration_parser'
      require 'dhcp_common/isc/subnet_service_initialization'
      require 'dhcp_isc/isc_state_changes_observer'
    end
  end
end

I think there are several places where you can throw an exception. I think you only need to implement load_classes and throw it there. If it does not work, then implement the load_dependency_injection_wirings, implement a service, start a service and throw it there.

There you have it. Dependency Injection for perhaps the most dynamic language in the world :-) And before anyone comments, I know that DI can make sense in the dynamic languages for some cases, I believe it is rather rare and it is not worth the investment.

Edited: Removed irrelevant parts so it is more readable. Not kidding.

[adiabramovitch]

@lzap Thanks for the idea!
@adamruzicka , @ezr-ondrej If you can take a look :)

[ezr-ondrej]

Looks cleaner, have you tested it won't run Ansible jobs when the dir is missing now?

BTW I will not insist, but it would deserve two PRs, but at least two commits (as the change of the loading style is not exactly related to the directory validation change :)

[adiabramovitch]

@ezr-ondrej When I tested I saw that the initialization of ansible failed and it doesn't show as a feature in smart proxy.
Moreover when I ran "run ansible roles" it failed as well.

[ezr-ondrej]

Apart of the one question, I'm good.
@adamruzicka ? :)

[adamruzicka]

One last thing and then it will be good to go

[adamruzicka]

Alright, looks good, sorry about the confusion. Let's get this in

[adamruzicka]

Thank you @adiabramovitch & @ezr-ondrej !