Skip to content

Conversation

yhabteab
Copy link
Member

This PR is similar to the changes made in the Icinga DB repository. This allows for more flexibility and consistency in building processes of the docker images as opposed to the previous method from the docker-icinga2 repository. The previously used Dockerfile has been renamed to Containerfile and the build process has been updated accordingly to make use of the Docker BuildKit caching capabilities. This change is expected to improve the build performance and allows for better layer caching, which drastically reduces the build time for subsequent builds on local machines.

As opposed to the previous behaviour, the current build process doesn't require a path to the source code to be passed
to the docker build command. Instead, the source code is bind mounted into the container at build time, more importantly
it doesn't require you to commit any changes made locally, but it simply uses the current state of the source code directory.
It's mounted as a readonly into the container, so no changes can be made to the source code from within the container.

Apart from that, this PR also automatically fixes various issues from the previous repository, such as:

  • There's now no intermediate locations for the config files, they are placed directly into the /data directory at build time. This eliminates the need for subsequent initialization steps by the container entrypoint script to copy the files around. As a result, the container entrypoint script has been dramatically simplified and now only contains the necessary steps to run a icinga2 node setup command if needed. This also means that users can now simply mount their own configuration files into the /data without any issues.
  • Since most of the issues in the legacy repository are about mounting issues or missing necessary env variables to additionally configure the container, this PR effectively removes the need for most of them. The only required environment variables are the ones that are already supported and documented in the docker-icinga2 repository. If a user wants to configure the container further, they can simply mount their own configuration files into the /data directory.

This PR also includes a For-Container.md file that contains the necessary information for users to set up the container and use it effectively. It provides a comprehensive guide on how to run the container, including the necessary environment variables, and how to mount configuration files. This file is intended to be a replacement for the README.md file in the docker-icinga2 repository.

Note: Commits from 5cd9ab2...20f28ec are cherry-picked from the Icinga DB repository.

See pushed images on Docker Hub.
See pushed images on GHCR.

@cla-bot cla-bot bot added the cla/signed label Jul 21, 2025
@yhabteab yhabteab requested review from julianbrost and lippserd July 21, 2025 10:54
@yhabteab yhabteab added enhancement New feature or request area/ci CI/CD labels Jul 21, 2025
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work with Docker Desktop on M3:

➜  icinga2 git:(docker-v2) docker run --rm -itv my-new-vol-4:/data -e ICINGA_MASTER=1 docker.io/icinga/icinga2:test
[2025-07-22 09:34:31] information/DockerEntrypoint: Icinga 2 Docker entrypoint script started.
[2025-07-22 09:34:31] information/DockerEntrypoint: Running Icinga 2 node setup command...
information/cli: Checking in existing certificates for common name '41c044b13034'...
information/cli: Certificates not yet generated. Running 'api setup' now.
information/cli: Generating new CA.
critical/Application: Error: Function call 'mkdir' for file '/data/var/lib/icinga2/ca' failed with error code 13, 'Permission denied'


Additional information is available in '/data/var/log/icinga2/crash/report.1753176871.799910'
/usr/local/bin/docker-entrypoint.sh: line 115:    16 Aborted                 icinga2 "${nodeSetup[@]}"
➜  icinga2 git:(docker-v2)

@yhabteab
Copy link
Member Author

It doesn't work with Docker Desktop on M3:

Is not architecture specific :) but thanks anyway for testing! I was thinking /usr/lib/icinga2/prepare-dirs would actually fix all Icinga 2 related directories but not its only purpose is to fix the /var/run, /var/cache etc. directories. With the last commit it should work normally now.

@Al2Klimov Al2Klimov dismissed their stale review July 22, 2025 13:00

OP addressed review

@yhabteab yhabteab force-pushed the docker-v2 branch 2 times, most recently from d9ab97d to 1b7aace Compare July 28, 2025 11:19
@yhabteab yhabteab force-pushed the docker-v2 branch 2 times, most recently from f49b776 to 520bdba Compare July 29, 2025 09:01
@yhabteab
Copy link
Member Author

