dongjoon-hyun commented on code in PR #884:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/884#discussion_r4097610500
##########
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:
Thank you, @peter-toth. Fixed in 3e3888d by adding `set -e` to both
permit-direction steps, the queued `SparkApplication` one and the `Suspended`
`SparkCluster` one.
For the uncovered `'Submitted'` and `'ScheduledToRestart'` entries, I didn't
add E2E steps. A resumed cluster gets its quota back right away in this test,
so it passes through `Submitted` almost immediately, and a label change in that
window would make the step timing-dependent. I verified both entries, together
with the other states, against a `kind` cluster instead.
##########
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:
Thank you. Fixed in 3e3888d. The `operatorRbac.kueue.enabled` row of the
values table now mentions the policy, and the Kueue bullet under Optional
Prerequisites says that whoever installs or upgrades the chart needs access to
`validatingadmissionpolicies` and `validatingadmissionpolicybindings` of
`admissionregistration.k8s.io`.
##########
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:
Thank you. Fixed in 3e3888d. The bullet now says "once its status says that
it started", and it adds that the policy reads only the status. So a resource
whose first status is not persisted yet looks like it has not started, and its
label may still change. In that window, the operator keeps the admitted
`Workload` of such a resource.
--
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]
