Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove nginx initialDelaySeconds #326

Merged
merged 5 commits into from
Mar 26, 2025

Conversation

klml
Copy link
Contributor

@klml klml commented Mar 4, 2025

What this PR does / why we need it

The readiness-probe nginx delays accepting traffic for 30 seconds with initialDelaySeconds, but nginx is ready after 1-2 seconds, this unnecessarily delays my deploy or restart.

The initialDelaySeconds: 30 on the readiness-probe can make sense for rails, because rails needs about 30 seconds to accept traffic, but the readiness-probe checks on port 3000 anyway.

Additional changes

Currently I would also suggest

  • removeinitialDelaySeconds: 30 on the readiness-probe, it checks on port 3000 anyway.
  • remove successThreshold: 1 Default is already to 1
  • remove periodSeconds: 10 Default is already to 10

Checklist

  • Chart Version bumped
  • Upgrading instructions are documented in the zammad/README.md

@mgruner
Copy link
Collaborator

mgruner commented Mar 11, 2025

@klml thanks for your research. It looks like the readiness probe is somewhat abused to make for the long rails startup time. Wouldn't it be better to add a startup probe for that?

  • startup probe: short interval, but high failure threshold to make for 30s startup time or more - this way it would complete earlier than 30s in case the stack is faster (e.g. in case of patchlevel update without migrations)
  • readiness probe: short interval (perhaps defaults are fine?), no initialDelay needed
  • liveness probe: same

We could choose a similar approach for all 3 affected pods: nginx, railsserver and websocket-server.

I tend to also agree that we can remove any values which don't change the defaults from k8s.

What do you think?

@klml
Copy link
Contributor Author

klml commented Mar 21, 2025

@martini

Wouldn't it be better to add a startup probe for that?

fullack

startup probe: short interval, but high failure threshold to make for 30s startup time or more - this way it would complete earlier than 30s in case the stack is faster (e.g. in case of patchlevel update without migrations)

I used high failureThreshold: 10 (default is 3) with short periodSeconds: 4 (default is 10), so I get 40 secs. In this time even rails is running

readiness probe: short interval (perhaps defaults are fine?), no initialDelay needed
liveness probe: same

removed it

This stack works great for my openshift, nginx is running now in 5 secs (before 32) 😁 and rails still needs 30 secs, but better than 45 before, if I give it a full 4 GB of RAM even in 20 seconds.

@mgruner mgruner requested review from monotek and mgruner March 21, 2025 14:00
@mgruner
Copy link
Collaborator

mgruner commented Mar 21, 2025

Looks good at first sight! Will try it.

Copy link
Collaborator

@mgruner mgruner left a comment

Choose a reason for hiding this comment

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

👏

@mgruner mgruner merged commit c5d0fa7 into zammad:main Mar 26, 2025
8 checks passed
@mgruner
Copy link
Collaborator

mgruner commented Mar 26, 2025

Thanks a lot @klml!

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.

2 participants