Sorry, for the force pushes! Just wanted to test the concurrency rules! I won't push anything now unless someone says otherwise.

@julianbrost
Copy link
Contributor

So what are all the user-visible changes introduced by this PR?

  • Snapshot images will be pushed as icinga/icinga2:edge instead of icinga/icinga2:master.
  • Changed volume initialization behavior, in particular bind mounting a directory from the host to /data will no longer be initialized properly.

Anything more?

There's the switch to Debian trixie for the base image, for most users it shouldn't make a difference, but it could given just the changed versions of software available.

Then there's also Icinga/github-actions#8. Is there intentionally no reference to that one? (Which is fine for me, as I said in Icinga/github-actions#8 (comment), I'd expect the information on Docker Hub only to be updated once it applies to the latest image there.)

@yhabteab
Copy link
Member Author

  • Changed volume initialization behavior, in particular bind mounting a directory from the host to /data will no longer be initialized properly.

Wait what does that mean? Isn't this PR basically all about in making this work. The only thing that won't work with this PR is, when use the /data as a mount point for an empty directory from your host. In that case, docker will obscure the entire /data volume as noted in the For-Container.md file but that's not how you should bind mount something into the container anyway, so no big deal.

Then there's also Icinga/github-actions#8. Is there intentionally no reference to that one?

Hmm, what kind of reference would you like? I already mentioned it in #10505 (comment).

Which is fine for me, as I said in Icinga/github-actions#8 (comment), I'd expect the information on Docker Hub only to be updated once it applies to the latest image there.)

Your reasoning applies to snapshot users as well, and the legacy and Icinga DB docker file does the same thing, so I don't know 🤷!

@julianbrost
Copy link
Contributor

Wait what does that mean?

Currently, the following works, with the new images it won't anymore:

install -d -u 5665 /tmp/data
docker run --rm -it -v /tmp/data:/data icinga/icinga2

In that case, docker will obscure the entire /data volume as noted in the For-Container.md file but that's not how you should bind mount something into the container anyway, so no big deal.

That's not what you should do because there's now something new in our documentation that says so? Or would you say that's something you shouldn't do in general? (If the latter, then I'm using containers wrong, because that's what I usually do if I care about the contents of a volume. Otherwise, deleting the volume is just one wrong invocation of docker system prune away.)

I'm not saying the change is bad, there's a good reason for it, but it's a user-visible change.

Hmm, what kind of reference would you like?

I didn't see anything in the PR description that made the relation between the PRs clear. So it wasn't clear to me if the absence of that means you see the PRs as completely independent.

I already mentioned it in #10505 (comment).

If it's somewhere in the middle of the comments, it will be overlooked, so that should go into the PR description.

Your reasoning applies to snapshot users as well,

You mean that once we upload a new icinga/icinga2:edge image, the new hints from For-Container.md already apply (particularly regarding mounting to /data) and the current documentation on Docker Hub would be outdated? Yes, that's true, but when only considering the options "documentation on Docker Hub describes only icinga/icinga2:latest" or "documentation on Docker Hub describes only icinga/icinga2:edge", the first one is the better option. In general, my question aim towards getting a better overview over all the changes to have a foundation for deciding what the best merge strategy for this change would be.

and the legacy and Icinga DB docker file does the same thing, so I don't know 🤷!

What exactly do you mean by the same thing?

@yhabteab
Copy link
Member Author

yhabteab commented Aug 28, 2025

That's not what you should do because there's now something new in our documentation that says so? Or would you say that's something you shouldn't do in general? (If the latter, then I'm using containers wrong, because that's what I usually do if I care about the contents of a volume. Otherwise, deleting the volume is just one wrong invocation of docker system prune away.)

