Probe fixes. #1520
Probe fixes. #1520
Conversation
…e found k8s complaining about having a handler type defined out of HTTP, TCP, Exec and gRPC.
abhishekbhakat
force-pushed
the
helm-chart-v1-15-0-astro-probe-fixes
branch
from
084c9e7 to
d631c1b
Compare
| {{- if .Values.dagProcessor.readinessProbe }} | ||
| readinessProbe: | ||
| initialDelaySeconds: {{ .Values.dagProcessor.readinessProbe.initialDelaySeconds }} | ||
| timeoutSeconds: {{ .Values.dagProcessor.readinessProbe.timeoutSeconds }} | ||
| failureThreshold: {{ .Values.dagProcessor.readinessProbe.failureThreshold }} | ||
| periodSeconds: {{ .Values.dagProcessor.readinessProbe.periodSeconds }} | ||
| exec: | ||
| command: | ||
| {{- if .Values.dagProcessor.readinessProbe.command }} | ||
| {{- toYaml .Values.dagProcessor.readinessProbe.command | nindent 16 }} | ||
| {{- else }} | ||
| {{- include "dag_processor_readiness_check_command" . | indent 14 }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
IMHO the way we did it in astronomer is way easier to maintain and use, other than needing to reference the values.yaml file to see the default probe:
There was a problem hiding this comment.
Nah! I think in Airflow chart there is a standard being followed. No yaml has default values hardcoded. All defaults comes from values.schema.json. Only exec.command is sometimes mentioned in the template yamls. Let's keep it that for the time being, perhaps?
There would be too many places to change. And would make more work for future oss airflow charts support.
|
Would you mind adding tests as well?? @abhishekbhakat |
The current tests use chart's
If I go with option 1, I have to fix all other failures in the test. An example error in tests is below (this has nothing to do with my PR): |
| {{- else }} | ||
| - sh | ||
| - -c | ||
| - "test -f /clean-logs && grep -q 'AIRFLOW__LOG_RETENTION_DAYS' /clean-logs" |
There was a problem hiding this comment.
in our astro runtime we are using ASTRONOMER__AIRFLOW__WORKER_LOG_RETENTION_DAYS as a env vars - above might fail right
There was a problem hiding this comment.
Once this PR is merged we have to override this value in astronomer airflow chart to override this change
There was a problem hiding this comment.
So it's a different clean logs script?
airflow/scripts/docker/clean-logs.sh
Line 23 in 1ba7a69
Can we add that ENV in this repo/branch? If we can, then let's just override the script with the one that we use?
There was a problem hiding this comment.
@abhishekbhakat yes, it is a different script https://github.com/astronomer/astro-runtime/blob/80a47156fa/image/bin/clean-airflow-logs#L6
Some probes have
execinstead of directcommandas we found k8s complaining about having a handler type defined out of HTTP, TCP, Exec and gRPC.Respective schemas have been updated.
We found these specific configurations working in our environment. Additionally also have a customer validation.