katello-nightly-rpm-pipeline 1633 failed

ekohl · April 5, 2023, 10:37am

Just making a note here: we can consistently see this failing over the past few days.

Looking at Katello’s and Foreman’s git log I don’t see anything that would even touch these areas, so I wonder if this is something in some other RPM we updated that now interacts unexpectedly. We don’t see this in Foreman’s tests so I don’t know if it’s just chance.

Katello does have additional middleware that Foreman doesn’t, so either Katello::Middleware::EventDaemon (which I still question; you shouldn’t attempt to start a daemon thread in middleware) or Katello::Middleware::OrganizationCreatedEnforcer are suspects to me.

For example, it could be this line:

github.com

Katello/katello/blob/18d404c80c13f7b5e814031a3fd227b06ca7c0cd/app/lib/katello/event_daemon/runner.rb#L16


      
          @cache = ActiveSupport::Cache::MemoryStore.new
          
          class << self
            def settings
              SETTINGS[:katello][:event_daemon]
            end
          
            def pid
              return unless pid_file && File.exist?(pid_file)
          
              File.read(pid_file).to_i
            end
          
            def pid_file
              pid_dir.join('katello_event_daemon.pid')
            end
          
            def tmp_dir
              Rails.root.join('tmp')
            end

If the file was somehow corrupted or there’s a race condition, it could be empty and return nil perhaps?

Not entirely the same situation and very old, but it looks like it could happen:

Reading more on Rails, you are supposed to wrap threads that you spawn:

So perhaps this is something that would make the EventDaemon safer.

I’ve opened a PR with some potential fixes:

github.com/Katello/katello

Fixes #36277 - Make the event daemon runner safer

Katello:master ← ekohl:safer-event-daemon-runner

opened 10:37AM - 05 Apr 23 UTC

ekohl

+8 -4

#### What are the changes introduced in this pull request? Most importantly thi…s wraps the monitor in an executor, which is recommended by Rails. Quoting the [manual](https://guides.rubyonrails.org/threading_and_code_execution.html#wrapping-application-code): > Each thread should be wrapped before it runs application code, so if your application manually delegates work to other threads, such as via Thread.new or Concurrent Ruby features that use thread pools, you should immediately wrap the block. In addition to that it uses the atomic `FileUtils.rm_f` instead of first checking if the file exists. Similarly on read it attempts to read the file, and catches the exception if it doesn't exist instead of first checking if it does to avoid a race condition. To be extra safe, it uses the safe operator in case File.read returns nil. #### Considerations taken when implementing this change? This is based on reading the Rails manual and applying [the Easier to Ask Forgiveness Than Permission (EAFP)](https://realpython.com/python-lbyl-vs-eafp/) approach that's common in Python. I've also seen RuboCop flag `File.unlink(path) if File.exist?(path)` and replace it with `FileUtils.rm_f` since it's atomic. #### What are the testing steps for this pull request? Hard to say. I've always found this code to be suspect, but https://community.theforeman.org/t/katello-nightly-rpm-pipeline-1633-failed/33071 triggered me to look into it again.