Performance improvements and host network issue fixes #16
Conversation
This reduces the chance of having dangling networks left. This can happen if all services are removed from a network and the network is automatically removed as is the case with e.g. "docker compose down". If Traefik does not leave a network quickly enough, the network is left unchanged because it is still "in use".
fix(main): disconnect networks early on "stop"
|
This PR seems very good to me and significantly reduces CPU consumption (from ~5% to 0% when idle). |
Sorry wasn't notified about this @mbrodala - there is already an image present: https://github.com/OXERY/traefik_network_connector/pkgs/container/traefik_network_connector |
|
@OXERY Maybe I'm missing something but how can one use this image? No matter what I try, I don't get a valid image name format. 🤔 |
|
@mbrodala Does pulling fail? |
|
Works this way, seems like trying to get an image name including SHA-checksum didn't work out. This is what I wanted: |
|
In any case it looks very well at the first sight. The latest version causes CPU spikes of ~15% every 2 seconds. Your version is basically at 0% all the time. :-) |
|
@mbrodala Nice you got it working! The black magic of cpu usage basically is: subscribe only to Docker topics you need 😊 |
|
Hi, I'm deeply sorry, I try to review this PR as soon as possible |
|
It seems that the |
|
@AlexT59 imho, I don't even think you'd need to make What is the benefit of the config variable |
|
No problem, I can create a separate PR if you want, as it is already configurable but is not used everywhere. I'm using the Sablier middleware, and it requires configuring Traefik using dynamic files rather than Docker labels. So I can't use the "traefik.enable" label, as that would create a new service. To work around this while still attaching the container's network to Traefik, I use the "traefik.connect" label (which is not recognised by Traefik). |
|
Hi @AlexT59, that sounds useful, indeed :) Also Sablier looks interesting - will put it on my Todo 👍 |
|
Ok should be done & therefore ready 4 review by @obeone :) |
|
@OXERY Wait, actually I'm working on another version that's kind of in between. I'll submit it for your review. You can tell me what you think. |
|
@OXERY Thanks for the quick update, but I think you forgot to update the |
|
@AlexT59 My bad - shouldn't code without enough caffeine. Fixed hopefully (TM) |
This commit addresses several issues found during PR #16 analysis: 1. Fix cache management bug: Replace `del` with `pop()` to prevent KeyError when processing sequential stop/die events on same container 2. Add explicit security warning: Log warning message when attempting to connect to 'host' network, improving visibility of security blocks 3. Add configuration validation: Validate monitoredLabel and monitoredLabelCondition are not empty at startup to fail fast These changes improve robustness and security of the PR #16 implementation.
4 participants
Came across the PR #12 today and updated it with a few performance optimizations.
The script now doesn't loop through all Docker events anymore, as this uses pretty much one CPU core on larger systems. Filters for relevant events and the
monitoredLabelexist, so that only events relevant for this tool are subscribed, including thestopevent, @mbrodala introduced in his PR.Also, this fixes #13 as from now on, connecting traefik to the network
hostis forbidden. (Could be made configurable, but I think connecting traefik tonetwork_mode: hostshould be a manual action either way.)