peter-toth commented on code in PR #884:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/884#discussion_r4097501239
##########
tests/e2e/kueue/chainsaw-test.yaml:
##########
@@ -168,6 +168,16 @@ spec:
type: Normal
reason: KueueAdmissionPending
(contains(message || '',
'sparkapplication-spark-job-kueue-queued-test')): true
+ # The ValidatingAdmissionPolicy of the chart lets the queued application
move to another
+ # queue, here and back again
+ - script:
+ timeout: 30s
+ env:
+ - name: LOCAL_QUEUE
+ value: ($LOCAL_QUEUE)
+ content: |
Review Comment:
**Finding 1.** This step cannot fail. A shell script exits with the status
of its last command, and there is no `set -e`, so the first `kubectl label` can
be rejected without the step noticing.
Measured locally, which is the whole mechanism:
```
$ sh -c 'ls /nonexistent-path-xyz
echo "second command ran"'; echo "exit=$?"
ls: /nonexistent-path-xyz: No such file or directory
second command ran
exit=0
```
And the second command here is specifically a no-op on failure. If the
policy rejects the move to `another-queue`, the label is still `$LOCAL_QUEUE`,
so `kubectl label --overwrite kueue.x-k8s.io/queue-name=$LOCAL_QUEUE` prints
`not labeled` and exits 0. The step goes green either way. The same applies to
the `Suspended` cluster step at `tests/e2e/kueue/chainsaw-test.yaml:591-593`.
That matters because these two steps are the only coverage of the *permit*
direction. The two deny-direction steps work correctly, since they assert
`($error != null)`, which is also what shows that chainsaw does check the exit
status. So today a regression in the allow-list, for instance dropping
`'Suspended'` from the `validations` expression, would pass CI in silence while
the deny steps kept passing.
```suggestion
content: |
set -e
```
Two notes while you are in there. The queued application in this step has no
persisted status, since the initial `Submitted` of a first attempt is never
written while the `Workload` waits, so it exercises the `''` branch rather than
`'Submitted'`. Of the four entries in the allow-list, `''` and `'Suspended'`
get covered once `set -e` is in, and `'Submitted'` and `'ScheduledToRestart'`
stay uncovered. A cluster resumed from `Suspended` does persist `Submitted`, so
the existing resume step could carry a label change and close that one cheaply.
I could not run the suite end to end here, since `docker info` returns
`Forbidden` on this machine so there is no cluster. The experiment that settles
it is removing `'Suspended'` from the `validations` expression and running the
`kueue` group: today it should stay green, and with `set -e` it should fail on
the cluster step.
##########
build-tools/helm/spark-kubernetes-operator/values.yaml:
##########
@@ -143,8 +143,10 @@ operatorRbac:
# granted through the ClusterRole only, so
{operatorRbac.clusterRole.create} is required
# for the operator to read them. Kueue itself must be installed separately.
# Enabling this also enables the Kueue integration of the operator
- # (spark.kubernetes.operator.kueue.enabled), which requires Kueue to be
installed.
- # Otherwise, the operator ignores the kueue.x-k8s.io/queue-name label.
+ # (spark.kubernetes.operator.kueue.enabled), which requires Kueue to be
installed, and
+ # installs a ValidatingAdmissionPolicy to reject a change of the
kueue.x-k8s.io/queue-name
Review Comment:
**Finding 2.** This comment now describes the policy, but the two places in
`docs/operations.md` that mirror it do not.
The values table row at `docs/operations.md:137` still reads "Grant the
operator access to Kueue `workloads`, `resourceflavors`,
`workloadpriorityclasses` and to `priorityclasses`, and enable the Kueue
integration". That table is the reference for chart users, and the flag now
also creates two cluster-scoped objects.
The Kueue bullet under Optional Prerequisites, `docs/operations.md:39-55`,
is the other one. It is where a reader looks for what the flag requires of
them, and the new requirement only appears in `docs/spark_custom_resources.md`.
Since the policy is cluster-scoped with `failurePolicy: Fail`, an installer
without access to `validatingadmissionpolicies` and
`validatingadmissionpolicybindings` in `admissionregistration.k8s.io` gets a
failed `helm install` or `helm upgrade` rather than a degraded install, so it
belongs next to the other prerequisites.
##########
docs/spark_custom_resources.md:
##########
@@ -678,6 +678,22 @@ spec:
terminating pods still occupy the quota. The `Workload` is deleted even if
the
`kueue.x-k8s.io/queue-name` label was removed after the admission. The
resource is queued again
when it is resumed with the `kueue.x-k8s.io/queue-name` label.
+* Like the webhooks of Kueue built-in integrations, which let only a suspended
job change its
+ queue, the Helm chart installs a `ValidatingAdmissionPolicy` with
`operatorRbac.kueue.enabled`.
+ It rejects an update that adds, changes or removes the
`kueue.x-k8s.io/queue-name` label of a
+ `SparkApplication` or a `SparkCluster` once it started, since the `Workload`
admitted for it
Review Comment:
**Finding 3.** "once it started" claims more than the policy can deliver,
because the only thing it can read is the status.
`oldState` falls back to `''` when
`oldObject.status.currentState.currentStateSummary` is absent, and the bullet
treats that as "before the resource starts". There is one more way to get
there. The initial `Submitted` of a first attempt is never persisted while the
`Workload` waits, so the first status a queued resource ever gets is the one
written *after* its driver or master is created: `DriverRequested` in
`AppInitStep`, `RunningHealthy` in `ClusterInitStep`. If that write fails, the
resource is running with no persisted status, `oldState` is `''`, and the
policy admits a label change.
This is the same window `SPARK-59769` is about, so the project already
treats it as real. And the policy cannot close it: nothing in the object says
the driver exists except the status that did not get written. So this is a doc
point, not a fix. Something like "once its state says it started" plus a
sentence that the operator's own handling covers the window where the first
status write has not landed yet would set the expectation right, and it
explains why both halves exist.
--
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]
