aaron-y-chen commented on code in PR #75945:
URL: https://github.com/apache/airflow/pull/75945#discussion_r4495998549
##########
chart/templates/dag-processor/dag-processor-deployment.yaml:
##########
@@ -21,6 +21,23 @@
## Airflow Dag Processor Deployment
#################################
{{- if .Values.dagProcessor.enabled }}
+{{- $globals := deepCopy . -}}
+{{- $dagProcessorSets := .Values.dagProcessor.sets | default list -}}
+{{- $enableDefault := .Values.dagProcessor.enableDefault -}}
+{{- if kindIs "invalid" $enableDefault -}}
+ {{- $enableDefault = not .Values.dagProcessor.sets -}}
+{{- end -}}
+{{- if $enableDefault -}}
+ {{- $dagProcessorSets = concat (list (dict "name" "default"))
$dagProcessorSets -}}
+{{- end -}}
+{{- range $dagProcessorSet := $dagProcessorSets -}}
+ {{- if hasKey $dagProcessorSet "dagBundleConfigList" -}}
+ {{- fail "`dagProcessor.sets` items cannot define `dagBundleConfigList`;
define all bundles in `dagProcessor.dagBundleConfigList` and select them per
set with `bundleNames`" -}}
+ {{- end -}}
+ {{- $dagProcessor := include "workersMergeValues" (list
$.Values.dagProcessor $dagProcessorSet) | fromYaml -}}
Review Comment:
A set can override `serviceAccount: {create: true, name: team-a}`, but the
ServiceAccount template only reads the parent configuration. The Deployment
therefore references an account the chart never creates, preventing Pod
creation unless it already exists. Please handle these overrides consistently
across both templates and add a regression test checking that the referenced
account is created.
##########
chart/templates/dag-processor/dag-processor-poddisruptionbudget.yaml:
##########
@@ -40,5 +55,11 @@ spec:
tier: airflow
component: dag-processor
release: {{ .Release.Name }}
+ {{- if ne .Values.dagProcessor.name "default" }}
Review Comment:
With `enableDefault: true` and PDBs enabled, the default PDB also selects
the named sets' Pods. Those Pods then match two PDBs, causing eviction failures
during node drains. Please make the selectors disjoint and add a regression
test verifying which Pods each PDB selects.
##########
chart/templates/dag-processor/dag-processor-deployment.yaml:
##########
@@ -32,10 +49,28 @@
{{- $containerSecurityContextWaitForMigrations := include
"containerSecurityContext" (list .Values.dagProcessor.waitForMigrations
.Values) }}
{{- $containerLifecycleHooks := or
.Values.dagProcessor.containerLifecycleHooks .Values.containerLifecycleHooks }}
{{- $containerLifecycleHooksLogGroomerSidecar := or
.Values.dagProcessor.logGroomerSidecar.containerLifecycleHooks
.Values.containerLifecycleHooks }}
+{{- $args := .Values.dagProcessor.args }}
+{{- with .Values.dagProcessor.bundleNames }}
+ {{- if not $args }}
+ {{- fail "`dagProcessor.bundleNames` requires `dagProcessor.args` to be
set" }}
+ {{- end }}
+ {{- $definedBundles := list }}
+ {{- range $.Values.dagProcessor.dagBundleConfigList }}
+ {{- $definedBundles = append $definedBundles .name }}
+ {{- end }}
+ {{- $bundleFlags := list }}
+ {{- range . }}
+ {{- if not (has . $definedBundles) }}
+ {{- fail (printf "Bundle %q is not defined in
`dagProcessor.dagBundleConfigList`" .) }}
+ {{- end }}
+ {{- $bundleFlags = append $bundleFlags (printf "--bundle-name %s" (squote
.)) }}
+ {{- end }}
+ {{- $args = append (initial $args) (printf "%s %s" (last $args) (join " "
$bundleFlags)) }}
Review Comment:
This assumes the last argument is a shell command. With `command:
["airflow"]` and `args: ["dag-processor"]`, adding `bundleNames` produces one
argument containing `"dag-processor --bundle-name 'bundle1'"`, so startup
fails. Please preserve direct argv usage and add a regression test combining
custom command/args with bundle selection.
--
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]
