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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
