You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "gerkElznik (via GitHub)" <gi...@apache.org> on 2026/06/02 00:26:40 UTC
[PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
gerkElznik opened a new pull request, #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126
## What is the purpose of the change
The Helm chart advertises `defaultConfiguration.config.yaml` (added in FLINK-38409), but a downstream consumer who sets it never gets it mounted — the operator silently runs on `flink-conf.yaml` built from chart defaults, while the user's `config.yaml` content lands in the `flink-operator-config` ConfigMap but is never read by the operator JVM.
Three interacting causes:
1. `values.yaml` hardcodes a non-empty `defaultConfiguration.flink-conf.yaml`. Helm's additive merge means a downstream `-f my-values.yaml` cannot unset it, so `hasKey "flink-conf.yaml"` is always true.
2. `templates/controller/configmap.yaml` unconditionally emits **both** `config.yaml` and `flink-conf.yaml` keys.
3. `templates/controller/deployment.yaml` selects the mounted file with an **independent** `hasKey "flink-conf.yaml"` — always true — so the `config.yaml` branch is unreachable.
This PR resolves the file choice in the chart template and emits/mounts a single, statically named `config.yaml`. The approach was discussed and agreed on the JIRA ticket.
### Why one always-`config.yaml` file, appended from a flat seed
The chart seed carries the **mandatory Java-17 `--add-opens`/`--add-exports` opts**, which must apply no matter which format the user supplies. So the seed is **prepended** to the resolved override — the same "append" model `log4j-*.properties` already uses — rather than **replaced** by it (the model `logback-*.xml` uses, since two XML documents can't be concatenated). It cannot use the replace model, because that would drop the Java-17 opts for anyone who sets their own `config.yaml`.
Because the mounted result is parsed by Flink's strict `config.yaml` parser (snakeyaml-engine, `allowDuplicateKeys=false`), an appended **flat** user override of a key that is also in the seed would be a duplicate-key crash. Two consequences, both reflected here:
- the seed is kept **flat** (a nested seed would instead collide with a *nested* user override, which is the recommended form); and
- the two redundant seed keys `taskmanager.numberOfTaskSlots: 1` / `parallelism.default: 1` are dropped — they only restate Flink's compiled default of `1`, so removing them is behavior-neutral while shrinking the collision surface.
## Brief change log
- **`configmap.yaml`** — emit a single config-file key, always named `config.yaml`, resolved with a nested `{{ with (index … "config.yaml") }} … {{ else }}{{ with (index … "flink-conf.yaml") }} … {{ end }}{{ end }}` (plain `with`/`else`, so it renders on every Helm version): the user's `config.yaml` wins, else their `flink-conf.yaml`, else the `values.yaml` default. Keys off value truthiness rather than `hasKey` (which downstream can't unset), consistent with the chart's existing `log4j`/`logback` blocks.
- **`deployment.yaml`** — mount the key statically at `path: config.yaml`. Emit and mount are both literally `config.yaml`, so they can't drift; `config.yaml` is also the only filename Flink 2.x reads, so default installs keep booting after the operator's eventual 2.x rebase.
- **`conf/flink-conf.yaml` → `conf/config.yaml`** (renamed) — the always-prepended seed, named to match what it is mounted as; kept flat; the two redundant `: 1` keys removed (see above).
- **`values.yaml`** — restructured the showcase comment into two mutually-exclusive format examples (modern `config.yaml` / legacy `flink-conf.yaml`) with precedence + mount notes; opinionated defaults stay here (not relocated into the always-applied seed).
- **docs** (`operations/configuration.md`, `operations/helm.md`, `development/guide.md`) — document the `config.yaml` format and precedence, and fix the now-stale `/opt/flink/conf/flink-conf.yaml` inspect path.
## Verifying this change
This change added tests and can be verified as follows:
- **helm-unittest matrix** (`tests/controller/{configmap,deployment}_test.yaml`): exactly one config-file key, always `config.yaml` (never `flink-conf.yaml`), carrying today's effective defaults with the two dropped seed keys absent; a user `flink-conf.yaml` still flows in when `config.yaml` is unset; `config.yaml` wins when both are set; and a flat override of a former seed key no longer duplicates it. The Deployment mounts the key statically as `config.yaml`. (Values are injected via structured `set:`; a dotted-string `set:` no-ops on keys containing dots.)
- **Parser load test**: each matrix row's rendered `config.yaml` was loaded through a strict, no-duplicate-key YAML parser equivalent to Flink's `config.yaml` loader, with a positive control confirming the check rejects a genuine duplicate. All rows parse clean.
- **In a kind cluster**: installed the chart and confirmed the operator loads `/opt/flink/conf/config.yaml` via Flink's standard YAML parser — with scalar, nested-map, and list values — that the ConfigMap carries a single `config.yaml` key, and that the operator reconciles FlinkDeployments with both flat and nested `flinkConfiguration`. `ct lint` against the chart-testing image (Helm 3.16.4) confirms the resolver renders on older Helm too.
- `helm lint` and a full `helm template` render pass.
- Effective-config equivalence for a default install: the mounted payload is unchanged except the two dropped keys, which resolve to Flink's compiled default of `1` either way.
## Does this pull request potentially affect one of the following parts:
- Dependencies (add or upgrade a dependency): **no**
- The public API (`CustomResourceDescriptors`): **no**
- Core observer or reconciler logic that is regularly executed: **no** (Helm chart + docs only)
## Documentation
- Does this introduce a new feature? **No** — it makes the existing `defaultConfiguration.config.yaml` work as advertised.
- How is it documented? **docs** — `operations/configuration.md` and `operations/helm.md`.
---
## Notes for reviewers (call-outs / open questions)
1. **Precedence flip.** When both keys are set, `config.yaml` now wins; the old `hasKey` behavior preferred `flink-conf.yaml`. Worth a release note.
2. **ConfigMap shape change.** `flink-operator-config` now carries one config-file key instead of two — visible to anyone reading that ConfigMap directly.
3. **Coordination with FLINK-39571.** That ticket's "known limitation" note on the configuration page isn't on `main` yet; if it lands first, I'll rebase and remove it.
4. **Residual collision surface.** A *flat* `config.yaml` override of the two remaining `kubernetes.operator.default-configuration.flink-version.*` Java-17 seed keys could still duplicate-key. It's rare (they're rarely flat-overridden, and the recommended nested form is collision-free). The clean long-term fix is shipping framework defaults as compiled `ConfigOption` defaults rather than a text-prepended seed — proposed as a separate follow-up.
5. **Chinese docs.** The `.zh` translations (incl. the `guide.md` inspect-path line) are intentionally left for the project's separate Chinese-sync pass.
6. **Backports?** Which active `release-*` branch(es) should this target?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-4938701283
Quick follow-up: two points in my comment above were from code tracing only, so I runtime-tested both in the same kind setup (webhook enabled via cert-manager, namespaced RBAC via `watchNamespaces`).
- **Webhook + env var**: the webhook container does honor `WATCH_NAMESPACES` through the same config path. With the var on the operator container only (and no ConfigMap line), the webhook informers stayed cluster-scoped and the first `FlinkSessionJob` apply failed admission with `InternalError ... cannot list flinkdeployments at the cluster scope`. The informers are created lazily, so nothing fails at startup; it surfaces on the first session job. With the var on both containers, admission worked normally. So the follow-up would just need to set it on both.
- **Dynamic namespaces**: with the env var unset and `kubernetes.operator.dynamic.namespaces.enabled: true`, a live ConfigMap edit rescoped all three controllers and the webhook informers with no restart. With the env var set, the same edit reloads the config (the log even prints the new namespace list) but the watch never changes and nothing warns about it; a FlinkDeployment created in the newly added namespace was never reconciled. So I'd want the precedence behavior to be a deliberate, documented call in the follow-up.
Neither changes the options above; just confirming the details for whoever picks up the cleanup ticket.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "bkinzle (via GitHub)" <gi...@apache.org>.
bkinzle commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3548776847
##########
docs/content/docs/operations/helm.md:
##########
@@ -71,9 +71,9 @@ The configurable parameters of the Helm chart and which default values as detail
| Parameters | Description | Default Value |
|------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| defaultConfiguration.append | Whether to append configuration files with configs. | true |
-| defaultConfiguration.config.yaml | The newer configuration file format for flink that will enforced in Flink 2.0. Note this was introduced in flink 1.19. | kubernetes.operator.metrics.reporter.slf4j.factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory<br/>kubernetes.operator.metrics.reporter.slf4j.interval: 5 MINUTE<br/>kubernetes.operator.reconcile.interval: 15 s<br/>kubernetes.operator.observer.progress-check.interval: 5 s |
+| defaultConfiguration.config.yaml | Modern YAML 1.2 config format (introduced in Flink 1.19, required by Flink 2.0). Takes precedence over flink-conf.yaml when set; the resolved config is always mounted as config.yaml. | kubernetes.operator.metrics.reporter.slf4j.factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory<br/>kubernetes.operator.metrics.reporter.slf4j.interval: 5 MINUTE<br/>kubernetes.operator.reconcile.interval: 15 s<br/>kubernetes.operator.observer.progress-check.interval: 5 s |
Review Comment:
done
##########
helm/flink-kubernetes-operator/tests/controller/configmap_test.yaml:
##########
@@ -68,3 +68,66 @@ tests:
asserts:
- isNotNull:
path: data["log4j-console.properties"]
+
+ # FLINK-39791: the ConfigMap emits a single config-file key, always named config.yaml,
+ # resolved as config.yaml (when set) > user flink-conf.yaml > the values.yaml default.
Review Comment:
done
##########
helm/flink-kubernetes-operator/tests/controller/deployment_test.yaml:
##########
@@ -383,3 +383,18 @@ tests:
key: log4j-operator.properties
path: log4j-operator.properties
+# FLINK-39791: the operator config is always mounted as config.yaml (the only filename Flink 2.x
+# reads), regardless of which defaultConfiguration key the user populates.
Review Comment:
done
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "Dennis-Mircea (via GitHub)" <gi...@apache.org>.
Dennis-Mircea commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3750894004
##########
docs/content.zh/docs/deployment/configuration.md:
##########
@@ -51,10 +51,28 @@ defaultConfiguration:
### YAML Configuration File Format
-Operator configuration is delivered through the `defaultConfiguration.flink-conf.yaml` block in the Helm `values.yaml`. The operator's runtime loads that file with Flink's legacy configuration parser, which accepts only flat key-value pairs written with dot notation and does not support nested YAML maps.
+Operator configuration can be delivered in either of Flink's two configuration formats, by defining the matching key under `defaultConfiguration` in the Helm `values.yaml`:
+
+- `flink-conf.yaml` is the legacy format, which accepts only flat key-value pairs written with dot notation and does not support nested YAML maps. This is the format the chart ships by default.
+- `config.yaml` is the YAML 1.2 format defined from Flink 1.19 onward and the only format Flink 2.0 accepts. It supports nested keys (see the [Flink Configuration File](https://nightlies.apache.org/flink/flink-docs-release-2.2/docs/deployment/config/#flink-configuration-file) documentation):
Review Comment:
Please reference here the Flink documentation master, not on a concrete release (e.g. `2.2`).
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-4771813362
> @gerkElznik the e2es seem to be failing
## Heads-up before going further: this regresses Flink 1.x application deployments
While tracking down the failing `test_application_operations.sh` smoke test, I found that switching the operator to mount `config.yaml` breaks **Flink 1.x application-mode** deployments. I think it reshapes this PR, so I wanted to lay it out.
### Symptom
A `v1_xx` application deployment's JobManager crashloops at startup:
```
ERROR ClusterEntrypoint - Could not create application program.
java.net.URISyntaxException: Illegal character in scheme name at index 0:
['local:///opt/flink/usrlib/myjob.jar']
at KubernetesApplicationClusterEntrypoint.fetchArtifacts(…) [flink-dist-1.20.4.jar]
```
### Isolated with an A/B
Same operator image (built from this branch), same kind cluster, same `v1_20`
`StateMachineExample` — the only difference is the chart's mounted config file:
| | mounts `config.yaml` (this PR) | mounts `flink-conf.yaml` (main) |
|---|---|---|
| operator config mode | standard-YAML | legacy |
| operator writes to the JM ConfigMap | `pipeline.jars: ['local:///…/myjob.jar']` | `pipeline.jars: local:///…/myjob.jar` |
| `v1_20` JobManager | URISyntaxException → CrashLoopBackOff | READY / RUNNING |
### Root cause
`FlinkConfMountDecorator#getClusterSideConfData` (the operator's copy, from FLINK-37236):
```java
// … otherwise it would simply inherit it from the base config (which would always be false currently).
Configuration clusterSideConfig = new Configuration(useStandardYamlConfig()); // false for v1_x → flink-conf.yaml ✓
clusterSideConfig.addAll(flinkConfig); // copies the operator's global-config values
return ConfigurationUtils.convertConfigToWritableLines(clusterSideConfig, false);
```
useStandardYamlConfig() correctly keys off the deployment's FlinkVersion, but addAll(flinkConfig) copies values from the operator's global config — and the comment assumes that global config is "always false" (legacy). Once the operator reads config.yaml, its global config is standardYaml=true, so list options (pipeline.jars, pipeline.classpaths, …) carry standard-YAML form and get written into the v1_x flink-conf.yaml as ['local://…'], which the legacy parser rejects. So it's a pre-existing operator assumption that this PR is simply the first to expose — and it affects any list-valued option on any Flink-1.x deployment, not only pipeline.jars.
How would you like to sequence this @gyfora? Tagging @Dennis-Mircea as well since he's mentioned planning for the overall 2.x upgrade.
1. Fix FlinkConfMountDecorator to serialize per the target deployment's YAML mode (downgrade list values when useStandardYamlConfig() is false) as a prerequisite, then land this chart change; or
2. Hold the config.yaml mount (this change) until the operator no longer manages Flink-1.x deployments.
Happy to help with either, including the operator-side code change.
_Two smaller notes, same root (single config.yaml key):_
_- Re your inline question on the removed lines: the watchNamespaces/operatorHealth injections aren't dropped, they were emitted once per config-file key (so twice), and the dedup keeps one copy (still just below the resolver; helm template shows them rendering once). The hasKey → with swap is the actual fix, and the flink-conf.yaml key is removed to emit a single file._
_- e2e-tests/test_dynamic_flink_conf.sh patches the flink-conf.yaml ConfigMap key, which is no longer mounted; its twin test_dynamic_config.sh already covers the same path via config.yaml, so it'd need updating/removing — but that's moot until the above is settled._
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "bkinzle (via GitHub)" <gi...@apache.org>.
bkinzle commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-4921407192
Thanks for the feedback @Dennis-Mircea! Inline comments are all addressed in the latest commits on both PRs (including the Chinese docs sync).
On the broader cleanup, I like the destination a lot, and I think it's the same end-state my note (4) above was gesturing at. `config.yaml` containing only user-supplied content seems like the right place for this to eventually land. Before forming an opinion on where the work should happen, I tried the pieces out in a kind cluster to check my own understanding, and I'd summarize what I saw as:
- `WATCH_NAMESPACES` works exactly as you said: setting the env on the operator deployment scoped the watch with no operator change needed; the chart just doesn't set it today. The health-probe side looked different in my test though; nothing seems to read the `KUBERNETES_OPERATOR_HEALTH_PROBE_*` env vars currently (I set them and the operator kept `true`/`8085` from the file), so I suspect that half would need a small amount of new operator code. I may be missing something though.
- Dropping the values.yaml overrides without touching the compiled defaults does change behavior -- I measured the reconcile cadence going from 15.0s to 60.1s on a default install. So if I'm reading it right, either the compiled defaults change to 15s/5s (which would also affect non-Helm users), or default installs slow down 4x and probably want a release note. I'm not sure which is the better call, which is partly why I wonder if it deserves its own ticket where that tradeoff can get proper visibility.
- For the Java-17 opts migration I'm less sure of the shape... it seems to involve deciding where per-Flink-version defaults live in operator code, and I don't have a strong sense yet of the right home for that.
The way I see it there are two options:
1. Keep this PR at its current scope (it fixes the user-facing bug on its own), and do the cleanup as a dedicated follow-up ticket implementing what you describe.
2. Fold the cleanup into this PR and reshape it around the broader fix.
I'll defer to your and @gyfora's call on which path to take. If it goes the follow-up route, I'm glad to file the ticket and take it on.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3430434439
##########
helm/flink-kubernetes-operator/templates/controller/configmap.yaml:
##########
@@ -25,24 +25,14 @@ metadata:
data:
config.yaml: |+
{{- if .Values.defaultConfiguration.append }}
- {{- $.Files.Get "conf/flink-conf.yaml" | nindent 4 -}}
+ {{- $.Files.Get "conf/config.yaml" | nindent 4 -}}
{{- end }}
-{{- if hasKey (.Values.defaultConfiguration) "config.yaml" }}
- {{- index (.Values.defaultConfiguration) "config.yaml" | nindent 4 -}}
-{{- end }}
-{{- if .Values.watchNamespaces }}
- kubernetes.operator.watched.namespaces: {{ join "," .Values.watchNamespaces }}
-{{- end }}
-{{- if index .Values "operatorHealth" }}
- kubernetes.operator.health.probe.enabled: true
- kubernetes.operator.health.probe.port: {{ .Values.operatorHealth.port }}
-{{- end }}
- flink-conf.yaml: |+
-{{- if .Values.defaultConfiguration.append }}
- {{- $.Files.Get "conf/flink-conf.yaml" | nindent 4 -}}
-{{- end }}
Review Comment:
Good question — there are three separate edits in that hunk:
1. **The `watchNamespaces` / `operatorHealth` injections aren't actually dropped.**
The old template emitted *two* config-file keys (`config.yaml` **and**
`flink-conf.yaml`) and appended both blocks to **each**, so they rendered twice.
Collapsing to a single `config.yaml` key removes the duplicate — one copy still
renders just below the resolver (lines 37–43); `helm template` confirms
`kubernetes.operator.health.probe.*` and the `watched.namespaces` line still
emit, once.
2. **The `hasKey "config.yaml"` block is replaced by the `with`-based resolver
above it.** That's the core of the ticket: `hasKey` keys off presence, and since
the chart hardcodes a non-empty `defaultConfiguration.flink-conf.yaml` that a
downstream `-f values.yaml` can't unset, the `config.yaml` branch was
unreachable. `with` keys off value-truthiness (like the existing `log4j`/
`logback` blocks), so the user's `config.yaml` actually wins.
3. **The `flink-conf.yaml:` key is removed so the ConfigMap emits a single,
always-`config.yaml` file** (the only name Flink 2.x reads), mounted statically.
The one behavioral consequence of (3) I'd like your read on before acting: runtime
patching of the operator ConfigMap now has to target the `config.yaml` key. In
particular `e2e-tests/test_dynamic_flink_conf.sh` patches the `flink-conf.yaml`
key and would no longer be picked up — its twin `test_dynamic_config.sh` already
covers the same dynamic-reload path via `config.yaml`. I'm inclined to drop
`test_dynamic_flink_conf.sh` (and its two `ci.yml` matrix rows) plus add a short
migration note, but since that removes legacy coverage I'd rather confirm with you
first.
(Aside, unrelated to this hunk: I've rebased onto latest `main` — the earlier
`test_application_operations.sh` smoke failure was a pipeline.jars/Flink 1.20.4
app-mode error on a stale base.)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "bkinzle (via GitHub)" <gi...@apache.org>.
bkinzle commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3548777795
##########
helm/flink-kubernetes-operator/values.yaml:
##########
@@ -174,14 +174,27 @@ defaultConfiguration:
# This option has not effect, if create is equal to false.
append: true
-# Uncomment to use the new config.yaml format, but also comment out the flink-config.yaml key.
+# Operator configuration overrides go under ONE of the two keys below. If both are set,
+# config.yaml wins; either way the operator mounts the result as config.yaml.
+#
+# config.yaml - modern YAML 1.2 format (Flink 1.19+, required by Flink 2.0). Recommended.
+# flink-conf.yaml - legacy flat "key: value" format (shipped as the default below).
+#
+# Prefer the nested config.yaml form: a flat, dotted-key override of a key the chart already
+# ships (e.g. the Java 17 opts in conf/config.yaml) would be a duplicate key under Flink's
+# strict YAML parser and fail at operator startup. Example, equivalent to the default below:
+#
# config.yaml: |+
# kubernetes.operator:
# metrics.reporter.slf4j:
# factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory
# interval: 5 MINUTE
# reconcile.interval: 15 s
# observer.progress-check.interval: 5 s
+# taskmanager:
+# numberOfTaskSlots: 1 # default for managed clusters; override per-job via spec.flinkConfiguration
Review Comment:
done
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "Dennis-Mircea (via GitHub)" <gi...@apache.org>.
Dennis-Mircea commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-5214387557
@gerkElznik Could you resolve the conflicts and update the docs to match the new documentation structure? That restructure landed before this PR.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-5248996579
@gyfora After really thinking through your comment about `CONF_OVERRIDE_DIR`, I went looking for other places where configuration arrives in the old shape from outside the chart. The override directory itself holds up, as covered above, but I found two things that would break on upgrade and I would rather put them in front of you now than have someone discover them later.
**1. The legacy values block is now parsed strictly.** Content under `defaultConfiguration.flink-conf.yaml` used to be read by Flink's legacy line parser and is now read as YAML 1.2. Running both parsers from flink-core 1.20.4 over the same text:
| content | `flink-conf.yaml` (before) | `config.yaml` (after) |
|---|---|---|
| `kubernetes.operator.exception.label.mapper: Task did not exit: hung` | loads | `Error parsing YAML configuration` |
| the same key twice | loads, last one wins | `Error parsing YAML configuration` |
So a value containing a colon followed by a space, or a duplicated key, now stops the operator at startup. The chart's own defaults are clean, which is why CI and my cluster runs are green. What gives me some pause is that `kubernetes.operator.user.artifacts.http.header` and `kubernetes.operator.exception.label.mapper` are both map options whose natural values contain `: `. The user-side fix is to quote the value or move to the nested form, but they would find that out from a crashlooping operator.
**2. A self-managed operator ConfigMap can stop the pod from starting.** With `defaultConfiguration.create: false` the Deployment still mounts `flink-operator-config` through an explicit item list, and that list changes from `flink-conf.yaml` to `config.yaml`. In kind, with a hand-written ConfigMap carrying only the keys the chart on main mounts:
- this branch: `MountVolume.SetUp failed ... configmap references non-existent config key: config.yaml`, pod stuck in ContainerCreating
- main, same ConfigMap: 1/1 Running
- this branch, ConfigMap that also has a `config.yaml` key: 1/1 Running
Anyone who copied the ConfigMap as the chart on main renders it is unaffected, since that output already carries `config.yaml` alongside `flink-conf.yaml`. Only a hand-minimized ConfigMap that matches just the mounted items is affected.
**What could eliminate both.** The chart could keep emitting and mounting `flink-conf.yaml` when only the legacy values key is set, and use `config.yaml` only when the user sets that key. That would be zero breakage: legacy users keep the legacy file and the legacy parser, and the reported bug is still fixed because a user-supplied `config.yaml` finally gets mounted.
The reason I am not proposing it is the history here. A mount-time selector between the two file names is what caused the original bug: `hasKey "flink-conf.yaml"` is always true while the chart ships a non-empty default, so the `config.yaml` branch was unreachable. Bringing a selector back reintroduces that shape, and it gives up the property Dennis called load-bearing when we agreed the approach, that `config.yaml` is the only name Flink 2.x reads, so a single static name keeps default installs booting after the operator's eventual rebase.
My inclination is to keep this PR as it stands and treat both items as upgrade notes. I do not have a feel for how this project prefers to surface that, whether it belongs in release notes, in `docs/operations/upgrade.md`, or both, so I will leave the shape of it to you. Happy to write whatever you would like, and equally happy to switch to the conditional mount if you would rather have zero breakage.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-4938361057
Thanks @Dennis-Mircea! Inline comments are all addressed in the latest commits on both PRs (including the Chinese docs sync).
Before responding to the cleanup proposal I tried each piece in a kind cluster and traced the code paths, to check my own understanding. Taking your three items in order:
**1. Dropping the SLF4J reporter and interval overrides from values.yaml**
Confirmed the compiled defaults are 60s (`kubernetes.operator.reconcile.interval`) and 10s (`kubernetes.operator.observer.progress-check.interval`), and that both are currently dead in Helm installs since the chart always overrides them. Dropping the overrides without touching the compiled defaults does change default-install behavior though: I measured the reconcile cadence going from 15.0s to 60.1s, and the progress check would go from 5s to 10s (slower observation of in-progress deployments and savepoints). So the choice is between changing the compiled defaults to 15s/5s (which also affects non-Helm deployments, the generated docs, and idle load on the API server and job REST endpoints) or letting default installs slow down with a release note. I don't have a strong opinion on which, but it feels like a decision that deserves its own visibility.
One consistency note: dropping the SLF4J reporter is also a behavior change on default installs, since operator metrics stop being logged every 5 minutes and anyone scraping or alerting on those lines would notice. I agree it should be user choice going forward; I'd just put it in the same release note.
**2. Migrating the Java-17 opts to compiled defaults**
I think this is the right destination, and it's the same end-state note 4 above was gesturing at. Two design questions the follow-up would need to answer, since the per-version keys are dynamic string prefixes resolved at runtime (`FlinkConfigManager.getRelevantVersionPrefixes`) and can't be plain `ConfigOption` defaults:
- Where the compiled seed gets injected decides precedence; user config needs to keep winning over it.
- Today `defaultConfiguration.append: false` produces a config without the Java-17 opts entirely. Compiled defaults can't be shed by omitting text, so the migration probably needs an explicit off-switch to preserve that escape hatch.
**3. watchNamespaces and operatorHealth as env vars**
`WATCH_NAMESPACES` works on the operator side exactly as you said: I set it on the operator container in kind and the watch scoped correctly with no code change. Two wrinkles I found while tracing the full path:
- The webhook consumes the watched namespaces too (`FlinkOperatorWebhook` scopes its informers through the same config path), and it runs as a separate container with its own env block. The chart would need to set the var on both containers; otherwise the webhook informers stay cluster-scoped, which fails under the namespaced RBAC that setting `watchNamespaces` switches on.
- The env var takes precedence over the config file, so shipping namespaces via env silently disables `kubernetes.operator.dynamic.namespaces.enabled` (no-restart namespace changes via ConfigMap edit). That might be acceptable, since an env change through helm upgrade forces a rolling restart, which is arguably more correct than today's silently stale config. But it seems worth a deliberate call in the follow-up.
For the health probe: nothing reads `KUBERNETES_OPERATOR_HEALTH_PROBE_*` today. Those names don't exist in the tree (I checked `EnvUtils` and `OperatorHealthService`), so that half is a small amount of new operator code mirroring `ENV_WATCH_NAMESPACES`. Notably it's only needed to keep `operatorHealth.port` overrides and disablement working: for a default install the compiled defaults (true/8085) already match what the chart writes, so dropping those two ConfigMap lines is behavior-neutral there. The Helm value itself stays either way, since it also wires the containerPort and the liveness/startup probes.
**On sequencing**
Landing #1152 first makes sense to me regardless. For this PR, my thinking is that the cleanup and the fix here are complementary rather than alternatives: the cleanup removes the collision surface, but even with an empty seed and env vars the chart still needs this PR's template changes to emit and mount the user's `config.yaml` (the always-true `hasKey` is what makes that branch unreachable today). That makes me lean toward landing this PR at its current scope and doing the cleanup as a dedicated follow-up ticket, where the compiled-defaults question can get proper visibility. Happy to fold it into this PR instead if you and @gyfora prefer; if it goes the follow-up route, I'm glad to file the ticket and take it on.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "Dennis-Mircea (via GitHub)" <gi...@apache.org>.
Dennis-Mircea commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3542291001
##########
helm/flink-kubernetes-operator/tests/controller/configmap_test.yaml:
##########
@@ -68,3 +68,66 @@ tests:
asserts:
- isNotNull:
path: data["log4j-console.properties"]
+
+ # FLINK-39791: the ConfigMap emits a single config-file key, always named config.yaml,
+ # resolved as config.yaml (when set) > user flink-conf.yaml > the values.yaml default.
Review Comment:
Comment not needed.
##########
helm/flink-kubernetes-operator/tests/controller/deployment_test.yaml:
##########
@@ -383,3 +383,18 @@ tests:
key: log4j-operator.properties
path: log4j-operator.properties
+# FLINK-39791: the operator config is always mounted as config.yaml (the only filename Flink 2.x
+# reads), regardless of which defaultConfiguration key the user populates.
Review Comment:
Comment not needed.
##########
helm/flink-kubernetes-operator/values.yaml:
##########
@@ -174,14 +174,27 @@ defaultConfiguration:
# This option has not effect, if create is equal to false.
append: true
-# Uncomment to use the new config.yaml format, but also comment out the flink-config.yaml key.
+# Operator configuration overrides go under ONE of the two keys below. If both are set,
+# config.yaml wins; either way the operator mounts the result as config.yaml.
+#
+# config.yaml - modern YAML 1.2 format (Flink 1.19+, required by Flink 2.0). Recommended.
+# flink-conf.yaml - legacy flat "key: value" format (shipped as the default below).
+#
+# Prefer the nested config.yaml form: a flat, dotted-key override of a key the chart already
+# ships (e.g. the Java 17 opts in conf/config.yaml) would be a duplicate key under Flink's
+# strict YAML parser and fail at operator startup. Example, equivalent to the default below:
+#
# config.yaml: |+
# kubernetes.operator:
# metrics.reporter.slf4j:
# factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory
# interval: 5 MINUTE
# reconcile.interval: 15 s
# observer.progress-check.interval: 5 s
+# taskmanager:
+# numberOfTaskSlots: 1 # default for managed clusters; override per-job via spec.flinkConfiguration
Review Comment:
Not necessary to specify this property here.
##########
docs/content/docs/operations/helm.md:
##########
@@ -71,9 +71,9 @@ The configurable parameters of the Helm chart and which default values as detail
| Parameters | Description | Default Value |
|------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| defaultConfiguration.append | Whether to append configuration files with configs. | true |
-| defaultConfiguration.config.yaml | The newer configuration file format for flink that will enforced in Flink 2.0. Note this was introduced in flink 1.19. | kubernetes.operator.metrics.reporter.slf4j.factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory<br/>kubernetes.operator.metrics.reporter.slf4j.interval: 5 MINUTE<br/>kubernetes.operator.reconcile.interval: 15 s<br/>kubernetes.operator.observer.progress-check.interval: 5 s |
+| defaultConfiguration.config.yaml | Modern YAML 1.2 config format (introduced in Flink 1.19, required by Flink 2.0). Takes precedence over flink-conf.yaml when set; the resolved config is always mounted as config.yaml. | kubernetes.operator.metrics.reporter.slf4j.factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory<br/>kubernetes.operator.metrics.reporter.slf4j.interval: 5 MINUTE<br/>kubernetes.operator.reconcile.interval: 15 s<br/>kubernetes.operator.observer.progress-check.interval: 5 s |
Review Comment:
The chinese docs can be synced with the same english content.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora merged PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-4768042468
@gerkElznik the e2es seem to be failing
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "bkinzle (via GitHub)" <gi...@apache.org>.
bkinzle commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3548777510
##########
helm/flink-kubernetes-operator/tests/controller/deployment_test.yaml:
##########
@@ -383,3 +383,18 @@ tests:
key: log4j-operator.properties
path: log4j-operator.properties
+# FLINK-39791: the operator config is always mounted as config.yaml (the only filename Flink 2.x
+# reads), regardless of which defaultConfiguration key the user populates.
Review Comment:
done
##########
helm/flink-kubernetes-operator/values.yaml:
##########
@@ -174,14 +174,27 @@ defaultConfiguration:
# This option has not effect, if create is equal to false.
append: true
-# Uncomment to use the new config.yaml format, but also comment out the flink-config.yaml key.
+# Operator configuration overrides go under ONE of the two keys below. If both are set,
+# config.yaml wins; either way the operator mounts the result as config.yaml.
+#
+# config.yaml - modern YAML 1.2 format (Flink 1.19+, required by Flink 2.0). Recommended.
+# flink-conf.yaml - legacy flat "key: value" format (shipped as the default below).
+#
+# Prefer the nested config.yaml form: a flat, dotted-key override of a key the chart already
+# ships (e.g. the Java 17 opts in conf/config.yaml) would be a duplicate key under Flink's
+# strict YAML parser and fail at operator startup. Example, equivalent to the default below:
+#
# config.yaml: |+
# kubernetes.operator:
# metrics.reporter.slf4j:
# factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory
# interval: 5 MINUTE
# reconcile.interval: 15 s
# observer.progress-check.interval: 5 s
+# taskmanager:
+# numberOfTaskSlots: 1 # default for managed clusters; override per-job via spec.flinkConfiguration
Review Comment:
done
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-5240434226
@gerkElznik one last question before we merge this. How will this affect the behaviour of the "ENV_CONF_OVERRIDE_DIR" feature that allows users to mount a new configmap with some extra config overrides. That extra configmap may use the old `flink-conf.yaml` format.
Code is at `FlinkConfigManager # 448-462`
We have to make sure that even though the operator config follows the new format (`config.yaml`) we can still load dynamic overrides from an external configmap in the old format. This is essential for operational continuity and we have to explicitly test this. Could you please add a unit test for this?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3425524341
##########
helm/flink-kubernetes-operator/templates/controller/configmap.yaml:
##########
@@ -25,24 +25,14 @@ metadata:
data:
config.yaml: |+
{{- if .Values.defaultConfiguration.append }}
- {{- $.Files.Get "conf/flink-conf.yaml" | nindent 4 -}}
+ {{- $.Files.Get "conf/config.yaml" | nindent 4 -}}
{{- end }}
-{{- if hasKey (.Values.defaultConfiguration) "config.yaml" }}
- {{- index (.Values.defaultConfiguration) "config.yaml" | nindent 4 -}}
-{{- end }}
-{{- if .Values.watchNamespaces }}
- kubernetes.operator.watched.namespaces: {{ join "," .Values.watchNamespaces }}
-{{- end }}
-{{- if index .Values "operatorHealth" }}
- kubernetes.operator.health.probe.enabled: true
- kubernetes.operator.health.probe.port: {{ .Values.operatorHealth.port }}
-{{- end }}
- flink-conf.yaml: |+
-{{- if .Values.defaultConfiguration.append }}
- {{- $.Files.Get "conf/flink-conf.yaml" | nindent 4 -}}
-{{- end }}
Review Comment:
Why are we removing these?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-4921442798
Thanks for the feedback @Dennis-Mircea! Inline comments are all addressed in the latest commits on both PRs (including the Chinese docs sync).
On the broader cleanup, I like the destination a lot, and I think it's the same end-state my note (4) above was gesturing at. config.yaml containing only user-supplied content seems like the right place for this to eventually land. Before forming an opinion on where the work should happen, I tried the pieces out in a kind cluster to check my own understanding, and I'd summarize what I saw as:
WATCH_NAMESPACES works exactly as you said: setting the env on the operator deployment scoped the watch with no operator change needed; the chart just doesn't set it today. The health-probe side looked different in my test though; nothing seems to read the KUBERNETES_OPERATOR_HEALTH_PROBE_* env vars currently (I set them and the operator kept true/8085 from the file), so I suspect that half would need a small amount of new operator code. I may be missing something though.
Dropping the values.yaml overrides without touching the compiled defaults does change behavior -- I measured the reconcile cadence going from 15.0s to 60.1s on a default install. So if I'm reading it right, either the compiled defaults change to 15s/5s (which would also affect non-Helm users), or default installs slow down 4x and probably want a release note. I'm not sure which is the better call, which is partly why I wonder if it deserves its own ticket where that tradeoff can get proper visibility.
For the Java-17 opts migration I'm less sure of the shape... it seems to involve deciding where per-Flink-version defaults live in operator code, and I don't have a strong sense yet of the right home for that.
The way I see it there are two options:
Keep this PR at its current scope (it fixes the user-facing bug on its own), and do the cleanup as a dedicated follow-up ticket implementing what you describe.
Fold the cleanup into this PR and reshape it around the broader fix.
I'll defer to your and @gyfora's call on which path to take. If it goes the follow-up route, I'm glad to file the ticket and take it on.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3548801506
##########
docs/content/docs/operations/helm.md:
##########
@@ -71,9 +71,9 @@ The configurable parameters of the Helm chart and which default values as detail
| Parameters | Description | Default Value |
|------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| defaultConfiguration.append | Whether to append configuration files with configs. | true |
-| defaultConfiguration.config.yaml | The newer configuration file format for flink that will enforced in Flink 2.0. Note this was introduced in flink 1.19. | kubernetes.operator.metrics.reporter.slf4j.factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory<br/>kubernetes.operator.metrics.reporter.slf4j.interval: 5 MINUTE<br/>kubernetes.operator.reconcile.interval: 15 s<br/>kubernetes.operator.observer.progress-check.interval: 5 s |
+| defaultConfiguration.config.yaml | Modern YAML 1.2 config format (introduced in Flink 1.19, required by Flink 2.0). Takes precedence over flink-conf.yaml when set; the resolved config is always mounted as config.yaml. | kubernetes.operator.metrics.reporter.slf4j.factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory<br/>kubernetes.operator.metrics.reporter.slf4j.interval: 5 MINUTE<br/>kubernetes.operator.reconcile.interval: 15 s<br/>kubernetes.operator.observer.progress-check.interval: 5 s |
Review Comment:
done
##########
helm/flink-kubernetes-operator/tests/controller/configmap_test.yaml:
##########
@@ -68,3 +68,66 @@ tests:
asserts:
- isNotNull:
path: data["log4j-console.properties"]
+
+ # FLINK-39791: the ConfigMap emits a single config-file key, always named config.yaml,
+ # resolved as config.yaml (when set) > user flink-conf.yaml > the values.yaml default.
Review Comment:
done
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-5248530591
Thanks @gyfora, good question to ask before merging. Short answer: the override directory keeps working with the old format, and I added a unit test for it (`FlinkConfigManagerTest#testLegacyFormatConfigOverrides`). Three things make it work, and the last one is subtle enough that I pinned it in the test:
1. Flink resolves the config file name **per directory**, legacy first: `GlobalConfiguration.loadConfiguration(dir)` looks for `flink-conf.yaml` and only falls back to `config.yaml`. This PR only changes the operator's own conf directory, so an override ConfigMap mounted with a `flink-conf.yaml` key is still found and parsed by the legacy parser.
2. The merge is programmatic rather than textual. `loadGlobalConfiguration` parses the override directory into a `Configuration` and passes it in as `dynamicProperties`, where `addAll` layers it on top of the operator's own configuration. Two independent parses merged as key/value maps, so there is no format mixing and no duplicate key hazard, and the overrides still win.
3. `standardYaml` is a process-global in flink-core, set by whichever load ran last. The override directory is loaded first and the operator's own directory second, so the effective format is the operator's `config.yaml`. That ordering is what keeps this safe: if those two calls were ever swapped, a legacy override directory would quietly put the whole operator back on the legacy parser. The test asserts `GlobalConfiguration.isStandardYaml()` after loading so that stays true.
I also ran it in a kind cluster to check operational continuity end to end. With the operator on `config.yaml` and an external ConfigMap mounted at `CONF_OVERRIDE_DIR` carrying a `flink-conf.yaml` key:
```
GlobalConfiguration [INFO] Using legacy YAML parser to load flink configuration file from /opt/flink/conf-override/flink-conf.yaml.
GlobalConfiguration [INFO] Using standard YAML parser to load flink configuration file from /opt/flink/conf/config.yaml.
GlobalConfiguration [INFO] Loading dynamic configuration property: kubernetes.operator.reconcile.interval, 42 s
FlinkConfigManager [INFO] Setting default configuration to {... kubernetes.operator.reconcile.interval=42 s ...}
```
The override was effective and not just loaded: reconcile deltas measured 42.0s, 42.1s, 42.1s, 42.1s against the chart default of 15s.
One nuance worth stating: values coming from a legacy override file are raw strings that are then read under standard YAML semantics. Scalars are identical either way. For List and Map typed options flink-core's standard parser tries YAML first and falls back to the legacy splitter on failure, so legacy forms like `a;b` and `k1:v1,k2:v2` resolve to the same values. Also worth noting that the test has to set `FLINK_CONF_DIR`, since without it flink-core returns `new Configuration(dynamicProperties)` and the format would come from the override directory instead. The chart sets `FLINK_CONF_DIR` on both containers, so production always takes the intended path.
On CI: `test_dynamic_flink_conf.sh` was a real failure and is fixed. It patched the `flink-conf.yaml` key of the operator ConfigMap, which is no longer mounted, so the patch was invisible and the test waited out its timeout. It now patches `config.yaml` while still configuring the chart through the legacy `defaultConfiguration.flink-conf.yaml` values key, so the pair of dynamic config tests now covers both branches of the resolver. I also noted in the docs that runtime edits belong to the `config.yaml` key, since that is the one behavior this PR cannot preserve. The `test_batch_job.sh` failure was unrelated: it died in the image build when the CRD compatibility checker could not fetch a released schema from raw.githubusercontent.com, and it needs a re-run.
@Dennis-Mircea the Flink docs link now points at `flink-docs-master` in both language versions.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3754983981
##########
docs/content.zh/docs/deployment/configuration.md:
##########
@@ -51,10 +51,28 @@ defaultConfiguration:
### YAML Configuration File Format
-Operator configuration is delivered through the `defaultConfiguration.flink-conf.yaml` block in the Helm `values.yaml`. The operator's runtime loads that file with Flink's legacy configuration parser, which accepts only flat key-value pairs written with dot notation and does not support nested YAML maps.
+Operator configuration can be delivered in either of Flink's two configuration formats, by defining the matching key under `defaultConfiguration` in the Helm `values.yaml`:
+
+- `flink-conf.yaml` is the legacy format, which accepts only flat key-value pairs written with dot notation and does not support nested YAML maps. This is the format the chart ships by default.
+- `config.yaml` is the YAML 1.2 format defined from Flink 1.19 onward and the only format Flink 2.0 accepts. It supports nested keys (see the [Flink Configuration File](https://nightlies.apache.org/flink/flink-docs-release-2.2/docs/deployment/config/#flink-configuration-file) documentation):
Review Comment:
Done
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#issuecomment-5221257342
Thanks @Dennis-Mircea, rebased onto main and the conflicts are resolved. The restructure had moved every page this PR touched, so the docs changes moved with them:
- `deployment/configuration.md` now documents both formats, that `config.yaml` takes precedence when both keys are set, and that the resolved config is always mounted as `config.yaml`. That replaces the note saying a `config.yaml` block is not effectively loadable, since that is the limitation this PR removes.
- `deployment/helm/installation.md` carries the updated values table rows.
- `internals/startup.md` described the ConfigMap as carrying six data keys including `flink-conf.yaml`, and the volume items as "config.yaml (default) or flink-conf.yaml". Both are now the single resolved `config.yaml` key, so I updated that page and the `GlobalConfiguration` reference in `deployment/compatibility.md` along with it.
- The development guide already refers to `config.yaml` after the restructure, so that edit was dropped as redundant.
- Chinese docs carry the same content.
Re-verified after the rebase: helm-unittest 67/67 and `helm lint` on Helm 4.2.3, plus `ct lint` on the 3.16.4 image, so both versions the chart CI now covers. In a kind cluster with the webhook enabled and an operator image built from this branch, the ConfigMap has the single `config.yaml` key (five data keys total), the Deployment mounts it as `config.yaml`, and three deployments reached RUNNING: `v2_2` with flat and with nested `flinkConfiguration`, and a `v1_20` application deployment.
That last one seems worth calling out now that #1152 is in. The operator runs on `config.yaml` while writing the v1_20 cluster's config to a `flink-conf.yaml` key with a scalar `pipeline.jars`, which is the combination that used to crashloop. I also flipped the operator between this chart and main's chart under those running jobs, killing the JobManagers after each flip, and saw no job disruption, no pod churn, and byte-identical generated configs.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "gerkElznik (via GitHub)" <gi...@apache.org>.
gerkElznik commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3548802063
##########
helm/flink-kubernetes-operator/tests/controller/deployment_test.yaml:
##########
@@ -383,3 +383,18 @@ tests:
key: log4j-operator.properties
path: log4j-operator.properties
+# FLINK-39791: the operator config is always mounted as config.yaml (the only filename Flink 2.x
+# reads), regardless of which defaultConfiguration key the user populates.
Review Comment:
done
##########
helm/flink-kubernetes-operator/values.yaml:
##########
@@ -174,14 +174,27 @@ defaultConfiguration:
# This option has not effect, if create is equal to false.
append: true
-# Uncomment to use the new config.yaml format, but also comment out the flink-config.yaml key.
+# Operator configuration overrides go under ONE of the two keys below. If both are set,
+# config.yaml wins; either way the operator mounts the result as config.yaml.
+#
+# config.yaml - modern YAML 1.2 format (Flink 1.19+, required by Flink 2.0). Recommended.
+# flink-conf.yaml - legacy flat "key: value" format (shipped as the default below).
+#
+# Prefer the nested config.yaml form: a flat, dotted-key override of a key the chart already
+# ships (e.g. the Java 17 opts in conf/config.yaml) would be a duplicate key under Flink's
+# strict YAML parser and fail at operator startup. Example, equivalent to the default below:
+#
# config.yaml: |+
# kubernetes.operator:
# metrics.reporter.slf4j:
# factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory
# interval: 5 MINUTE
# reconcile.interval: 15 s
# observer.progress-check.interval: 5 s
+# taskmanager:
+# numberOfTaskSlots: 1 # default for managed clusters; override per-job via spec.flinkConfiguration
Review Comment:
done
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml [flink-kubernetes-operator]
Posted by "bkinzle (via GitHub)" <gi...@apache.org>.
bkinzle commented on code in PR #1126:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3548776847
##########
docs/content/docs/operations/helm.md:
##########
@@ -71,9 +71,9 @@ The configurable parameters of the Helm chart and which default values as detail
| Parameters | Description | Default Value |
|------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| defaultConfiguration.append | Whether to append configuration files with configs. | true |
-| defaultConfiguration.config.yaml | The newer configuration file format for flink that will enforced in Flink 2.0. Note this was introduced in flink 1.19. | kubernetes.operator.metrics.reporter.slf4j.factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory<br/>kubernetes.operator.metrics.reporter.slf4j.interval: 5 MINUTE<br/>kubernetes.operator.reconcile.interval: 15 s<br/>kubernetes.operator.observer.progress-check.interval: 5 s |
+| defaultConfiguration.config.yaml | Modern YAML 1.2 config format (introduced in Flink 1.19, required by Flink 2.0). Takes precedence over flink-conf.yaml when set; the resolved config is always mounted as config.yaml. | kubernetes.operator.metrics.reporter.slf4j.factory.class: org.apache.flink.metrics.slf4j.Slf4jReporterFactory<br/>kubernetes.operator.metrics.reporter.slf4j.interval: 5 MINUTE<br/>kubernetes.operator.reconcile.interval: 15 s<br/>kubernetes.operator.observer.progress-check.interval: 5 s |
Review Comment:
done
##########
helm/flink-kubernetes-operator/tests/controller/configmap_test.yaml:
##########
@@ -68,3 +68,66 @@ tests:
asserts:
- isNotNull:
path: data["log4j-console.properties"]
+
+ # FLINK-39791: the ConfigMap emits a single config-file key, always named config.yaml,
+ # resolved as config.yaml (when set) > user flink-conf.yaml > the values.yaml default.
Review Comment:
done
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org