sdaberdaku commented on code in PR #76749:
URL: https://github.com/apache/airflow/pull/76749#discussion_r4522533176
##########
chart/values.schema.json:
##########
@@ -1747,6 +1747,14 @@
"items": {
"type": "string"
}
+ },
+ "terminationGracePeriodSeconds": {
+ "description": "Grace period, in seconds,
for a kill triggered by this probe. Overrides the pod's
`terminationGracePeriodSeconds` for liveness kills only.",
+ "type": [
+ "integer",
+ "null"
+ ],
+ "default": null
Review Comment:
Looking at this again, I would rather keep `null` here. It is how the chart
marks optional values (`livenessProbe.command` right above is `["array",
"null"]` with `default: null`), and the parameters reference only lists a
property that has a `default`, so dropping it would hide the setting from the
docs.
What I did add is `"minimum": 1`: Kubernetes rejects a probe-level grace
period below 1, and the template was silently dropping a `0`, so the pod-level
period applied while the value looked set. Now the install fails instead.
If you would still prefer no `null`, the alternative I see is a real default
(e.g. 600, same as the pod-level one), but that changes behaviour for anyone
with a longer pod grace period. Happy to go that way if you think it is the
better default.
##########
chart/values.yaml:
##########
@@ -646,6 +646,12 @@ workers:
failureThreshold: 5
periodSeconds: 60
command: ~
+ # Grace period for a kill triggered by this probe. Unset, the pod's
+ # `terminationGracePeriodSeconds` applies, which workers often set to
hours
+ # so that a rolling update drains running tasks; a worker killed because
it
+ # stopped answering is not draining anything, so a shorter period here
+ # restarts it sooner.
Review Comment:
To answer the question more directly: a failing probe does not mean nothing
is running. The probe is `celery inspect ping`, which only tells us the main
worker process stopped answering on the control channel. The pool children can
still be running tasks, or be stuck in them.
That is what we hit: after a Redis failover three workers stayed connected
but stopped consuming and stopped answering pings. The liveness probe fired,
SIGTERM started a warm shutdown, and the warm shutdown waited on two hung
tasks. With a 3 h pod grace period the containers stayed "Running" with no
consumer until we deleted the pods by hand.
The comment in `values.yaml` was misleading on this point ("is not draining
anything"), I reworded it in the last commit. The trade-off is that tasks still
running on a worker killed by its liveness probe get SIGKILLed once the shorter
period runs out, which is why it stays unset by default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
