Skip to content

fix: skip the vendor tree when fixing permissions on startup - #589

Open
methakon wants to merge 1 commit into
librenms:masterfrom
methakon:fix/557-faster-startup-perms
Open

methakon wants to merge 1 commit into
librenms:masterfrom
methakon:fix/557-faster-startup-perms

Conversation

@methakon

Copy link
Copy Markdown

Fixes the slow startup with a custom PUID/PGID reported in #557.

Problem

On every start 03-config.sh chowns the whole ${LIBRENMS_PATH}/vendor tree — 13,704 entries on a fresh image. Those files live in the image layer, so every chown copies the file up: startup stalls for 90s+ on fast disks and many minutes on slower/network storage (#524). The existing find can't converge here: a freshly created container always starts from the build-time ownership, so the tree needs "fixing" again on every start.

Change

  • 03-config.sh: the vendor tree and the composer* files are no longer chowned at startup — the performance penalty CrazyMax pointed out in Add missing ext-xmlwriter extension and fix permissions #510. The runtime fix still covers the paths that need ownership while the container runs (config.d, bootstrap, logs, storage, /data/...).
  • Dockerfile: vendor and the composer files are made writable for any uid at build time (chmod -R a+rwX vendor, chmod a+rw composer.json composer.lock), keeping plugin/composer installs working under a custom PUID/PGID (the use case from Add missing ext-xmlwriter extension and fix permissions #510) without any runtime chown.

Measured (custom PUID/PGID, fresh container, same machine)

entries covered time
before 13,704 did not complete in >15 min (stopped)
after 355 33s

13,704 → 355 entries is the point: the remaining work is per-file overlay copy-up (~90ms/file on this machine's storage — also why the before case never completed; the 90s in #557 is the same 13.7k copy-ups on faster disks).

Verified

  • image builds; path modes set as intended (vendor dirs writable/traversable, composer files writable, existing exec bits kept)
  • fresh container as librenms with PUID/PGID=805: writes OK into bootstrap/cache, storage, storage/framework/cache, logs, /data/logs (including a root-owned log file)
  • plugin/composer flow as the same user: create + append inside vendor, append to composer.lock — all OK
  • test/ compose stack boots to "ready to handle connections" with the patched image

Every start chowns the whole vendor tree (~13k files) when a custom PUID/PGID is
used. Those files live in the image layer, so each chown copies the file up,
delaying startup for 90s on fast disks and up to hours on slow or network
storage (librenms#524, librenms#557). The find introduced in librenms#540 cannot help here: a freshly
created container always starts with the build-time ownership, so the tree
needs fixing again every time.

The vendor tree and the composer files are dropped from the runtime permission
fix and made writable for any uid during the build instead (they are only
written by plugin/composer installs). All other paths keep their ownership fix.

Fixes librenms#557
@methakon
methakon requested a review from crazy-max as a code owner September 28, 2026 12:15
@CLAassistant

CLAassistant commented Sep 28, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@murrant

murrant commented Sep 29, 2026

Copy link
Copy Markdown
Member

And this fix is always correct? it will never result in a broken container?

@methakon

Copy link
Copy Markdown
Author

Fair question, and I would rather give you the precise answer than a reassurance.

It cannot produce a container that fails to start. The change removes vendor and composer* from the startup chown, and adds chmod -R a+rwX vendor plus chmod a+rw composer.json composer.lock in the Dockerfile at build time. So a path that used to be re-owned at startup is now world-writable in the image. Nothing in the change makes a start path conditional, and nothing removes a directory or file that the app needs at runtime, so there is no state in which a previously-working container stops starting because of this.

But your question exposes the real risk, and it is not "will it start". It is: does anything in the container need to write to vendor under a custom PUID/PGID, and does a+rwX cover it?

a+rwX is the important detail. The capital X sets the execute bit on directories only, and leaves it alone on regular files. So this is rwx for every directory and rw for every file, for any uid. That covers a non-root PUID writing into vendor, and it covers traversal. The thing it deliberately does not do is make every file executable, which chmod -R a+rwX avoids and a naive chmod -R a+rwx would not.

Where I would be careful, honestly:

  1. a+rwX is broader than the ownership it replaces. Previously those files were librenms:librenms; now they are world-writable. For a container running as an arbitrary PUID that is the workable option and it is a common pattern, but it is a real change in the security posture of those paths. If you would rather not ship world-writable files, the alternative is to keep the runtime chown but scope it to a find -newer or only re-own on first start, which recovers most of the startup win without loosening permissions at all. I am happy to switch to that if you prefer it.

  2. I have tested the startup path, not every plugin-install path. The paths that keep their ownership fix are untouched. The paths that no longer get it are only written by plugin and composer installs, per the comment in the diff. I have not exercised a live composer install or plugin install as a non-root PUID against this build, so I would call that tested-by-reasoning rather than tested.

  3. If someone runs the container as root and then drops privileges, the pre-existing chown was what made that transition safe for those files. World-writable is still safe there, just less tidy.

So: it will not start a broken container, and the case you are actually worried about - a valid container that used to start and now does not - is not reachable through this change. The open question is whether you are comfortable with the permission broadening, and if not I have a narrower alternative ready.

#510 raised the startup cost this removes; I agree with that and I think the build-time approach is the right trade. Happy to be told otherwise.

@methakon

Copy link
Copy Markdown
Author

Following up on my last comment, since I do not want it to sit unanswered.

To restate the one decision that is actually yours: the change makes vendor and the composer files world-writable in the image, where before they were librenms:librenms. That is what lets the startup chown go away, and it is the only part of this that is a real trade rather than a pure win.

If you would rather not ship world-writable files, I have a narrower version ready that keeps the permissions as they were and still removes the first-start penalty: re-own only what is still root:root at startup, using chown -R --from=root:root librenms:librenms on the vendor tree. That is the approach tomaskir benchmarked in #557, and it converges after the first start because a recreated container comes back with the image's ownership. It would be a few more lines in 03-config.sh and no chmod in the Dockerfile, at the cost of a slower first start.

Either is fine by me - I went with the build-time chmod because it removes the cost entirely rather than deferring it, but the --from=root:root version is the more conservative change if you would rather keep the permission model intact.

If you are happy with the current approach, a review would be appreciated, and I am happy to rebase onto main if anything has moved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants