-
Notifications
You must be signed in to change notification settings - Fork 536
[FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml #1126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 }} | ||
|
Comment on lines
-33
to
-43
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we removing these?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question — there are three separate edits in that hunk:
The one behavioral consequence of (3) I'd like your read on before acting: runtime (Aside, unrelated to this hunk: I've rebased onto latest |
||
| {{- if hasKey (.Values.defaultConfiguration) "flink-conf.yaml" }} | ||
| {{- index (.Values.defaultConfiguration) "flink-conf.yaml" | nindent 4 -}} | ||
| {{- with (index .Values.defaultConfiguration "config.yaml") }} | ||
| {{- . | nindent 4 -}} | ||
| {{- else }} | ||
| {{- with (index .Values.defaultConfiguration "flink-conf.yaml") }} | ||
| {{- . | nindent 4 -}} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- if .Values.watchNamespaces }} | ||
| kubernetes.operator.watched.namespaces: {{ join "," .Values.watchNamespaces }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment not needed.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| - it: Emits a single config.yaml key with today's effective defaults and no redundant seed keys | ||
| asserts: | ||
| - isNotNull: | ||
| path: data["config.yaml"] | ||
| - notExists: | ||
| path: data["flink-conf.yaml"] | ||
| - matchRegex: | ||
| path: data["config.yaml"] | ||
| pattern: "(?m)^\\s*kubernetes.operator.metrics.reporter.slf4j.factory.class:" | ||
| - matchRegex: | ||
| path: data["config.yaml"] | ||
| pattern: "(?m)^\\s*kubernetes.operator.reconcile.interval: 15 s" | ||
| - matchRegex: | ||
| path: data["config.yaml"] | ||
| pattern: "(?m)^\\s*kubernetes.operator.health.probe.enabled: true" | ||
| - notMatchRegex: | ||
| path: data["config.yaml"] | ||
| pattern: "(?m)^\\s*taskmanager.numberOfTaskSlots:" | ||
| - notMatchRegex: | ||
| path: data["config.yaml"] | ||
| pattern: "(?m)^\\s*parallelism.default:" | ||
|
|
||
| - it: A user flink-conf.yaml override flows into config.yaml when config.yaml is unset | ||
| set: | ||
| defaultConfiguration: | ||
| flink-conf.yaml: |+ | ||
| kubernetes.operator.reconcile.interval: 99 s | ||
| asserts: | ||
| - matchRegex: | ||
| path: data["config.yaml"] | ||
| pattern: "reconcile.interval: 99 s" | ||
|
|
||
| - it: When both are set, config.yaml wins and flink-conf.yaml is ignored | ||
| set: | ||
| defaultConfiguration: | ||
| config.yaml: |+ | ||
| kubernetes.operator: | ||
| reconcile.interval: 30 s | ||
| flink-conf.yaml: |+ | ||
| kubernetes.operator.reconcile.interval: 99 s | ||
| asserts: | ||
| - matchRegex: | ||
| path: data["config.yaml"] | ||
| pattern: "reconcile.interval: 30 s" | ||
| - notMatchRegex: | ||
| path: data["config.yaml"] | ||
| pattern: "reconcile.interval: 99 s" | ||
|
|
||
| - it: A flat config.yaml override of a former seed key is not duplicated (relies on dropping it from the seed) | ||
| set: | ||
| defaultConfiguration: | ||
| config.yaml: |+ | ||
| taskmanager.numberOfTaskSlots: 2 | ||
| asserts: | ||
| - matchRegex: | ||
| path: data["config.yaml"] | ||
| pattern: "taskmanager.numberOfTaskSlots: 2" | ||
| - notMatchRegex: | ||
| path: data["config.yaml"] | ||
| pattern: "taskmanager.numberOfTaskSlots: 1" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment not needed.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| - it: Should mount the operator config as config.yaml by default | ||
| asserts: | ||
| - contains: | ||
| path: spec.template.spec.volumes[0].configMap.items | ||
| content: | ||
| key: config.yaml | ||
| path: config.yaml | ||
| - notContains: | ||
| path: spec.template.spec.volumes[0].configMap.items | ||
| content: | ||
| key: flink-conf.yaml | ||
| path: flink-conf.yaml | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessary to specify this property here.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| # | ||
| # Set job parallelism via spec.job.parallelism, not parallelism.default (the operator manages it). | ||
|
|
||
| flink-conf.yaml: |+ | ||
| # Flink Config Overrides | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chinese docs can be synced with the same english content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done