Icinga / docker-icinga2

Official Icinga 2 Docker images
GNU General Public License v2.0
66 stars 30 forks source link

Running with --pid=host skips data dir init #101

Open flyingflo opened 1 year ago

flyingflo commented 1 year ago

It would be tempting to run the icinga2 container without a pid namespace, i.e. with --pid=host, to allow checks to see all processes/threads on the host.

However, the entrypoint program only initializes the data dir if its own PID is 1, which is obviously not the case with --pid=host. Is there a good reason for this behaviour? Why do we ony initialize if pid==1? Why not initialize whenever the dirs don't exist?

Al2Klimov commented 1 year ago

It would be tempting to run the icinga2 container without a pid namespace

What for?

flyingflo commented 1 year ago

It would be tempting to run the icinga2 container without a pid namespace

What for?

.. to allow checks to see all processes/threads on the host. The check_procs nagios plugin checks processes and threads which is not very meaningful inside the container.

lippserd commented 1 year ago

Is there a good reason for this behaviour? Why do we ony initialize if pid==1? Why not initialize whenever the dirs don't exist?

@Al2Klimov This sounds like a simple change to me. Is there any justification for pid == 1 or disadvantages for the proposed change?

Al2Klimov commented 1 year ago

Now, looking at the code post factum, I can think only of defensive programming à la "just to be sure" as reason for that condition. (It's basically the same as comparing len() with 0 with > and not !=.)

docker exec, despite my worries, doesn’t run the entrypoint, otherwise I'd see a "Running %#v" log message. The only other command runner except already spawned processes I know is CMD, where we obviously want the entrypoint to run. In short, only CMD runs entrypoint, we're safe on that side.

On the other hand, if I collapse a few control structures to see the whole entrypoint code, I see it only runs a given command in addition to the initialisation. Nothing one urgently needs our entrypoint for, bash does it as well. In short, one basically uses /entrypoint just for initialisation.

So AFAIK nothing really speaks against removing this condition. But, as always, I'm pretty prone to overseeing 🐘🐘, so I'd like a second opinion. You're pretty familiar with both Go and containers. Please be my second opinion.

flyingflo commented 1 year ago

One important difference is, that when running as pid 1, dumb-init is prepended to the command, such that dumb-init runs as pid 1 and forks icinga2.

This makes a lot of sense, because pid1 processes have special signal handling aspects, most importantly no default actions. Thus, a SIGTERM doesn't stop a process that isn't explicitly installing a handler for it, if it is pid 1. dumb-init takes care of this.

We could instead run the container with --init=true, to enable the init implementation that comes with docker or podman.