aicam commented on code in PR #6617: URL: https://github.com/apache/texera/pull/6617#discussion_r3639871788
########## bin/k8s/utils/general-purpose-nodepool-disruption.yaml: ########## @@ -0,0 +1,72 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Disruption-policy override for the EKS Auto Mode `general-purpose` NodePool +# (AWS / EKS only). Standalone cluster manifest, NOT part of the Helm release. +# +# WHY THIS EXISTS +# --------------- +# Any CU pod that does NOT carry the cu-pool nodeSelector/toleration (e.g. before +# cu-nodepool.yaml is applied, or if the placement env vars are unset) lands on Review Comment: Removed. You are right that it is redundant once the placement values are set — with `values-aws.yaml` applied, no CU ever lands on the `general-purpose` pool, so there is nothing for this to protect. It was also never a reliable fix: that pool is EKS-managed (`app.kubernetes.io/managed-by=eks`), so Auto Mode reconciles the patch back to `WhenEmptyOrUnderutilized` within ~30 min. Keeping a file that silently stops working is worse than not having it. `bin/k8s/utils/` is gone from the PR as a result. ########## common/config/src/main/resources/kubernetes.conf: ########## @@ -54,4 +54,21 @@ kubernetes { # GPU resource key used in Kubernetes (vendor-specific) computing-unit-gpu-resource-key = "nvidia.com/gpu" computing-unit-gpu-resource-key = ${?KUBERNETES_COMPUTING_UNIT_GPU_RESOURCE_KEY} + + # Optional: pin CU pods to a dedicated, tainted Karpenter NodePool so their + # disruption policy (WhenEmpty) is isolated from shared nodes and persists. + # See bin/k8s/utils/cu-nodepool.yaml. When the label key+value are set, the + # cu-manager adds a matching nodeSelector to each CU pod; when the toleration + # key is set, it adds a NoSchedule/Exists toleration for that taint. + # + # Leave all three EMPTY (default) to schedule CU pods on the cluster's default + # pool — required for local/dev clusters that have no dedicated CU NodePool. + compute-unit-node-selector-label = "" + compute-unit-node-selector-label = ${?KUBERNETES_COMPUTE_UNIT_NODE_SELECTOR_LABEL} Review Comment: Good question, and the answer is no — but the concern behind it was right, so I removed the possibility of drift entirely. A CU pod's `nodeSelector` matches a **node label**, not a NodePool's `metadata.name`. The NodePool stamps `texera.io/node-role: computing-unit` onto the nodes it provisions (via `spec.template.metadata.labels`) and the pod selects that; the pool could be renamed and nothing would break. (Karpenter does put a `karpenter.sh/nodepool` label on its nodes, but it is a restricted label that cannot be set in a NodePool template, so keying off it is not the cleaner option it looks like.) What matters is that the pod's selector/toleration and the pool's label/taint agree, so they now come from one place: `workflowComputingUnitManager.computeUnitPlacement` in `values-aws.yaml` is both what the cu-manager stamps onto each CU pod and what renders the pool's label and taint. Change it once, both sides move. Enabling the pool without those values now fails the render with an explanatory message instead of producing a pool nothing selects. Setting these by hand is still supported for a self-managed node pool, and the config comment now spells out that the label and taint must exist on those nodes or the CU pods stay Pending. ########## computing-unit-managing-service/src/main/scala/org/apache/texera/service/util/KubernetesClient.scala: ########## @@ -133,6 +133,37 @@ object KubernetesClient { specBuilder.withRuntimeClassName("nvidia") } + // Pin CU pods to the dedicated Karpenter CU NodePool when configured. + // + // The CU NodePool (bin/k8s/utils/cu-nodepool.yaml) is non-EKS-managed and Review Comment: Removed — down to two lines pointing at `KubernetesConfig`. The rationale now lives in `kubernetes.conf` and in the NodePool template, next to the settings it explains rather than in the call site. -- 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]
