Repository navigation
[SPARK-59596] Fix Helm chart configuration table in operations.md to match values.yaml - #839
dongjoon-hyun wants to merge 2 commits into
Conversation
…o match `values.yaml`
There was a problem hiding this comment.
Thanks for the PR, @dongjoon-hyun!
I verified the table mechanically rather than by eye: flattening values.yaml and diffing both directions, every documented key now exists and every leaf in values.yaml has a documented ancestor, with the only three "missing" keys being the deliberate ones (image.digest, operatorRbac.annotations, dynamicConfig.enable). Every scalar default in the table matches too, including the long jvmArgs string. Diffing against values.schema.json instead of values.yaml is where the remaining gaps are - nameOverride is a live key with no row, and configManagement.create is documented as a toggle the chart ignores.
Non-blocking
- 1.
nameOverridehas no row, andfullnameOverrideis dead: both are schema-declared and absent fromvalues.yaml, the same category as theoperatorRbac.annotationsrow you added.nameOverridechanges 11 places in the rendered output;fullnameOverridechanges nothing, becausespark-operator.fullnamehas no consumer. [inline:operations.md:86] - 2.
configManagement.createis documented as a toggle no template reads: rendering withcreate=falseandcreate=trueis identical. The row now names the real gate, which is an improvement, but still presentscreateas the switch. [inline:operations.md:121]
Minor
- 3. The table documents one deprecated key but not its twin:
dynamicConfig.enablehas a row at:154,networkPolicy.enabledoes not, and this PR's newnetworkPolicy.enabledrow is what makes that an in-table asymmetry. [inline:operations.md:88]
| | image.digest | The image digest of spark-kubernetes-operator. If set then it takes precedence and the image tag will be ignored. | | | ||
| | imagePullSecrets | The image pull secrets of spark-kubernetes-operator. | | | ||
| | operatorDeployment.replica | Operator replica count. Must be 1 unless leader election is configured. | 1 | | ||
| | operatorDeployment.replicas | Operator replica count. Must be 1 unless leader election is configured. | 1 | |
There was a problem hiding this comment.
Finding 1. Anchoring here because a nameOverride row belongs just above, with the other top-level keys.
Diffing the table against values.yaml finds nothing left - I flattened both and every leaf has a documented ancestor. Diffing against values.schema.json finds two more, both top-level and both absent from values.yaml, which is exactly why they slipped through:
$ # schema properties with no documented ancestor, ignoring container nodes
nameOverride
fullnameOverride
operatorDeployment.networkPolicy.enable # finding 3
They need opposite treatment.
nameOverride is live and should get a row. The schema describes it as "Override chart name", and _helpers.tpl:20 and :32 both read it:
$ helm template spark <chart> --set nameOverride=myop | grep -c myop
11
spark-operator.name is referenced from eight template files, so this key renames labels, the NetworkPolicy, the PDB and the helm-test pods. It is the same category as the operatorRbac.annotations row you added - schema-declared, no default in values.yaml, still settable.
fullnameOverride is dead and should not get a row. spark-operator.fullname is defined at _helpers.tpl:28 and referenced nowhere else in the chart:
$ grep -rn "spark-operator.fullname" build-tools/helm/spark-kubernetes-operator/
.../templates/_helpers.tpl:28:{{- define "spark-operator.fullname" -}}
$ helm template spark <chart> --set fullnameOverride=my-op | grep -c "my-op"
0
So documenting it would advertise a key that does nothing. Either drop the helper and its schema entry, or leave it out of the table - but the schema currently promises something the chart does not deliver, which is the same class of problem this PR is fixing from the other direction.
| | operatorRbac.configManagement.create | Enable this to create a Role for operator configuration management (hot property loading and leader election). | true | | ||
| | operatorRbac.configManagement.roleName | Role name for operator configuration management. | `spark-operator-config-role` | | ||
| | operatorRbac.configManagement.roleBinding | RoleBinding name for operator configuration management. | `"spark-operator-config-monitor-role-binding"` | | ||
| | operatorRbac.configManagement.create | Enable this to create a Role for operator configuration management (hot property loading from ConfigMap). Requires `dynamicConfig` with the `configMap` source. | true | |
There was a problem hiding this comment.
Finding 2. The second half of this description is the accurate part and a real improvement - operator-rbac.yaml:239 gates the Role on dynamicConfig.enabled and source == configMap. The first half is still wrong: nothing reads create.
$ grep -rn "configManagement" build-tools/helm/spark-kubernetes-operator/templates/
.../operator-rbac.yaml:242: name: {{ .Values.operatorRbac.configManagement.roleName }}
.../operator-rbac.yaml:262: name: {{ .Values.operatorRbac.configManagement.roleBindingName }}
.../operator-rbac.yaml:268: name: {{ .Values.operatorRbac.configManagement.roleName }}
Only the two name fields. So the row promises a switch that does not exist, and I confirmed the render is identical either way:
$ helm template spark <chart> --set operatorRbac.configManagement.create=false \
--set operatorConfiguration.dynamicConfig.enabled=true | grep -c spark-operator-config-monitor
3
$ # same command with create=true
3
A user following this table to suppress that Role and RoleBinding cannot, which is the failure mode the "Why" section describes. SPARK-59537 tracks wiring create in and is still Open, so until it lands the row could say so:
| operatorRbac.configManagement.create | Whether to create a Role for operator configuration management (hot property loading from ConfigMap). Currently not honored, see SPARK-59537: the Role is created whenever `dynamicConfig` is enabled with the `configMap` source. | true |
Or fix the chart here and keep the description as written - either resolves it, and the doc-only version is the smaller change.
| | operatorDeployment.replica | Operator replica count. Must be 1 unless leader election is configured. | 1 | | ||
| | operatorDeployment.replicas | Operator replica count. Must be 1 unless leader election is configured. | 1 | | ||
| | operatorDeployment.strategy.type | Operator pod upgrade strategy. Must be Recreate unless leader election is configured. | Recreate | | ||
| | operatorDeployment.networkPolicy.enabled | When enabled, a NetworkPolicy allows ingress to the operator pod only on the health probe and metrics ports. Requires a CNI plugin with NetworkPolicy support. | false | |
There was a problem hiding this comment.
Finding 3. operatorDeployment.networkPolicy.enable is still honored - _helpers.tpl resolves the toggle with or $np.enabled $np.enable - and it is removed in chart 2.0.0 under the same ticket, SPARK-59533. The table gives its twin a row:
:154 | operatorConfiguration.dynamicConfig.enable | Deprecated, use `operatorConfiguration.dynamicConfig.enabled`. Still honored: `enable: true` wins over `enabled: false`. Removed in chart 2.0.0 (SPARK-59533). |
Before this PR the table had no networkPolicy rows at all, so the legacy key living only in the prose at :196 was consistent. Now that enabled has a row, the pair reads as though only one of the two toggles was ever renamed. A matching row keeps the table self-contained:
| operatorDeployment.networkPolicy.enable | Deprecated, use `operatorDeployment.networkPolicy.enabled`. Still honored: `enable: true` wins over `enabled: false`. Removed in chart 2.0.0 (SPARK-59533). | |
|
Thank you for the review, @peter-toth. Addressed in 7bdd09d.
|
|
Merged to main |
What changes were proposed in this pull request?
This PR aims to fix the Helm chart configuration table in
docs/operations.mdto be consistent with the Helm chart (values.yamlandtemplates/*.yaml).Fix the wrong keys, descriptions and default values
operatorDeployment.replicaoperatorDeployment.replicasoperatorDeployment.additionalContainersoperatorDeployment.operatorPod.additionalContainersoperatorDeployment.operatorPod.operatorContainer.jvmArgs-Dfile.encoding=UTF8 -XX:+CrashOnOutOfMemoryError -XX:ErrorFile=/dev/stderr -XX:+UseParallelGC-Dfile.encoding=UTF8 -XX:+CrashOnOutOfMemoryError -XX:ErrorFile=/dev/stderr -XX:+UseParallelGC -XX:InitialRAMPercentage=80 -XX:MaxRAMPercentage=80 -XX:+AlwaysPreTouch -XX:+UseCompactObjectHeadersoperatorRbac.serviceAccount.nameoperatorRbac.configManagement.createdynamicConfigwith theconfigMapsource.operatorRbac.configManagement.roleNamespark-operator-config-role"spark-operator-config-monitor"operatorRbac.configManagement.roleBindingoperatorRbac.configManagement.roleBindingNameworkloadResources.serviceAccounts.createworkloadResources.serviceAccount.createworkloadResources.serviceAccounts.nameworkloadResources.serviceAccount.nameworkloadResources.sparkApplicationSentinel.sentinelNamespacesworkloadResources.sparkApplicationSentinel.sentinelNamespaces.dataworkloadResources.sparkClusterSentinel.sentinelNamespacesworkloadResources.sparkClusterSentinel.sentinelNamespaces.dataAdd the missing keys
nameOverrideoperatorDeployment.networkPolicy.enabledfalseoperatorDeployment.networkPolicy.enable(deprecated)operatorDeployment.networkPolicy.metricsIngressoperatorDeployment.operatorPod.affinityoperatorDeployment.operatorPod.tolerationsoperatorDeployment.operatorPod.dnsPolicyoperatorDeployment.operatorPod.operatorContainer.volumeMountsoperatorDeployment.operatorPod.operatorContainer.metrics.port19090operatorRbac.annotationsoperatorConfiguration.configMap.annotationsoperatorConfiguration.configMap.labelsoperatorConfiguration.dynamicConfig.sourceconfigMapWhy are the changes needed?
The documentation is inconsistent with the actual Helm chart. Some documented keys do not exist in the chart, so users cannot configure the chart by following the documentation. In addition, several supported keys are not documented.
Does this PR introduce any user-facing change?
No behavior change. This is a documentation-only fix.
How was this patch tested?
Manual review.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5