peter-toth commented on code in PR #851:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/851#discussion_r4057760272
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -228,6 +260,107 @@ private static void checkNoNodeSelectorConflict(
}
}
+ /**
+ * Checks whether the priority class of the given Workload is followed and
whether Kueue accepts
+ * the change. Like Kueue's `classifyWorkloadsForPriorityUpdate`, only a
Workload without a
+ * priority class or with a WorkloadPriorityClass follows the owner, so the
priority of a Workload
+ * backed by a Kubernetes PriorityClass is never rewritten, e.g. when the
global default changes.
+ * Once the quota is reserved, the Workload CEL rules freeze the presence,
the group and the kind
+ * of the priorityClassRef. The name of a WorkloadPriorityClass stays
mutable, which is what Kueue
+ * relies on to raise the priority of a Workload waiting for its admission
checks.
+ */
+ private static boolean isPriorityClassChangeAllowed(
+ final Workload workload, final PriorityClassRef desired) {
+ PriorityClassRef current = workload.getSpec().getPriorityClassRef();
+ if (current != null &&
!Constants.KUEUE_API_GROUP.equals(current.getGroup())) {
+ return false;
+ }
+ if (workload.getStatus() == null ||
!workload.getStatus().isQuotaReserved()) {
Review Comment:
**Finding 9.** I re-ran the per-clause sweep on the rewritten guard. Five of
the six clauses are killed — the Pod-`PriorityClass` skip, `current != null`,
`desired != null`, the desired-group check, and the caller's `getPriority() !=
null`. This one survives.
Removing it is not a no-op. Without the short-circuit the guard falls
straight to `current != null && …`, so a `Workload` that currently has **no**
priority class can never gain one — including while it is merely pending. That
is the case a user hits most: a queued resource created without the label, then
labelled while it waits for quota. Every existing test starts from a `Workload`
that already carries a `WorkloadPriorityClass` ref (`low` → `high`), and
`quotaReservedWorkloadWithoutPriorityClassDoesNotGainOne` covers only the
negative, quota-reserved side of the same transition.
This passes as written and fails with the clause removed — I ran both:
```java
@Test
void pendingWorkloadWithoutPriorityClassFollowsAnAddedLabel() {
createWorkloadPriorityClass("high", 1000);
// No label and no pod priority class, so the Workload is created
without a ref
KueueWorkloadUtils.requestAdmission(kubernetesClient,
workload("owner-uid-1", 1));
Assertions.assertNull(getWorkload().getSpec().getPriorityClassRef());
// The label is added while the Workload waits for quota
Assertions.assertEquals(
AdmissionResult.PENDING,
KueueWorkloadUtils.requestAdmission(kubernetesClient,
workloadWithPriorityClass("high")));
Assertions.assertEquals("high",
getWorkload().getSpec().getPriorityClassRef().getName());
Assertions.assertEquals(1000, getWorkload().getSpec().getPriority());
}
```
It also documents the nil-to-ref direction, which is currently only asserted
in its blocked form.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -40,21 +42,29 @@
import com.fasterxml.jackson.databind.SerializationFeature;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.OwnerReference;
+import io.fabric8.kubernetes.api.model.PodTemplateSpec;
import io.fabric8.kubernetes.api.model.Toleration;
+import io.fabric8.kubernetes.api.model.scheduling.v1.PriorityClass;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.KubernetesClientException;
import lombok.extern.slf4j.Slf4j;
+import org.apache.spark.k8s.operator.Constants;
import org.apache.spark.k8s.operator.kueue.v1beta2.PodSet;
import org.apache.spark.k8s.operator.kueue.v1beta2.PodSetAssignment;
+import org.apache.spark.k8s.operator.kueue.v1beta2.PriorityClassRef;
import org.apache.spark.k8s.operator.kueue.v1beta2.ResourceFlavor;
import org.apache.spark.k8s.operator.kueue.v1beta2.Workload;
+import org.apache.spark.k8s.operator.kueue.v1beta2.WorkloadPriorityClass;
+import org.apache.spark.k8s.operator.kueue.v1beta2.WorkloadSpec;
import org.apache.spark.k8s.operator.kueue.v1beta2.WorkloadStatus;
import org.apache.spark.k8s.operator.utils.ModelUtils;
import org.apache.spark.k8s.operator.utils.ReconcilerUtils;
+import org.apache.spark.k8s.operator.utils.StringUtils;
/** Utilities to create, check and release Kueue Workloads. */
@Slf4j
+@SuppressWarnings("PMD.GodClass")
Review Comment:
**Finding 10.** Answering the question at the end of [your
comment](https://github.com/apache/spark-kubernetes-operator/pull/851#issuecomment-5751418533):
yes, I would take the split.
The numbers argue for it rather than against. `pmdMain` is green on `main`
for this class, and with this PR it reports:
```
KueueWorkloadUtils.java:67: GodClass: Possible God Class (WMC=61, ATFD=97,
TCC=3.297%)
```
That is 14 over the threshold of 47, not one or two, and `TCC=3.297%` says
the methods share almost no state — the class is now three unrelated jobs: the
`Workload` lifecycle (`requestAdmission`, `releaseWorkload`, `isAdmitted`,
`hashPodSets`), flavor resolution from #850, and priority resolution from this
PR. #847 adds `holdForAdmission` on top of that.
I tried the cut. Moving `setPriority`, `getPodPriorityClass` and
`isPriorityClassChangeAllowed` into a `KueueWorkloadPriority` class in the same
package, with `requestAdmission` calling
`KueueWorkloadPriority.setPriority(...)` and
`KueueWorkloadPriority.isPriorityClassChangeAllowed(...)`, makes `pmdMain` pass
with **no suppression on either class** — the only fallout was the imports left
behind in `KueueWorkloadUtils`, which have to move too. It also gives the
priority tests an obvious home, mirroring how #850 put `KueuePodSetFlavor`
beside the resolver.
The `KueueWorkloadFactory` precedent is real, so suppressing is not
unreasonable. The difference is that this class trips the rule because of the
change in front of us, and the cut that fixes it is small and available.
--
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]
