Add readiness coordination for systemd notifications#87
Merged
Conversation
bastelfreak
force-pushed
the
feature/readiness-service
branch
3 times, most recently
from
9a248b0 to
bcfdd08
Compare
Signed-off-by: Tim Meusel <tim@bastelfreak.de>
bastelfreak
force-pushed
the
feature/readiness-service
branch
from
bcfdd08 to
331ef47
Compare
Contributor
Author
|
To test this on EL10: log from
This looks very confusing. Like trapperkeeper would iterate on services, reloads them, but sends a We see the message just twice, not 16 times. Copilot claims "your bootstrap.cfg list is a single app’s service graph, not multiple apps. In normal puppetserver runtime, that usually means 1 app with many services." But if there's just one app, why the second reload? With the fix in this PR, the reload works fine. full log: https://gist.github.com/bastelfreak/d23a4f565eb6e4565beb405243163253 The odd thing is that the amount of |
bastelfreak
force-pushed
the
feature/readiness-service
branch
from
331ef47 to
aea14b0
Compare
bastelfreak
commented
Jul 11, 2026
| ;; Trapperkeeper apps in the process. It can be used when connecting via nrepl | ||
| ;; to allow you to do useful things, and also may be used for other things | ||
| ;; (such as signal handling). | ||
| (def tk-apps (atom [])) |
Contributor
Author
There was a problem hiding this comment.
maybe we need to connect with nrepl and check whats in here. maybe there are two apps?
bastelfreak
commented
Jul 11, 2026
Comment on lines
+127
to
+129
| (defn app-log-id | ||
| [app] | ||
| (str "app=0x" (Integer/toHexString (System/identityHashCode app)))) |
Contributor
Author
There was a problem hiding this comment.
This was intended for debugging and not that helpful. or do the apps have humand readable names somewhere?
bastelfreak
commented
Jul 11, 2026
Comment on lines
+406
to
+412
| (defn maybe-restart-tk-apps | ||
| "Restart all TK apps unless this SIGHUP request is a rapid duplicate." | ||
| [apps] | ||
| (if (should-handle-sighup-restart?) | ||
| (restart-tk-apps apps) | ||
| (log/warn (i18n/trs "Ignoring duplicate SIGHUP restart request received within {0} ms" | ||
| min-sighup-restart-interval-ms)))) |
Contributor
Author
There was a problem hiding this comment.
I think this would be enough as a minimal workaround and we don't need the readinessservice (although I think it makes sense to have it). However, I don't know why we even receive a second SIGHUP.
bastelfreak
force-pushed
the
feature/readiness-service
branch
from
311467e to
7322721
Compare
corporate-gadfly
previously requested changes
Jul 11, 2026
Signed-off-by: Tim Meusel <tim@bastelfreak.de>
bastelfreak
force-pushed
the
feature/readiness-service
branch
from
7322721 to
e548da1
Compare
…t is ignored Apply suggestion from @corporate-gadfly Co-authored-by: Corporate Gadfly <corporate-gadfly@users.noreply.github.com> Signed-off-by: Tim Meusel <tim@bastelfreak.de>
bastelfreak
force-pushed
the
feature/readiness-service
branch
from
0068407 to
ba1c807
Compare
corporate-gadfly
approved these changes
Jul 11, 2026
Contributor
Author
|
whole log after the change from @corporate-gadfly The important pieces: |
Sharpie
reviewed
Jul 12, 2026
|
|
||
| ;; Debounce at the signal handler boundary to avoid back-to-back | ||
| ;; restart cycles that can interrupt in-flight agent requests. | ||
| (def min-sighup-restart-interval-ms 500) |
Member
There was a problem hiding this comment.
My one concern about this is that a reload cycle for a production service can take much longer than 500ms to complete.
The atom should probably track if we are :running, or :reloading, only allow one invocation of SIGHUP to swap over to :reloading, and short-circuit all other invocations until the atom is set back to :running.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short description
Checklist
I have:
Signed-off-byannotation to each of my commitsGenerated-byorAssisted-byannotations to each of my commits created with the help of an AI agent