peter-toth commented on code in PR #839:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/839#discussion_r4034630086
##########
docs/operations.md:
##########
@@ -83,25 +83,32 @@ following table:
| image.tag | The image
tag of spark-kubernetes-operator.
| 1.1.0-SNAPSHOT
|
| 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
|
Review 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.
##########
docs/operations.md:
##########
@@ -83,25 +83,32 @@ following table:
| image.tag | The image
tag of spark-kubernetes-operator.
| 1.1.0-SNAPSHOT
|
| 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
|
| 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
|
Review 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). | |
```
##########
docs/operations.md:
##########
@@ -111,11 +118,12 @@ following table:
| operatorRbac.roleBinding.create | Whether
to create RoleBinding for operator to use. At least one of
`clusterRoleBinding.create` or `roleBinding.create` should be enabled
| false
|
| operatorRbac.roleBinding.name | Name of
the operator RoleBinding in each workload namespace(s).
| `"spark-operator-rolebinding"`
|
| operatorRbac.roleBinding.roleRef | RoleRef
for the created Operator RoleBinding. Override this when you want the created
RoleBinding refer to ClusterRole / Role that's different from the default
operator Role. | Refers to default `operatorRbac.role.name`
|
-| 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
|
Review 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](https://issues.apache.org/jira/browse/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.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