No, not just because it states in our docs but because that's how bind mounts work in Docker. If you for whatever reason don't want to use a named volume (and no, saying volumes are vulnerable to docker system prune isn't an argument), you can first start the container and let it initialise the /data volume and copy that directory to your host system (docker cp docker-master:/data /tmp/data). Now, stop the container and use /data as mount point for your /tmp/data directory and boom it works.

If it's somewhere in the middle of the comments, it will be overlooked, so that should go into the PR description.

You could also just merge it and we wouldn’t have this discussion :).

What exactly do you mean by the same thing?

Same thing as they constantly push that file to docker hub on every push to the main branch.

Previously, the https://github.com/Icinga/docker-icinga2 repository was
used to build the Docker images for Icinga 2. However, due to its
various design flaws, the resulted images had limited usability and
required a lot of manual tweaking to make something useful out of them.
This commit now follows our new principles of building Docker images
from the Icinga DB repository, and replaces the old separate repository
with this one. It makes use of the newest Docker BuildKit features to
build the images in a more efficient way, while also granting users full
flexibility to easily extend or modify the images as they see fit
without any issues.
It's going to be released tomorrow, so let's use the latest Debian release
as the base image while we are at it. Also run always `apt upgrade` to upgrade
any outdated packages (if any).
@yhabteab
Copy link
Member Author

Just rebased it and changed the CMake build type flag from ReleaseWithDebInfo -> RelWithDebInfo.

@yhabteab
Copy link
Member Author

What exactly do you mean by the same thing?

Same thing as they constantly push that file to docker hub on every push to the main branch.

I've just noticed that I already changed that as your requested https://github.com/Icinga/github-actions/pull/8/files#diff-08d154f5dd3b5d03f02345316d0fe31c3e57b8e7d2125c6f6520cb9de447fd74R193 🤦‍♂️

@julianbrost
Copy link
Contributor

For Icinga DB, the changes were merged just a few days before the new version was released.

In general, my question aim towards getting a better overview over all the changes to have a foundation for deciding what the best merge strategy for this change would be.

So that's basically part of the question if merging this PR should be tied to some release schedule or not.

Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soft approval: change-wise this PR is fine for me, but I'm not formally approving it as I don't want it merged just yet until:

  • There's positive feedback from @jschmidt-icinga that the the build args in the current state work for his use case.
  • We've agreed on how we want to release this.

Regarding the latter, my suggestion would be as follows:

  1. Merge this PR.
  2. Subsequently, announce upcoming changes to our Icinga 2 container images, with the current snapshot builds under the edge tag being a preview (blog post anyone?)
  3. Maybe keep the last master-tagged image for 2-4 weeks, so that any users have time to switch over, but then remove it so that anyone who tries to pull from it notices its gone. (Note: users includes icinga-testing)
  4. Ensure that Icinga/github-actions#8 is merged before the next step (until then, merge order isn't relevant).
  5. Eventually, release Icinga 2.16.0 based on the new build process.
  6. For the time being, keep the builds for 2.15.x (and earlier) using the existing build process. Switching that over can then be considered at a later point.

Copy link
Contributor

@jschmidt-icinga jschmidt-icinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this with my workflow and everything works as expected. I'm able to mount my (partial) config directories into the container, certs are writable and while I would have liked to have CMAKE_CXX_FLAGS as an arg, I can live without it for now.

@yhabteab
Copy link
Member Author

Wait 😞! Just noticed that I'm installing boost-dev libraries in the final image as well.

@yhabteab
Copy link
Member Author

Too much copy-pasted from the previous build steps. The final image installs the non-dev version of the boost libraries now and removes the superfluous boost-test library as well.

@julianbrost julianbrost merged commit 2063d2b into master Sep 15, 2025
29 of 30 checks passed
@julianbrost julianbrost deleted the docker-v2 branch September 15, 2025 10:52
@julianbrost
Copy link
Contributor

First attempt failed because this repo didn't yet have access to the renamed token secret. I've allowed the repo to access the secret and restarted the job.

@julianbrost
Copy link
Contributor

Now it ran successfully and there are docker.io/icinga/icinga2:edge (or just icinga/icinga2:edge) and ghcr.io/icinga/icinga2:edge as intended. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci CI/CD cla/signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants