Temporary directories cleanup

lzap · March 9, 2021, 4:17pm

Hey,

it’s been a while since we enabled PrivateTmp for Foreman running via systemd. This makes sure that both /tmp and /var/tmp directories are unique and separated via namespaces. However it also brings a problem - these directories cannot be cleaned via systemd-tmpfiles as private tmp paths are excluded by the tool. They are managed by systemd itself and deleted only when a service is stopped (restarted).

However, we have a symlink /usr/share/foreman/tmp which points to /run/foreman where we hold Rails temp files (mostly cache but few temporary files like uploaded files - Katello ZIP manifests). This directory is a regular directory which can be put under systemd-tmpfiles control, in fact we do have such configuration already:

# cat /usr/lib/tmpfiles.d/foreman.conf
d /run/foreman 0750 foreman foreman -

Meaning of this is: create /run/foreman with the specified owner, group, permissions and never clean it. This is expected configuration.

Now, bootdisk users are running into problem when temporary files are not being deleted and many of these 50MB-sized bootdisks fill tmpfs. This is limitation of the RoR - send_file method is asynchronous and it’s not possible to delete temporary file after call. There are some ugly hacks but I would like to avoid them.

The solution is to create /run/foreman/bootdisk directory and put it under tmpfiles control with some short cleanup strategy (one hour). But I think this might be useful also for other plugins or code. Therefore I propose to create the following directories with cleanup configuration:

/run/foreman/clean-hourly
/run/foreman/clean-daily
/run/foreman/clean-weekly

Plugins and core could use them via File.join(Rails.root, "tmp", "clean-daily", "a_file.txt").

Opinions?

ezr-ondrej · March 10, 2021, 12:21am

I’m not sure if we need all of those, but I’m definitely for having them and given there is not much cost of having them all, lets just have them all?

Although one issue is what means clean-daily? Will my file live for a day or 0 - 24 h? Isn’t there a tiny possibility the cleaner will clean my file too soon?

Dirk · March 10, 2021, 7:10am

Here systemd-tmpfilesd differs from cron in a very positive way as you define a “Age” and it always compares timestamps and age, so it will really only remove a file after one day and not one time the day all files. It also will look into mtime, atime and ctime, which could also be an advantages.

https://www.freedesktop.org/software/systemd/man/tmpfiles.d.html#Age

lzap · March 10, 2021, 3:08pm

I guess tmpfiles command line utility has nothing to do with cron daemon, but I know what you mean.

I find quite many features of systemd extremely useful, for this particular case timers allows you to define things like “10 minutes after boot” or “when computer is idle” thanks to its tight integration with systemd. It’s not like cron is going anywhere, I tend to still use cron for running scripts and systemd.timers for user-based units or packaged software.

ekohl · March 10, 2021, 3:26pm

Not a full reply, but some notes.

AFAIK it’s only /tmp, not /var/tmp.

This tmpfiles is only shipped in RPMs, not in debs. That means that on Debian after a systemctl stop foreman that /run/foreman is gone.

My rule of thumb with application servers is that anything that writes to disk is suspicious and you always need to ask yourself if there’s a better way.

Is it possible to stream the file using send_data instead? This would have the benefit of not having files on disk at all and thus also solving it for containers. It may also work better in load balanced setups.

For reference, Archlinux doesn’t ship cron by default and only uses timers.

lzap · March 10, 2021, 4:03pm

It’s both according to the documentation.

Hmm that’s not good.

Bootdisk actually creates several files, downloads kernel, initramdisk, performs couple of UNIX commands to build an ISO. I wish it was easier. I think send_data would be kind of a step back, instead streaming file directly without opening it I’d need to read the whole 50MB into Ruby memory which I don’t want to do.

Okay no packaging changes if Debian does not use it. I will take care of deleting in the plugin itself.

Dirk · March 10, 2021, 4:16pm

I fully agree!

I have some customers preferring the one over the other, others completely do not care.

I typically stick with cron when I would need to create a script, a service and a timer for systemd, too. I do not see the benefit in this cases most times as it will increase complexity instead of reducing it.

Also some syntax and limitation of systemd is weird. An example of me implementing a systemd.timer for mysqldump as backup:

[Unit]
Description=mysqldump backup
 
[Timer]
OnCalendar=*-*-* 04:20:00
 
[Install]
WantedBy=timers.target


[Unit]
Description=mysqldump backup
 
[Service]
Type=oneshot
ExecStart=sh -c 'exec mysqldump --defaults-file=/root/.my.cnf --all-databases --add-drop-database --lock-all-tables > /var/backup/mysql-$$(date +%%A).sql'
ExecStart=sh -c 'tar --remove-files -C /var/backup/ -czvf /var/backup/mysql-$$(date +%%A).sql.tar.gz mysql-$$(date +%%A).sql'

So the OnCalender has some syntax you have to get used to, similar to cron’s syntax, but at least cron’s syntax I know now for over fifteen years.
And having to use two ExecStart as I can not use | or && is limiting and confusing and requiring sh -c to get output redirection also.
But if tools get modernized with systemd in mind like you are suggesting here for Foreman I see great results in the future for many use cases.

ekohl · March 10, 2021, 4:46pm

TIL

That’s too bad. You can’t pipe the output of a program to send_data?

It may not be a blocker. I’m not sure if there’s a good reason for the difference between deb and RPM. It may very well be an oversight.

The first hint that this is different is that it lives here:

If it was shared (and sharing is good), it would live here:

IMHO we should decide on what the correct behavior is (persistent or transient /run/foreman). From there on we can see what would be good behavior.

Also note that bootdisk can, if it wants to, ship its own tmpfiles with an age to remove old files. That is likely a better solution than trying to provide some generic solution to this.

Another thing to think about. Apache has sendfile support but I don’t know if that works in our reverse proxy setup. If you also consider that (/var)/tmp is private, I doubt Apache can support that. Does this mean a Ruby process is streaming the data anway?

Lastly, there is JoinsNamespaceOf= which timer units can use. From reading the documentation I think that can clean (/var)/tmp even if they are private.

lzap · March 11, 2021, 9:46am

Yeah, in this case you could do Daily if you don’t mind running it on midnight. There is also some randomization and time resolution options available to make it less common to execute the job right at midnight.

Actually this is a feature, not bug. You want your services to start fast, meaning they must not run shell. Shell is bad when it comes to performance. But I feel the pain here, you don’t want to break this into three individual pieces: timer, service and backup script. That’s for these kinds of jobs I think crond is still better (one script, one line) and systemd timers are better suited for OS services (e.g. Foreman).

Agreed, that’s more on the infra team to make the final call. If Debian packages can catch up it would be awesome to have some common place getting regularly cleaned. By the way all Foreman uploads go straightly into /tmp (private) so an attacker can actually perform very easy DDoS with uploading multiple 6MB-sized files (there is some kind of maximum limit in Rails I think) into foreman (e.g. manifest). We will never clean them until the service restart. BTW this attack would fill all virtual memory, not just disk.

Yeah but then I have the same problem - Debian. I’d rather see some common place that is available on both platforms.

I have no idea honestly, I really really hope it’s not getting loaded into Ruby memory and it is actually streamed.

Feels a bit hacky.

ekohl · March 11, 2021, 10:47am

The code suggests it streams it in chunks of 16k if it can’t let the webserver send it directly:

github.com

rails/rails/blob/168ddaa08a63cd956bb7c3ba10be1a7ae36d4ee2/actionpack/lib/action_dispatch/http/response.rb#L336-L364


# Avoid having to pass an open file handle as the response body.
# Rack::Sendfile will usually intercept the response and uses
# the path directly, so there is no reason to open the file.
class FileBody #:nodoc:
  attr_reader :to_path
  def initialize(path)
    @to_path = path
  end
  def body
    File.binread(to_path)
  end
  # Stream the file's contents if Rack::Sendfile isn't present.
  def each
    File.open(to_path, "rb") do |file|
      while chunk = file.read(16384)
        yield chunk
      end

This file has been truncated. show original

We also don’t configure x_sendfile_header and I doubt we can use X-Sendfile since Apache can’t read the file from the private tmp so Ruby will have to do it.

github.com

theforeman/foreman/blob/71c10db57fd64bc6cee49e904d6d3836bfe7d3da/config/environments/production.rb#L34-L36


# Specifies the header that your server uses for sending files.
# config.action_dispatch.x_sendfile_header = "X-Sendfile" # for apache
# config.action_dispatch.x_sendfile_header = 'X-Accel-Redirect' # for nginx

render (ActionController::Base) - APIdock suggests using this pattern:

  # Streams about 180 MB of generated data to the browser.
  render :text => proc { |response, output|
    10_000_000.times do |i|
      output.write("This is line #{i}\n")
    end
  }

That should give you a place to do cleanup after sending all the data, right?

lzap · March 15, 2021, 11:43am

I will look into that, first I have a similar PR in core: Fixes #31274 - stream reports to save memory by lzap · Pull Request #8136 · theforeman/foreman · GitHub

Once I get it working, then I can take a look on Bootdisk.

Thanks.