diff --git a/api/v1beta1/nodeset_types.go b/api/v1beta1/nodeset_types.go index 04ccd8dc..d7ac7b06 100644 --- a/api/v1beta1/nodeset_types.go +++ b/api/v1beta1/nodeset_types.go @@ -7,6 +7,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" ) const ( @@ -19,6 +20,7 @@ var ( ) // NodeSetSpec defines the desired state of NodeSet +// +kubebuilder:validation:XValidation:rule="(!has(self.preferKubernetesNodeName) || self.preferKubernetesNodeName) == (!has(oldSelf.preferKubernetesNodeName) || oldSelf.preferKubernetesNodeName)",message="preferKubernetesNodeName is immutable" type NodeSetSpec struct { // controllerRef is a reference to the Controller CR to which this has membership. // +required @@ -124,6 +126,13 @@ type NodeSetSpec struct { // +default:=false PinToNode bool `json:"pinToNode"` + // PreferKubernetesNodeName enables DaemonSet-style Slurm node naming for StatefulSet workers. + // Only takes effect with pinToNode=true and oversubscribeNode=false. + // Defaults to true. This preference is immutable after creation. + // +optional + // +kubebuilder:default:=true + PreferKubernetesNodeName *bool `json:"preferKubernetesNodeName,omitempty"` + // WorkloadDisruptionProtection controls whether or not pods in this nodeset which are actively running Slurm jobs are protected by // a Pod Disruption Budget. // See https://kubernetes.io/docs/tasks/run-application/configure-pdb/ for more information. @@ -144,6 +153,23 @@ type NodeSetSpec struct { OversubscribeNode bool `json:"oversubscribeNode,omitempty"` } +type SlurmNodeNameModeType string + +const ( + SlurmNodeNameModePodHostname SlurmNodeNameModeType = "PodHostname" + SlurmNodeNameModeKubernetesNode SlurmNodeNameModeType = "KubernetesNode" +) + +func (spec *NodeSetSpec) EffectiveSlurmNodeNameMode() SlurmNodeNameModeType { + if spec.ScalingMode == ScalingModeDaemonset { + return SlurmNodeNameModeKubernetesNode + } + if ptr.Deref(spec.PreferKubernetesNodeName, true) && spec.PinToNode && !spec.OversubscribeNode { + return SlurmNodeNameModeKubernetesNode + } + return SlurmNodeNameModePodHostname +} + // ScalingModeType is a string enumeration of how a NodeSet scales its pods. // +enum type ScalingModeType string diff --git a/api/v1beta1/well_known.go b/api/v1beta1/well_known.go index 0ca8546c..33a730e5 100644 --- a/api/v1beta1/well_known.go +++ b/api/v1beta1/well_known.go @@ -49,9 +49,8 @@ const ( // Ref: https://slurm.schedmd.com/slurm.conf.html#OPT_Features AnnotationNodeFeaturesSpec = FeaturesPrefix + "spec" - // AnnotationNodeHostnameOverride may be set to override the pod hostname assigned to NodeSet DaemonSet-mode - // pod scheduled on the node. When present, the value is used verbatim as the pod's spec.hostname - // (and therefore the Slurm node name) instead of the default derived from the node name. + // AnnotationNodeHostnameOverride overrides DaemonSet hostnames and StatefulSet Slurm names + // when the effective naming mode is KubernetesNode. AnnotationNodeHostnameOverride = NodeSetPrefix + "hostname-override" ) @@ -81,8 +80,10 @@ const ( // NOTE: Set by the NodeSet controller. LabelNodeSetPodIndex = NodeSetPrefix + "pod-index" - // LabelNodeSetPodHostname indicates the pod hostname (used as Slurm node name). - // NOTE: Set by the NodeSet controller. + // LabelNodeSetPodHostname records the Pod's Slurm node name in both scaling modes. + // The historical key does not imply equality with spec.hostname or the runtime hostname. + // Empty means the Slurm identity is unresolved. + // NOTE: Set by the NodeSet controller or Pod binding webhook. LabelNodeSetPodHostname = NodeSetPrefix + "pod-hostname" // LabelNodeSetPodProtect indicates whether the pod is protected against eviction using a PodDisruptionBudget @@ -92,6 +93,8 @@ const ( // LabelNodeSetScalingMode indicates the scaling mode (DaemonSet or StatefulSet). // NOTE: Set by the NodeSet controller. LabelNodeSetScalingMode = NodeSetPrefix + "scaling-mode" + + LabelNodeSetSlurmNodeNameMode = NodeSetPrefix + "slurm-node-name-mode" ) // Well Known Finalizers diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 9c7c9a1c..ee46c3c3 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -624,6 +624,11 @@ func (in *NodeSetSpec) DeepCopyInto(out *NodeSetSpec) { } in.UpdateStrategy.DeepCopyInto(&out.UpdateStrategy) out.PersistentVolumeClaimRetentionPolicy = in.PersistentVolumeClaimRetentionPolicy + if in.PreferKubernetesNodeName != nil { + in, out := &in.PreferKubernetesNodeName, &out.PreferKubernetesNodeName + *out = new(bool) + **out = **in + } if in.WorkloadDisruptionProtection != nil { in, out := &in.WorkloadDisruptionProtection, &out.WorkloadDisruptionProtection *out = new(bool) diff --git a/config/crd/bases/slinky.slurm.net_nodesets.yaml b/config/crd/bases/slinky.slurm.net_nodesets.yaml index f1519667..4170a7d3 100644 --- a/config/crd/bases/slinky.slurm.net_nodesets.yaml +++ b/config/crd/bases/slinky.slurm.net_nodesets.yaml @@ -186,6 +186,13 @@ spec: When disabled, all stored node pinnings are removed. Used only when `scalingMode=StatefulSet`. type: boolean + preferKubernetesNodeName: + default: true + description: |- + PreferKubernetesNodeName enables DaemonSet-style Slurm node naming for StatefulSet workers. + Only takes effect with pinToNode=true and oversubscribeNode=false. + Defaults to true. This preference is immutable after creation. + type: boolean pruneSlurmNodeRecords: default: Never description: PruneSlurmNodeRecords controls when the operator deletes @@ -397,6 +404,10 @@ spec: required: - controllerRef type: object + x-kubernetes-validations: + - message: preferKubernetesNodeName is immutable + rule: (!has(self.preferKubernetesNodeName) || self.preferKubernetesNodeName) + == (!has(oldSelf.preferKubernetesNodeName) || oldSelf.preferKubernetesNodeName) status: description: NodeSetStatus defines the observed state of NodeSet properties: diff --git a/docs/usage/nodeset-operations.md b/docs/usage/nodeset-operations.md index bc95770e..b816ffbe 100644 --- a/docs/usage/nodeset-operations.md +++ b/docs/usage/nodeset-operations.md @@ -24,9 +24,11 @@ primitives. For design-level details, see - [External Drain Preservation](#external-drain-preservation) - [External Health Checker Integration Pattern](#external-health-checker-integration-pattern) - [Node Identity](#node-identity) + - [DaemonSet Mode](#daemonset-mode) - [StatefulSet Mode](#statefulset-mode) - [Node Pinning](#node-pinning) - - [DaemonSet Mode](#daemonset-mode) + - [Kubernetes Node Names in Slurm](#kubernetes-node-names-in-slurm) + - [Pruning Slurm Node Records](#pruning-slurm-node-records) @@ -258,7 +260,7 @@ marks its Slurm node down, so the Slurm controller stops assigning work to a node that is going away: ```sh -scontrol update nodename=$(hostname) state=down reason='slurm-operator: Pod is terminating'; +scontrol update nodename="$SLURM_NODE_NAME" state=down reason='slurm-operator: Pod is terminating'; ``` Set `spec.slurmd.lifecycle.preStop` on the NodeSet to run something else @@ -279,7 +281,8 @@ spec: command: - /usr/bin/sh - -c - - scontrol update nodename=$(hostname) state=drain reason='slurm-operator: Pod is terminating'; + - >- + scontrol update nodename="$SLURM_NODE_NAME" state=drain reason='slurm-operator: Pod is terminating'; ``` The equivalent through the `slurm` Helm chart: @@ -294,7 +297,8 @@ nodesets: command: - /usr/bin/sh - -c - - scontrol update nodename=$(hostname) state=drain reason='slurm-operator: Pod is terminating'; + - >- + scontrol update nodename="$SLURM_NODE_NAME" state=drain reason='slurm-operator: Pod is terminating'; ``` Points to keep in mind: @@ -363,26 +367,36 @@ See [Override with Node Annotation](#override-with-node-annotation) and ## Node Identity -A Nodeset's scalingMode will determine whether its pods, which represent Slurm -nodes, are loosely or strictly mapped to the Kubernetes nodes they run on. +A NodeSet's `scalingMode` determines whether its Pods, which represent Slurm +nodes, are loosely or strictly mapped to the Kubernetes Nodes they run on. + +### DaemonSet Mode + +When using `scalingMode=DaemonSet`, NodeSet Pods are strictly mapped to +Kubernetes Nodes. Their hostname is the Node's +`nodeset.slinky.slurm.net/hostname-override` value, if set, otherwise the Node +name up to the first dot. ### StatefulSet Mode -When using `scalingMode=StatefulSet`, Nodeset pods are loosely mapped to -Kubernetes nodes and may be rescheduled freely. +When using `scalingMode=StatefulSet`, NodeSet Pods may be loosely mapped to +Kubernetes Nodes and may be rescheduled freely. + +Pod names remain ordinal-based. The configured `spec.hostname` is the Pod +template's hostname prefix plus ordinal, or the Pod name if no prefix is set. +Without node pinning, Slurm uses this configured hostname as the node name. If a stricter node mapping is preferred, node pinning can be enabled on the NodeSet. #### Node Pinning -When enabled, NodeSet pods are pinned to the Kubernetes node it was first -scheduled on. Once a pod is assigned to a node, subsequent recreations of that -pod (e.g. after eviction, deletion, or node maintenance) will always land on the -same physical node. If the node is unavailable, the pod remains in `Pending` -state until the node comes back. However, node pinnings will be removed under -specific conditions: if the node no longer exists; or if the new NodeSet pod no -longer matches the node it was pinned to (e.g. affinity, nodeSelector). +When enabled, each NodeSet Pod is pinned to the Kubernetes Node it was first +scheduled on. Subsequent recreations of that Pod (e.g. after eviction, deletion, +or node maintenance) return to the same Node while the pin remains valid. If the +Node is unavailable, the Pod remains in `Pending` state until the Node comes +back. The pin is removed if the Node no longer exists or no longer matches the +Pod template (e.g. affinity, nodeSelector). To use node pinning, set `pinToNode=true` on a NodeSet in the Slurm Helm chart: @@ -404,21 +418,108 @@ spec: replicas: 4 ``` -When enabled, the controller: +With node pinning enabled: -1. The pod is initially scheduled like normal. -1. Records the node-to-pod mapping in `status.nodeToOrdinal`. +1. The Pod is initially scheduled normally. +1. The controller records the node-to-pod mapping in `status.nodeToOrdinal`. 1. On subsequent pod recreations, a [node affinity][node-affinity] is added to the pod such that it can only be scheduled to the recorded node. -1. Reset the node in the node-to-pod map if: +1. The controller resets the node in the node-to-pod map if: - the Kubernetes node no longer exists - the NodeSet pod template no longer matches the recorded Kubernetes Node (e.g. affinity, nodeSelector). -### DaemonSet Mode +##### Kubernetes Node Names in Slurm + +Pinning controls placement; `spec.preferKubernetesNodeName` also lets pinned +StatefulSet workers register in Slurm using the Node's hostname override or +short name instead of the Pod hostname. This boolean defaults to `true` when +omitted or set to YAML `null`. Set it to `false` when creating a NodeSet to keep +Pod-hostname naming even with pinning enabled. Node-derived naming requires both +`pinToNode: true` and `oversubscribeNode: false`. + +To use the same Node-derived naming rule as DaemonSet mode while retaining +replica-based scaling: + +```yaml +nodesets: + slinky: + scalingMode: StatefulSet + preferKubernetesNodeName: true + pinToNode: true + oversubscribeNode: false +``` + +These fields can also be set directly on the NodeSet's `spec`. Disabling pinning +or enabling oversubscription falls back to Pod-hostname naming, which uses the +Pod's configured `spec.hostname` as its Slurm name. + +Node-derived naming changes only the Slurm name. The StatefulSet Pod's name and +configured `spec.hostname` remain ordinal-based on both first creation and +pinned recreation. + +With Node-derived naming enabled, the operator passes the Node's hostname +override or short name explicitly to slurmd. Resolved names must be valid Pod +hostnames (a DNS label of at most 63 characters). The binding webhook rejects +invalid names before the Pod starts. + +During autoscaling, worker Pods can wait for Nodes that do not exist yet. Their +Slurm names remain unresolved until binding; unresolved workers are excluded +from Slurm operations while still counting toward Kubernetes replicas. + +The preference itself is immutable after creation, but `pinToNode` and +`oversubscribeNode` remain mutable. + +The operator-managed `nodeset.slinky.slurm.net/pod-hostname` label records the +Pod's Slurm node name in both scaling modes. Despite its historical name, this +Slurm node name label need not match the Pod's configured `spec.hostname` or +runtime hostname. In a StatefulSet using Node-derived naming, it holds the +Node-derived Slurm name while `spec.hostname` remains ordinal-based. The label +can be empty until binding resolves the name if no valid pin is available; an +empty value means unresolved, not a fallback to the Pod hostname. + +The slurmd container sources `SLURM_NODE_NAME` from this label through the +Downward API and passes it to slurmd's native `-N` option. The default +termination hook uses the same variable. Custom images and startup overrides +must preserve the recorded Slurm identity. The separate +`nodeset.slinky.slurm.net/slurm-node-name-mode` Pod label is reserved for the +operator. + +With host networking, the runtime hostname may differ from `spec.hostname`; +slurmd still uses the explicitly supplied Slurm name. + +Legacy host-networked StatefulSet workers using implicit Node names must be +drained before upgrading and recreated with the new controller. + +To change the preference itself, create a new NodeSet and retire the old one +after draining its workloads. + +If a pin is released, the replacement Pod can run on another Node and register +under that Node's name. Existing scheduling and eviction policies still apply; +this option does not force deletion of Pods on `NotReady` Nodes. + +For cleanup of Slurm records after node replacement, see +[Pruning Slurm Node Records](#pruning-slurm-node-records). + +### Pruning Slurm Node Records + +`spec.pruneSlurmNodeRecords` controls cleanup of owned, defunct Slurm node +records. The default `Never` policy leaves records in place for manual cleanup. +Set it to `NodeNotFound` to allow the operator to remove records after their +Node-backed identity changes or their pin is lost, including records left behind +by hostname overrides. + +In the Slurm Helm chart: + +```yaml +nodesets: + slinky: + pruneSlurmNodeRecords: NodeNotFound +``` -When using `scalingMode=Daemonset`, Nodeset pods are strictly mapped to -Kubernetes nodes and share the hostname of the node they run on. +This field can also be set directly on the NodeSet's `spec`. A valid pin retains +the record across Pod restarts. Deleting a Pod does not itself delete its Slurm +record. diff --git a/helm/slurm-operator-crds/templates/slinky.slurm.net_nodesets.yaml b/helm/slurm-operator-crds/templates/slinky.slurm.net_nodesets.yaml index f1519667..4170a7d3 100644 --- a/helm/slurm-operator-crds/templates/slinky.slurm.net_nodesets.yaml +++ b/helm/slurm-operator-crds/templates/slinky.slurm.net_nodesets.yaml @@ -186,6 +186,13 @@ spec: When disabled, all stored node pinnings are removed. Used only when `scalingMode=StatefulSet`. type: boolean + preferKubernetesNodeName: + default: true + description: |- + PreferKubernetesNodeName enables DaemonSet-style Slurm node naming for StatefulSet workers. + Only takes effect with pinToNode=true and oversubscribeNode=false. + Defaults to true. This preference is immutable after creation. + type: boolean pruneSlurmNodeRecords: default: Never description: PruneSlurmNodeRecords controls when the operator deletes @@ -397,6 +404,10 @@ spec: required: - controllerRef type: object + x-kubernetes-validations: + - message: preferKubernetesNodeName is immutable + rule: (!has(self.preferKubernetesNodeName) || self.preferKubernetesNodeName) + == (!has(oldSelf.preferKubernetesNodeName) || oldSelf.preferKubernetesNodeName) status: description: NodeSetStatus defines the observed state of NodeSet properties: diff --git a/helm/slurm/README.md b/helm/slurm/README.md index 9e3c528f..d4775ffd 100644 --- a/helm/slurm/README.md +++ b/helm/slurm/README.md @@ -132,7 +132,7 @@ Kubernetes: `>= 1.29.0-0` | loginsets | map[string]object | `{}` | Slurm LoginSet (sackd, sshd, sssd) configurations. | | nameOverride | string | `nil` | Overrides the name of the release. | | namespaceOverride | string | `nil` | Overrides the namespace of the release. | -| nodesetDefaults | object | `{"enabled":true,"extraConf":null,"extraConfMap":{},"logfile":{"image":{"digest":null,"repository":"docker.io/library/alpine","tag":"latest"},"resources":{}},"metadata":{},"ordinalPadding":0,"oversubscribeNode":false,"partition":{"config":null,"configMap":{},"enabled":false},"pinToNode":false,"podSpec":{"affinity":{},"initContainers":[],"nodeSelector":{"kubernetes.io/os":"linux"},"resources":{},"tolerations":[],"volumes":[]},"pruneSlurmNodeRecords":"Never","replicas":1,"scalingMode":"StatefulSet","slurmd":{"args":[],"env":[],"image":{"digest":null,"repository":"ghcr.io/slinkyproject/slurmd","tag":"26.05-ubuntu26.04"},"lifecycle":{},"resources":{},"volumeMounts":[]},"ssh":{"enabled":false,"extraSshdConfig":null},"updateStrategy":{"rollingUpdate":{"maxUnavailable":"25%"},"scheduledUpdate":{},"type":"RollingUpdate"},"workloadDisruptionProtection":true}` | Defines defaults for the NodeSet map values. | +| nodesetDefaults | object | `{"enabled":true,"extraConf":null,"extraConfMap":{},"logfile":{"image":{"digest":null,"repository":"docker.io/library/alpine","tag":"latest"},"resources":{}},"metadata":{},"ordinalPadding":0,"oversubscribeNode":false,"partition":{"config":null,"configMap":{},"enabled":false},"pinToNode":false,"podSpec":{"affinity":{},"initContainers":[],"nodeSelector":{"kubernetes.io/os":"linux"},"resources":{},"tolerations":[],"volumes":[]},"preferKubernetesNodeName":true,"pruneSlurmNodeRecords":"Never","replicas":1,"scalingMode":"StatefulSet","slurmd":{"args":[],"env":[],"image":{"digest":null,"repository":"ghcr.io/slinkyproject/slurmd","tag":"26.05-ubuntu26.04"},"lifecycle":{},"resources":{},"volumeMounts":[]},"ssh":{"enabled":false,"extraSshdConfig":null},"updateStrategy":{"rollingUpdate":{"maxUnavailable":"25%"},"scheduledUpdate":{},"type":"RollingUpdate"},"workloadDisruptionProtection":true}` | Defines defaults for the NodeSet map values. | | nodesetDefaults.enabled | bool | `true` | Enable use of this NodeSet. | | nodesetDefaults.extraConf | string | `nil` | Raw extra configuration added to the `--conf` argument. Ref: https://slurm.schedmd.com/slurmd.html#OPT_conf-%3Cnode-parameters%3E Ref: https://slurm.schedmd.com/slurm.conf.html#SECTION_NODE-CONFIGURATION | | nodesetDefaults.extraConfMap | map[string]string \| map[string][]string | `{}` | Extra configuration added to the `--conf` option. If `extraConf` is not empty, it takes precedence. Ref: https://slurm.schedmd.com/slurmd.html#OPT_conf-%3Cnode-parameters%3E Ref: https://slurm.schedmd.com/slurm.conf.html#SECTION_NODE-CONFIGURATION | @@ -152,13 +152,14 @@ Kubernetes: `>= 1.29.0-0` | nodesetDefaults.podSpec.resources | object | `{}` | The pod resource limits and requests. Ref: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#resource-requests-and-limits-of-pod-and-container | | nodesetDefaults.podSpec.tolerations | list | `[]` | Tolerations for pod assignment. Ref: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/ | | nodesetDefaults.podSpec.volumes | list | `[]` | List of volumes to use. Ref: https://kubernetes.io/docs/concepts/storage/volumes/ | +| nodesetDefaults.preferKubernetesNodeName | bool | `true` | Prefer Node-derived Slurm names for pinned, non-oversubscribed StatefulSets; otherwise use Pod hostnames. Immutable preference, defaults to true; set false to opt out. DaemonSet naming is unchanged. | | nodesetDefaults.pruneSlurmNodeRecords | string | `"Never"` | Control when the operator deletes Slurm node records. One of: Never; NodeNotFound. | | nodesetDefaults.replicas | int | `1` | Number of replicas to deploy. Ignored when scalingMode is daemonset. | | nodesetDefaults.scalingMode | string | `"StatefulSet"` | Scaling mode: "StatefulSet" (fixed replica count) or "DaemonSet" (one pod per matching node). | | nodesetDefaults.slurmd.args | list | `[]` | Arguments passed to the image. Ref: https://slurm.schedmd.com/slurmd.html#SECTION_OPTIONS | | nodesetDefaults.slurmd.env | list | `[]` | Environment passed to the image. | | nodesetDefaults.slurmd.image | string \| object | `{"digest":null,"repository":"ghcr.io/slinkyproject/slurmd","tag":"26.05-ubuntu26.04"}` | The image to use. Ref: https://kubernetes.io/docs/concepts/containers/images/#image-names | -| nodesetDefaults.slurmd.lifecycle | corev1.Lifecycle | `{}` | The container lifecycle hooks. A `preStop` hook set here replaces the operator default, which marks the Slurm node down as the pod terminates: `scontrol update nodename=$(hostname) state=down reason='slurm-operator: Pod is terminating';` Ref: https://kubernetes.io/docs/concepts/containers/container-lifecycle-hooks/ | +| nodesetDefaults.slurmd.lifecycle | corev1.Lifecycle | `{}` | The container lifecycle hooks. A `preStop` hook set here replaces the operator default, which marks the Slurm node down as the pod terminates: `scontrol update nodename="$SLURM_NODE_NAME" state=down reason='slurm-operator: Pod is terminating';` Ref: https://kubernetes.io/docs/concepts/containers/container-lifecycle-hooks/ | | nodesetDefaults.slurmd.resources | object | `{}` | The container resource limits and requests. Ref: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#resource-requests-and-limits-of-pod-and-container | | nodesetDefaults.slurmd.volumeMounts | list | `[]` | List of volume mounts to use. Ref: https://kubernetes.io/docs/concepts/storage/volumes/ | | nodesetDefaults.ssh.enabled | bool | `false` | Enable SSH access to worker pods with pam_slurm_adopt. Ref: https://slurm.schedmd.com/pam_slurm_adopt.html | diff --git a/helm/slurm/templates/nodeset/nodeset-cr.yaml b/helm/slurm/templates/nodeset/nodeset-cr.yaml index a628823b..793a5cf8 100644 --- a/helm/slurm/templates/nodeset/nodeset-cr.yaml +++ b/helm/slurm/templates/nodeset/nodeset-cr.yaml @@ -48,6 +48,7 @@ spec: {{- end }}{{- /* if .enabled */}} {{- end }}{{- /* with $nodeset.ssh */}} scalingMode: {{ $nodeset.scalingMode }} + preferKubernetesNodeName: {{ if eq $nodeset.preferKubernetesNodeName nil }}true{{ else }}{{ $nodeset.preferKubernetesNodeName }}{{ end }} replicas: {{ $nodeset.replicas }} slurmd: {{- $_ := set $slurmd "imagePullPolicy" (get $slurmd "imagePullPolicy" | default $.Values.imagePullPolicy) -}} diff --git a/helm/slurm/tests/__snapshot__/nodeset_test.yaml.snap b/helm/slurm/tests/__snapshot__/nodeset_test.yaml.snap index 4889a5b4..b6803d32 100644 --- a/helm/slurm/tests/__snapshot__/nodeset_test.yaml.snap +++ b/helm/slurm/tests/__snapshot__/nodeset_test.yaml.snap @@ -22,6 +22,7 @@ manifest should match snapshot: partition: enabled: false pinToNode: false + preferKubernetesNodeName: true pruneSlurmNodeRecords: Never replicas: 1 scalingMode: StatefulSet diff --git a/helm/slurm/tests/nodeset_test.yaml b/helm/slurm/tests/nodeset_test.yaml index b90482e7..0c9d737f 100644 --- a/helm/slurm/tests/nodeset_test.yaml +++ b/helm/slurm/tests/nodeset_test.yaml @@ -10,6 +10,91 @@ chart: version: 1.2.3 appVersion: 1.2.3 tests: + - it: should enable the naming preference by default without enabling pinning + set: + nodesets.slinky.enabled: true + asserts: + - equal: + path: spec.preferKubernetesNodeName + value: true + - equal: + path: spec.pinToNode + value: false + - notExists: + path: spec.slurmNodeNameMode + - it: should accept null naming defaults + set: + nodesetDefaults.preferKubernetesNodeName: null + nodesets.slinky.enabled: true + asserts: + - equal: + path: spec.preferKubernetesNodeName + value: true + - it: should allow a null nodeset preference to select the default true + set: + nodesetDefaults.preferKubernetesNodeName: false + nodesets.slinky.enabled: true + nodesets.slinky.preferKubernetesNodeName: null + asserts: + - equal: + path: spec.preferKubernetesNodeName + value: true + - it: should preserve daemonset defaults + set: + nodesets.slinky.enabled: true + nodesets.slinky.scalingMode: DaemonSet + asserts: + - equal: + path: spec.preferKubernetesNodeName + value: true + - equal: + path: spec.scalingMode + value: DaemonSet + - it: should inherit preferred node naming + set: + nodesetDefaults.pinToNode: true + nodesets.slinky.enabled: true + asserts: + - equal: + path: spec.preferKubernetesNodeName + value: true + - equal: + path: spec.scalingMode + value: StatefulSet + - equal: + path: spec.pinToNode + value: true + - equal: + path: spec.oversubscribeNode + value: false + - it: should preserve explicit false in nodeset defaults + set: + nodesetDefaults.preferKubernetesNodeName: false + nodesetDefaults.pinToNode: true + nodesets.slinky.enabled: true + asserts: + - equal: + path: spec.preferKubernetesNodeName + value: false + - it: should allow an explicit true to override false defaults + set: + nodesetDefaults.preferKubernetesNodeName: false + nodesets.slinky.enabled: true + nodesets.slinky.preferKubernetesNodeName: true + asserts: + - equal: + path: spec.preferKubernetesNodeName + value: true + - it: should disable the preference per nodeset + set: + nodesetDefaults.preferKubernetesNodeName: true + nodesetDefaults.pinToNode: true + nodesets.slinky.enabled: true + nodesets.slinky.preferKubernetesNodeName: false + asserts: + - equal: + path: spec.preferKubernetesNodeName + value: false - it: manifest should match snapshot set: nodesets: diff --git a/helm/slurm/values.yaml b/helm/slurm/values.yaml index a28ce2b0..1d635297 100644 --- a/helm/slurm/values.yaml +++ b/helm/slurm/values.yaml @@ -723,6 +723,8 @@ nodesetDefaults: enabled: true # -- Scaling mode: "StatefulSet" (fixed replica count) or "DaemonSet" (one pod per matching node). scalingMode: StatefulSet + # -- Prefer Node-derived Slurm names for pinned, non-oversubscribed StatefulSets; otherwise use Pod hostnames. Immutable preference, defaults to true; set false to opt out. DaemonSet naming is unchanged. + preferKubernetesNodeName: true # -- Number of replicas to deploy. Ignored when scalingMode is daemonset. replicas: 1 # -- Pin pods to their initially assigned Kubernetes nodes. @@ -765,7 +767,7 @@ nodesetDefaults: # - name: nfs-home # mountPath: /home # -- (corev1.Lifecycle) The container lifecycle hooks. - # A `preStop` hook set here replaces the operator default, which marks the Slurm node down as the pod terminates: `scontrol update nodename=$(hostname) state=down reason='slurm-operator: Pod is terminating';` + # A `preStop` hook set here replaces the operator default, which marks the Slurm node down as the pod terminates: `scontrol update nodename="$SLURM_NODE_NAME" state=down reason='slurm-operator: Pod is terminating';` # Ref: https://kubernetes.io/docs/concepts/containers/container-lifecycle-hooks/ lifecycle: {} # The block below is the operator default. Uncomment and edit it to run @@ -775,7 +777,8 @@ nodesetDefaults: # command: # - /usr/bin/sh # - -c - # - scontrol update nodename=$(hostname) state=down reason='slurm-operator: Pod is terminating'; + # - >- + # scontrol update nodename="$SLURM_NODE_NAME" state=down reason='slurm-operator: Pod is terminating'; # LogFile sidecar configurations. logfile: # -- (string \| object) The image to use. diff --git a/internal/builder/workerbuilder/worker_app.go b/internal/builder/workerbuilder/worker_app.go index cd49ebc6..887888de 100644 --- a/internal/builder/workerbuilder/worker_app.go +++ b/internal/builder/workerbuilder/worker_app.go @@ -195,6 +195,13 @@ func (b *WorkerBuilder) slurmdContainer(nodeset *slinkyv1beta1.NodeSet, controll cpus, memory := b.getResourceLimits(&nodeset.Spec) + slurmNodeNameEnv := corev1.EnvVar{ + Name: "SLURM_NODE_NAME", + ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: fmt.Sprintf("metadata.labels['%s']", slinkyv1beta1.LabelNodeSetPodHostname)}, + }, + } + opts := common.ContainerOpts{ Base: corev1.Container{ Name: labels.WorkerApp, @@ -216,6 +223,7 @@ func (b *WorkerBuilder) slurmdContainer(nodeset *slinkyv1beta1.NodeSet, controll Name: "POD_MEMORY", Value: strconv.FormatInt(memory, 10), }, + slurmNodeNameEnv, }, Ports: ports, StartupProbe: &corev1.Probe{ @@ -281,14 +289,14 @@ func slurmdPreStop() *corev1.LifecycleHandler { Command: []string{ "/usr/bin/sh", "-c", - "scontrol update nodename=$(hostname) state=down reason='slurm-operator: Pod is terminating';", + `scontrol update nodename="$SLURM_NODE_NAME" state=down reason='slurm-operator: Pod is terminating';`, }, }, } } func slurmdArgs(nodeset *slinkyv1beta1.NodeSet, controller *slinkyv1beta1.Controller) []string { - args := []string{"-Z"} + args := []string{"-Z", "-N", "$(SLURM_NODE_NAME)"} args = append(args, common.ConfiglessArgs(controller)...) args = append(args, slurmdConfArgs(nodeset)...) return args diff --git a/internal/builder/workerbuilder/worker_app_test.go b/internal/builder/workerbuilder/worker_app_test.go index bd3c3984..fdf2a791 100644 --- a/internal/builder/workerbuilder/worker_app_test.go +++ b/internal/builder/workerbuilder/worker_app_test.go @@ -23,6 +23,28 @@ import ( "github.com/SlinkyProject/slurm-operator/internal/builder/labels" ) +func TestSlurmdContainerNodeName(t *testing.T) { + const hostnameFieldPath = "metadata.labels['" + slinkyv1beta1.LabelNodeSetPodHostname + "']" + nodeset := &slinkyv1beta1.NodeSet{} + container := New(fake.NewFakeClient()).slurmdContainer(nodeset, &slinkyv1beta1.Controller{}) + require.Empty(t, container.Command) + require.GreaterOrEqual(t, len(container.Args), 3) + require.Equal(t, []string{"-Z", "-N", "$(SLURM_NODE_NAME)"}, container.Args[:3]) + require.Contains(t, container.Env, corev1.EnvVar{ + Name: "SLURM_NODE_NAME", + ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: hostnameFieldPath}, + }, + }) + require.NotNil(t, container.Lifecycle) + require.NotNil(t, container.Lifecycle.PreStop) + require.NotNil(t, container.Lifecycle.PreStop.Exec) + require.Equal(t, []string{ + "/usr/bin/sh", "-c", + `scontrol update nodename="$SLURM_NODE_NAME" state=down reason='slurm-operator: Pod is terminating';`, + }, container.Lifecycle.PreStop.Exec.Command) +} + func TestBuilder_BuildWorkerPodTemplate(t *testing.T) { type fields struct { client client.Client diff --git a/internal/controller/nodeset/nodeset_history.go b/internal/controller/nodeset/nodeset_history.go index fa2a8677..13bde48d 100644 --- a/internal/controller/nodeset/nodeset_history.go +++ b/internal/controller/nodeset/nodeset_history.go @@ -199,6 +199,9 @@ func getPatch(nodeset *slinkyv1beta1.NodeSet) ([]byte, error) { // revision patch must be manually added here. specCopy["ordinalPadding"] = nodeset.Spec.OrdinalPadding specCopy["oversubscribeNode"] = nodeset.Spec.OversubscribeNode + if nodeset.Spec.ScalingMode != slinkyv1beta1.ScalingModeDaemonset && nodeset.Spec.EffectiveSlurmNodeNameMode() == slinkyv1beta1.SlurmNodeNameModeKubernetesNode { + specCopy["preferKubernetesNodeName"] = true + } if slurmd, ok := spec["slurmd"].(map[string]any); ok { slurmd["$patch"] = "replace" specCopy["slurmd"] = slurmd diff --git a/internal/controller/nodeset/nodeset_history_test.go b/internal/controller/nodeset/nodeset_history_test.go index e5b9d700..8baac4a5 100644 --- a/internal/controller/nodeset/nodeset_history_test.go +++ b/internal/controller/nodeset/nodeset_history_test.go @@ -22,6 +22,46 @@ import ( "github.com/SlinkyProject/slurm-operator/internal/utils/structutils" ) +func TestDefaultNodeNamePreferenceRevision(t *testing.T) { + const legacyPatch = `{"spec":{"extraConf":"Weight=10",` + + `"logfile":{"$patch":"replace","image":"alpine","name":"","resources":{}},` + + `"ordinalPadding":0,"oversubscribeNode":false,` + + `"slurmd":{"$patch":"replace","image":"slurmd","name":"","resources":{}},` + + `"template":{"$patch":"replace","metadata":{"labels":{"foo":"bar"}},"spec":{"containers":null}}}}` + const legacyRevisionName = "workers-f8bb4d887" + + nodeset := newNodeSet("workers", "slurm", 2) + legacy, err := newRevision(nodeset, 1, ptr.To[int32](0)) + require.NoError(t, err) + require.Equal(t, legacyPatch, string(legacy.Data.Raw), "patch captured from commit 399ee2039253b6912c81a6d7bbc8eccc06e27744") + require.Equal(t, legacyRevisionName, legacy.Name, "revision name captured from commit 399ee2039253b6912c81a6d7bbc8eccc06e27744") + nodeset.Spec.PreferKubernetesNodeName = ptr.To(false) + explicitFalse, err := newRevision(nodeset, 2, ptr.To[int32](0)) + require.NoError(t, err) + require.True(t, history.EqualRevision(legacy, explicitFalse)) + nodeset.Spec.PinToNode = true + pinnedLegacy, err := newRevision(nodeset, 3, ptr.To[int32](0)) + require.NoError(t, err) + require.True(t, history.EqualRevision(legacy, pinnedLegacy)) + nodeset.Spec.PreferKubernetesNodeName = ptr.To(true) + nodeNamed, err := newRevision(nodeset, 4, ptr.To[int32](0)) + require.NoError(t, err) + require.False(t, history.EqualRevision(legacy, nodeNamed)) + nodeset.Spec.PreferKubernetesNodeName = nil + defaultNodeNamed, err := newRevision(nodeset, 5, ptr.To[int32](0)) + require.NoError(t, err) + require.True(t, history.EqualRevision(nodeNamed, defaultNodeNamed)) + nodeset.Spec.PinToNode = false + fallback, err := newRevision(nodeset, 6, ptr.To[int32](0)) + require.NoError(t, err) + require.True(t, history.EqualRevision(legacy, fallback)) + nodeset.Spec.PinToNode = true + nodeset.Spec.OversubscribeNode = true + oversubscribed, err := newRevision(nodeset, 7, ptr.To[int32](0)) + require.NoError(t, err) + require.False(t, history.EqualRevision(nodeNamed, oversubscribed)) +} + func TestNodeSetReconciler_truncateHistory(t *testing.T) { const clusterName = "slurm" type fields struct { diff --git a/internal/controller/nodeset/nodeset_sync.go b/internal/controller/nodeset/nodeset_sync.go index 2201696f..811469c6 100644 --- a/internal/controller/nodeset/nodeset_sync.go +++ b/internal/controller/nodeset/nodeset_sync.go @@ -9,7 +9,9 @@ import ( "fmt" "slices" "sort" + "strconv" "strings" + "sync" "time" "unicode" @@ -20,6 +22,7 @@ import ( k8slabels "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/klog/v2" podutil "k8s.io/kubernetes/pkg/api/v1/pod" kubecontroller "k8s.io/kubernetes/pkg/controller" @@ -620,89 +623,105 @@ func (r *NodeSetReconciler) syncSlurmNodeRecordsNodeNotFound( nodeset *slinkyv1beta1.NodeSet, ) error { mainLogger := log.FromContext(ctx) - - switch nodeset.Spec.ScalingMode { - default: - fallthrough - case slinkyv1beta1.ScalingModeStatefulset: + daemonSet := nodeset.Spec.ScalingMode == slinkyv1beta1.ScalingModeDaemonset + if !daemonSet && !ptr.Deref(nodeset.Spec.PreferKubernetesNodeName, true) { return nil - case slinkyv1beta1.ScalingModeDaemonset: - defunctNodes, err := r.slurmControl.GetDefunctNodesForNodeSet(ctx, nodeset) - if err != nil { - if errors.Is(err, slurmcontrol.ErrNoSlurmClient) { - return nil - } - return err - } + } - syncSlurmNodeRecordsFn := func(i int) error { - defunctNode := defunctNodes[i] - podKey := types.NamespacedName{ - Namespace: defunctNode.PodInfo.Namespace, - Name: defunctNode.PodInfo.PodName, - } + defunctNodes, err := r.slurmControl.GetDefunctNodesForNodeSet(ctx, nodeset) + if err != nil { + if errors.Is(err, slurmcontrol.ErrNoSlurmClient) { + return nil + } + return err + } + getPins := sync.OnceValues(func() (map[string]string, error) { + return r.calculateOrdinalToNode(ctx, nodeset, nil) + }) - // If the pod still exists it is not defunct -- skip. - pod := &corev1.Pod{} - if err := r.Get(ctx, podKey, pod); err == nil { + syncSlurmNodeRecordsFn := func(index int) error { + defunctNode := defunctNodes[index] + podKey := types.NamespacedName{ + Namespace: defunctNode.PodInfo.Namespace, + Name: defunctNode.PodInfo.PodName, + } + kubeNodeKey := types.NamespacedName{Name: defunctNode.PodInfo.Node} + logger := mainLogger.WithValues("slurmNode", defunctNode.Name, "pod", podKey, "kubeNode", kubeNodeKey.Name) + + pod := &corev1.Pod{} + switch err := r.Get(ctx, podKey, pod); { + case err == nil: + if daemonSet || + !metav1.IsControlledBy(pod, nodeset) || + !podutils.IsRunning(pod) || + podutils.IsTerminating(pod) || + nodesetutils.GetSlurmNodeName(pod) == defunctNode.Name { return nil - } else if !apierrors.IsNotFound(err) { - return err } + case apierrors.IsNotFound(err): + if daemonSet { + if kubeNodeKey.Name == "" { + logger.V(2).Info("Skipping defunct Slurm node deletion because PodInfo does not include a Kubernetes node") + return nil + } - logger := mainLogger.WithValues("slurmNode", defunctNode.Name, "pod", podKey) - - if defunctNode.PodInfo.Node == "" { - logger.V(2).Info("Skipping defunct Slurm node deletion because PodInfo does not include a Kubernetes node") - return nil - } + kubeNode := &corev1.Node{} + switch err := r.Get(ctx, kubeNodeKey, kubeNode); { + case apierrors.IsNotFound(err): + // K8s node is gone -- let it be deleted. + case err != nil: + return err + default: + override := kubeNode.Annotations[slinkyv1beta1.AnnotationNodeHostnameOverride] + expected := nodesetutils.GetDaemonSetPodHostname(kubeNodeKey.Name, override) + + // Kubernetes nodes that still exist but do not match the DaemonSet pod's NodeSelector + // should be deleted + selectorMatch := true + for key, value := range nodeset.Spec.Template.PodSpecWrapper.NodeSelector { + nodeValue, ok := kubeNode.Labels[key] + if !ok || nodeValue != value { + selectorMatch = false + break + } + } - kubeNodeKey := types.NamespacedName{Name: defunctNode.PodInfo.Node} - logger = logger.WithValues("kubeNode", kubeNodeKey.Name) - kubeNode := &corev1.Node{} - switch err := r.Get(ctx, kubeNodeKey, kubeNode); { - case apierrors.IsNotFound(err): - // K8s node is gone -- let it be deleted. - case err != nil: - return err - default: - override := kubeNode.Annotations[slinkyv1beta1.AnnotationNodeHostnameOverride] - expected := nodesetutils.GetDaemonSetPodHostname(kubeNodeKey.Name, override) - - // Kubernetes nodes that still exist but do not match the DaemonSet pod's NodeSelector - // should be deleted - selectorMatch := true - for key, value := range nodeset.Spec.Template.PodSpecWrapper.NodeSelector { - nodeValue, ok := kubeNode.Labels[key] - if !ok || nodeValue != value { - selectorMatch = false - break + if expected == defunctNode.Name && selectorMatch { + logger.V(2).Info("Skipping defunct Slurm node deletion because the Kubernetes node still maps to it") + return nil } } - - if expected == defunctNode.Name && selectorMatch { - logger.V(2).Info("Skipping defunct Slurm node deletion because the Kubernetes node still maps to it") - return nil + } else { + pins, err := getPins() + if err != nil { + return err + } + ordinal := nodesetutils.GetOrdinal(&corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: podKey.Name}}) + if nodeName := pins[strconv.Itoa(ordinal)]; nodeName != "" || nodeset.Spec.EffectiveSlurmNodeNameMode() == slinkyv1beta1.SlurmNodeNameModePodHostname { + expectedPod := nodesetutils.NewNodeSetStatefulSetPod(r.Client, nodeset, &slinkyv1beta1.Controller{}, ordinal, "") + expectedPod.Spec.NodeName = nodeName + if nodesetutils.GetSlurmNodeName(expectedPod) == defunctNode.Name { + return nil + } } } - - // Prune the Slurm node: its backing pod is gone and the K8s node no longer maps here. - logger.V(1).Info("Deleting defunct Slurm node without a corresponding Kubernetes Pod/Node") - if err := r.slurmControl.DeleteNode(ctx, nodeset, defunctNode.Name); err != nil { - return fmt.Errorf("failed to delete defunct Slurm node %s for pod %s/%s on node %s: %w", - defunctNode.Name, podKey.Namespace, podKey.Name, kubeNodeKey.Name, err) - } - r.eventRecorder.Eventf(nodeset, nil, corev1.EventTypeNormal, DefunctSlurmNodePrunedReason, "Delete", - "Deleted defunct Slurm node %s: backing Pod %s/%s is gone and Kubernetes node %s no longer maps to its Slurm node", - defunctNode.Name, podKey.Namespace, podKey.Name, kubeNodeKey.Name) - return nil - } - if _, err := utils.SlowStartBatch(len(defunctNodes), utils.SlowStartInitialBatchSize, syncSlurmNodeRecordsFn); err != nil { + default: return err } + logger.V(1).Info("Deleting defunct Slurm node whose backing worker no longer maps to its identity") + if err := r.slurmControl.DeleteNode(ctx, nodeset, defunctNode.Name); err != nil { + return fmt.Errorf("failed to delete defunct Slurm node %s for pod %s/%s on node %s: %w", + defunctNode.Name, podKey.Namespace, podKey.Name, kubeNodeKey.Name, err) + } + r.eventRecorder.Eventf(nodeset, nil, corev1.EventTypeNormal, DefunctSlurmNodePrunedReason, "Delete", + "Deleted defunct Slurm node %s: its backing worker no longer maps to this identity", defunctNode.Name) return nil } + if _, err := utils.SlowStartBatch(len(defunctNodes), utils.SlowStartInitialBatchSize, syncSlurmNodeRecordsFn); err != nil { + return err + } + return nil } // syncSlurmNodes handles Slurm node drift where nodes may become unregistered but its pod is running and healthy. @@ -1003,19 +1022,7 @@ func (r *NodeSetReconciler) podsShouldBeOnNode( podsToDelete = append(podsToDelete, pod) default: - hostnameOverride := node.Annotations[slinkyv1beta1.AnnotationNodeHostnameOverride] - expectedHostname := nodesetutils.GetDaemonSetPodHostname(node.Name, hostnameOverride) - if pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] != expectedHostname { - mainLogger.V(2).Info("Daemon pod hostname mismatch detected, will recreate", - "pod", klog.KObj(pod), "node", klog.KObj(node), - "currentHostname", pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname], "expectedHostname", expectedHostname) - r.eventRecorder.Eventf(nodeset, pod, corev1.EventTypeNormal, "HostnameMismatch", "Info", - "Recreating daemon pod %s/%s: hostname changed from %q to %q", - pod.Namespace, pod.Name, pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname], expectedHostname) - podsToDelete = append(podsToDelete, pod) - } else { - daemonPodsRunning = append(daemonPodsRunning, pod) - } + daemonPodsRunning = append(daemonPodsRunning, pod) } } @@ -1041,6 +1048,71 @@ func (r *NodeSetReconciler) podsShouldBeOnNode( return nodesNeedingDaemonPods, podsToDelete } +func (r *NodeSetReconciler) getHostnameMismatches(ctx context.Context, nodeset *slinkyv1beta1.NodeSet, pods []*corev1.Pod) ([]*corev1.Pod, error) { + var mismatches []*corev1.Pod + if nodeset.Spec.EffectiveSlurmNodeNameMode() != slinkyv1beta1.SlurmNodeNameModeKubernetesNode { + return mismatches, nil + } + daemonSet := nodeset.Spec.ScalingMode == slinkyv1beta1.ScalingModeDaemonset + pins, err := r.calculateOrdinalToNode(ctx, nodeset, nil) + if err != nil { + return nil, err + } + for _, pod := range pods { + if podutils.IsTerminating(pod) { + continue + } + nodeName := pod.Spec.NodeName + if daemonSet { + if podutils.IsFailed(pod) || podutils.IsSucceeded(pod) { + continue + } + targetNodeName, err := daemonutils.GetTargetNodeName(pod) + if err != nil { + continue + } + nodeName = targetNodeName + } else { + if pod.Labels[slinkyv1beta1.LabelNodeSetSlurmNodeNameMode] != string(slinkyv1beta1.SlurmNodeNameModeKubernetesNode) { + continue + } + ordinal := nodesetutils.GetOrdinal(pod) + if !podutils.IsRunning(pod) || nodeName == "" || pins[strconv.Itoa(ordinal)] != nodeName { + continue + } + } + node := &corev1.Node{} + if err := r.Get(ctx, client.ObjectKey{Name: nodeName}, node); err != nil { + if daemonSet && apierrors.IsNotFound(err) { + continue + } + return nil, err + } + if daemonSet { + if _, shouldContinueRunning := r.NodeShouldRunDaemonPod(ctx, node, nodeset); !shouldContinueRunning { + continue + } + } + expectedName := nodesetutils.GetDaemonSetPodHostname(node.Name, node.Annotations[slinkyv1beta1.AnnotationNodeHostnameOverride]) + if !daemonSet { + if problems := utilvalidation.IsDNS1123Label(expectedName); len(problems) > 0 { + return nil, fmt.Errorf("invalid hostname override or node name on node %s: %v", nodeName, problems) + } + } + currentHostname := pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] + if currentHostname != expectedName { + log.FromContext(ctx).V(2).Info("Pod hostname mismatch detected, will recreate", + "pod", klog.KObj(pod), "node", klog.KObj(node), + "currentHostname", currentHostname, "expectedHostname", expectedName) + r.eventRecorder.Eventf(nodeset, pod, corev1.EventTypeNormal, "HostnameMismatch", "Info", + "Recreating %s pod %s/%s: Slurm name changed from %q to %q", + nodeset.Spec.ScalingMode, pod.Namespace, pod.Name, currentHostname, expectedName) + mismatches = append(mismatches, pod) + } + } + return mismatches, nil +} + // syncNodeSetPods will reconcile NodeSet pod replica counts. // Pods will be: // - Scaled out when: `replicaCount < replicasWant“ @@ -1066,6 +1138,11 @@ func (r *NodeSetReconciler) syncNodeSetPods( } } + mismatches, err := r.getHostnameMismatches(ctx, nodeset, podsNewScaling) + if err != nil { + return err + } + if nodeset.Spec.ScalingMode == slinkyv1beta1.ScalingModeDaemonset { logger.V(2).Info("Processing NodeSet pods in DaemonSet mode") nodeList := &corev1.NodeList{} @@ -1073,8 +1150,11 @@ func (r *NodeSetReconciler) syncNodeSetPods( return err } nodeToDaemonPods := r.getNodesToDaemonPods(ctx, nodeset, podsNewScaling, false) + for nodeName, daemonPods := range nodeToDaemonPods { + nodeToDaemonPods[nodeName] = nodesetutils.ExcludePods(daemonPods, mismatches) + } var nodesNeedingDaemonPods []string - var podsToDelete []*corev1.Pod + podsToDelete := mismatches for _, node := range nodeList.Items { nodesNeedingDaemonPodsOnNode, podsToDeleteOnNode := r.podsShouldBeOnNode( ctx, &node, nodeToDaemonPods, nodeset) @@ -1105,6 +1185,9 @@ func (r *NodeSetReconciler) syncNodeSetPods( } } else { logger.V(2).Info("Processing NodeSet pods in StatefulSet mode") + if len(mismatches) > 0 { + return r.doPodScale(ctx, nodeset, nil, mismatches, nil) + } // Handle replica scaling by comparing the known pods to the target number of replicas. // Create or delete pods as needed to reach the target number. diff --git a/internal/controller/nodeset/nodeset_sync_test.go b/internal/controller/nodeset/nodeset_sync_test.go index 05102bb8..3bf48501 100644 --- a/internal/controller/nodeset/nodeset_sync_test.go +++ b/internal/controller/nodeset/nodeset_sync_test.go @@ -49,6 +49,7 @@ import ( "github.com/SlinkyProject/slurm-operator/internal/controller/nodeset/podcontrol" "github.com/SlinkyProject/slurm-operator/internal/controller/nodeset/slurmcontrol" nodesetutils "github.com/SlinkyProject/slurm-operator/internal/controller/nodeset/utils" + "github.com/SlinkyProject/slurm-operator/internal/defaults" "github.com/SlinkyProject/slurm-operator/internal/utils/historycontrol" "github.com/SlinkyProject/slurm-operator/internal/utils/podinfo" "github.com/SlinkyProject/slurm-operator/internal/utils/podutils" @@ -1169,6 +1170,7 @@ func TestNodeSetReconciler_processCondemned(t *testing.T) { Namespace: corev1.NamespaceDefault, Name: "pod-0", }, + Spec: corev1.PodSpec{NodeName: "worker-a", Hostname: "pod-0"}, Status: corev1.PodStatus{ Phase: corev1.PodRunning, Conditions: []corev1.PodCondition{ @@ -1223,6 +1225,7 @@ func TestNodeSetReconciler_processCondemned(t *testing.T) { Namespace: corev1.NamespaceDefault, Name: "pod-0", }, + Spec: corev1.PodSpec{NodeName: "worker-a", Hostname: "pod-0"}, Status: corev1.PodStatus{ Phase: corev1.PodRunning, Conditions: []corev1.PodCondition{ @@ -1267,6 +1270,7 @@ func TestNodeSetReconciler_processCondemned(t *testing.T) { Namespace: corev1.NamespaceDefault, Name: "pod-0", }, + Spec: corev1.PodSpec{NodeName: "worker-a", Hostname: "pod-0"}, Status: corev1.PodStatus{ Phase: corev1.PodRunning, Conditions: []corev1.PodCondition{ @@ -1327,6 +1331,7 @@ func TestNodeSetReconciler_processCondemned(t *testing.T) { DeletionTimestamp: &now, Finalizers: []string{"test-finalizer"}, }, + Spec: corev1.PodSpec{NodeName: "worker-a", Hostname: "pod-0"}, Status: corev1.PodStatus{ Phase: corev1.PodRunning, Conditions: []corev1.PodCondition{ @@ -1381,6 +1386,7 @@ func TestNodeSetReconciler_processCondemned(t *testing.T) { Namespace: corev1.NamespaceDefault, Name: "pod-0", }, + Spec: corev1.PodSpec{NodeName: "worker-a", Hostname: "pod-0"}, Status: corev1.PodStatus{ Phase: corev1.PodRunning, Conditions: []corev1.PodCondition{ @@ -1435,6 +1441,7 @@ func TestNodeSetReconciler_processCondemned(t *testing.T) { Namespace: corev1.NamespaceDefault, Name: "pod-0", }, + Spec: corev1.PodSpec{NodeName: "worker-a", Hostname: "pod-0"}, Status: corev1.PodStatus{ Phase: corev1.PodRunning, Conditions: []corev1.PodCondition{ @@ -1499,6 +1506,7 @@ func TestNodeSetReconciler_processCondemned(t *testing.T) { Namespace: corev1.NamespaceDefault, Name: "pod-0", }, + Spec: corev1.PodSpec{NodeName: "worker-a", Hostname: "pod-0"}, Status: corev1.PodStatus{ Phase: corev1.PodRunning, Conditions: []corev1.PodCondition{ @@ -4988,6 +4996,109 @@ func TestNodeSetReconciler_podsShouldBeOnNode(t *testing.T) { } } +func TestDaemonSetHostnameMismatchReplacement(t *testing.T) { + for _, test := range []struct { + name, hostname string + phase corev1.PodPhase + wantMismatch bool + matchingPod bool + busy bool + terminating bool + missingNode bool + selectorMismatch bool + }{ + {name: "matching hostname", hostname: "gpu-a", phase: corev1.PodRunning}, + {name: "running mismatch", hostname: "gpu-old", phase: corev1.PodRunning, wantMismatch: true}, + {name: "pending unbound mismatch", hostname: "gpu-old", phase: corev1.PodPending, wantMismatch: true}, + {name: "mismatch excluded from duplicate cleanup", hostname: "gpu-old", phase: corev1.PodPending, wantMismatch: true, matchingPod: true}, + {name: "busy mismatch is drained", hostname: "gpu-old", phase: corev1.PodRunning, wantMismatch: true, busy: true}, + {name: "terminating pod is ignored", hostname: "gpu-old", phase: corev1.PodRunning, terminating: true}, + {name: "failed pod uses existing cleanup", hostname: "gpu-old", phase: corev1.PodFailed}, + {name: "succeeded pod uses existing cleanup", hostname: "gpu-old", phase: corev1.PodSucceeded}, + {name: "deleted target is ignored", hostname: "gpu-old", phase: corev1.PodRunning, missingNode: true}, + {name: "ineligible target uses placement cleanup", hostname: "gpu-old", phase: corev1.PodRunning, selectorMismatch: true}, + } { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + nodeset := newNodeSet("workers", "slurm", 1) + nodeset.UID = "workers-uid" + nodeset.Spec.ScalingMode = slinkyv1beta1.ScalingModeDaemonset + nodeset.Spec.UpdateStrategy.Type = slinkyv1beta1.OnDeleteNodeSetStrategyType + if test.selectorMismatch { + nodeset.Spec.Template.PodSpecWrapper.NodeSelector = map[string]string{"pool": "other"} + } + controller := &slinkyv1beta1.Controller{ObjectMeta: metav1.ObjectMeta{Name: "slurm", Namespace: nodeset.Namespace}} + node := newNodeForNodeSetTest("worker-a", map[string]string{"pool": "workers"}, false) + node.Annotations = map[string]string{slinkyv1beta1.AnnotationNodeHostnameOverride: "gpu-a"} + kubeClient := fake.NewFakeClient(nodeset, controller) + if !test.missingNode { + require.NoError(t, kubeClient.Create(ctx, node)) + } + pod := nodesetutils.NewNodeSetDaemonSetPod(kubeClient, nodeset, controller, node.Name, test.hostname, "") + pod.Name = "workers-old" + pod.UID = "workers-old-uid" + pod.Status.Phase = test.phase + if test.phase != corev1.PodPending { + pod.Spec.NodeName = node.Name + } + if test.terminating { + pod.Finalizers = []string{"test"} + } + require.NoError(t, kubeClient.Create(ctx, pod)) + if test.terminating { + require.NoError(t, kubeClient.Delete(ctx, pod)) + require.NoError(t, kubeClient.Get(ctx, client.ObjectKeyFromObject(pod), pod)) + } + pods := []*corev1.Pod{pod} + if test.matchingPod { + matching := pod.DeepCopy() + matching.Name = "workers-current" + matching.UID = "workers-current-uid" + matching.ResourceVersion = "" + matching.Spec.NodeName = node.Name + matching.Spec.Hostname = "gpu-a" + matching.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = "gpu-a" + matching.Status.Phase = corev1.PodRunning + matching.Status.Conditions = []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}} + require.NoError(t, kubeClient.Create(ctx, matching)) + pods = append(pods, matching) + } + state := []slurmapi.V0044NodeState{slurmapi.V0044NodeStateIDLE, slurmapi.V0044NodeStateDRAIN} + if test.busy { + state = []slurmapi.V0044NodeState{slurmapi.V0044NodeStateALLOCATED} + } + slurmClient := newFakeClientList(sinterceptor.Funcs{}, &slurmtypes.V0044NodeList{Items: []slurmtypes.V0044Node{{ + V0044Node: slurmapi.V0044Node{Name: ptr.To(test.hostname), State: &state}, + }}}) + reconciler := newNodeSetController(kubeClient, newClientMap("slurm", slurmClient)) + mismatches, err := reconciler.getHostnameMismatches(ctx, nodeset, pods) + require.NoError(t, err) + if !test.wantMismatch { + require.Empty(t, mismatches) + return + } + require.Equal(t, []*corev1.Pod{pod}, mismatches) + require.NoError(t, reconciler.syncNodeSetPods(ctx, nodeset, pods, "")) + current := &corev1.Pod{} + err = kubeClient.Get(ctx, client.ObjectKeyFromObject(pod), current) + wantRemaining := len(pods) + if test.busy { + require.NoError(t, err) + require.Equal(t, "true", current.Annotations[slinkyv1beta1.AnnotationPodCordon]) + } else { + require.True(t, apierrors.IsNotFound(err)) + wantRemaining-- + } + if test.matchingPod { + require.NoError(t, kubeClient.Get(ctx, client.ObjectKeyFromObject(pods[1]), &corev1.Pod{})) + } + remaining := &corev1.PodList{} + require.NoError(t, kubeClient.List(ctx, remaining)) + require.Len(t, remaining.Items, wantRemaining) + }) + } +} + func TestNodeSetReconciler_syncSlurmNodes(t *testing.T) { controller := &slinkyv1beta1.Controller{ ObjectMeta: metav1.ObjectMeta{ @@ -5101,6 +5212,377 @@ func TestNodeSetReconciler_syncSlurmNodes(t *testing.T) { } } +func TestNodeNamedScaleUpBeforeNodesExist(t *testing.T) { + ctx := context.Background() + nodeset := newNodeSet("workers", "slurm", 2) + nodeset.UID = "workers-uid" + nodeset.Spec.PinToNode = true + nodeset.Spec.PreferKubernetesNodeName = ptr.To(true) + defaults.SetNodeSetDefaults(nodeset) + controller := &slinkyv1beta1.Controller{ObjectMeta: metav1.ObjectMeta{ + Name: "slurm", Namespace: nodeset.Namespace, + }} + kubeClient := fake.NewClientBuilder().WithObjects(nodeset, controller). + WithStatusSubresource(&corev1.Pod{}).Build() + lookups := 0 + slurmClient := newFakeClientList(sinterceptor.Funcs{ + Get: func(context.Context, slurmobject.ObjectKey, slurmobject.Object, ...slurmclient.GetOption) error { + lookups++ + return errors.New("unexpected lookup before worker Nodes exist") + }, + }, &slurmtypes.V0044NodeList{}) + reconciler := newNodeSetController(kubeClient, newClientMap("slurm", slurmClient)) + revision, err := newRevision(nodeset, 1, ptr.To[int32](0)) + require.NoError(t, err) + hash := historycontrol.GetRevision(revision.Labels) + require.NoError(t, reconciler.syncNodeSetPods(ctx, nodeset, nil, hash)) + + podList := &corev1.PodList{} + require.NoError(t, kubeClient.List(ctx, podList)) + require.Len(t, podList.Items, 2) + for index := range podList.Items { + pod := &podList.Items[index] + pod.Status.Phase = corev1.PodPending + require.NoError(t, kubeClient.Status().Update(ctx, pod)) + require.NotEmpty(t, pod.Name) + require.Empty(t, pod.Spec.NodeName) + require.Empty(t, nodesetutils.GetSlurmNodeName(pod)) + } + pods := structutils.ReferenceList(podList.Items) + for range 2 { + require.NoError(t, reconciler.syncNodeSetPods(ctx, nodeset, pods, hash)) + require.NoError(t, reconciler.syncSlurmStatus(ctx, nodeset, pods)) + status, err := reconciler.calculateReplicaStatus(ctx, nodeset, pods, revision, revision) + require.NoError(t, err) + require.Equal(t, int32(2), status.Replicas) + require.Equal(t, int32(2), status.Updated) + require.Equal(t, int32(2), status.Unavailable) + slurmStatus, err := reconciler.slurmControl.CalculateNodeStatus(ctx, nodeset, pods) + require.NoError(t, err) + require.Zero(t, slurmStatus.Total) + } + for _, pod := range pods { + current := &corev1.Pod{} + require.NoError(t, kubeClient.Get(ctx, client.ObjectKeyFromObject(pod), current)) + require.Equal(t, pod.Spec, current.Spec) + require.Empty(t, nodesetutils.GetSlurmNodeName(current)) + require.False(t, podutils.IsPodCordon(current)) + } + + nodeset.Spec.Replicas = ptr.To[int32](0) + require.NoError(t, reconciler.syncNodeSetPods(ctx, nodeset, pods, hash)) + require.NoError(t, kubeClient.List(ctx, podList)) + require.Empty(t, podList.Items) + require.Zero(t, lookups) +} + +func TestPreferredNamingPlacementUpdates(t *testing.T) { + for _, change := range []struct { + name string + oldPinned, newPinned, oldOversubscribe, newOversubscribe bool + busy bool + }{ + {name: "enable pin", newPinned: true}, + {name: "disable pin", oldPinned: true}, + {name: "enable oversubscription", oldPinned: true, newPinned: true, newOversubscribe: true}, + {name: "disable oversubscription", oldPinned: true, newPinned: true, oldOversubscribe: true}, + {name: "busy worker switching to node naming", newPinned: true, busy: true}, + {name: "busy worker falling back to pod naming", oldPinned: true, busy: true}, + } { + for _, strategy := range []slinkyv1beta1.NodeSetUpdateStrategyType{slinkyv1beta1.RollingUpdateNodeSetStrategyType, slinkyv1beta1.OnDeleteNodeSetStrategyType, slinkyv1beta1.ScheduledUpdateNodeSetStrategyType} { + t.Run(change.name+"/"+string(strategy), func(t *testing.T) { + ctx := context.Background() + nodeset := newNodeSet("workers", "slurm", 1) + nodeset.UID = "workers-uid" + nodeset.Spec.PreferKubernetesNodeName = ptr.To(true) + nodeset.Spec.PinToNode = change.oldPinned + nodeset.Spec.OversubscribeNode = change.oldOversubscribe + nodeset.Spec.UpdateStrategy.Type = strategy + nodeset.Status.OrdinalToNode = map[string]string{"0": "worker-a"} + controller := &slinkyv1beta1.Controller{ObjectMeta: metav1.ObjectMeta{Name: "slurm", Namespace: nodeset.Namespace}} + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "worker-a", Annotations: map[string]string{slinkyv1beta1.AnnotationNodeHostnameOverride: "gpu-a"}}} + kclient := fake.NewFakeClient(nodeset, node, controller) + oldRevision, err := newRevision(nodeset, 1, ptr.To[int32](0)) + require.NoError(t, err) + pod := nodesetutils.NewNodeSetStatefulSetPod(kclient, nodeset, controller, 0, historycontrol.GetRevision(oldRevision.Labels)) + pod.Spec.NodeName = node.Name + pod.Status.Phase = corev1.PodRunning + pod.Status.Conditions = []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}} + require.NoError(t, kclient.Create(ctx, pod)) + oldName := nodesetutils.GetSlurmNodeName(pod) + nodeset.Spec.PinToNode = change.newPinned + nodeset.Spec.OversubscribeNode = change.newOversubscribe + revision, err := newRevision(nodeset, 2, ptr.To[int32](0)) + require.NoError(t, err) + require.False(t, history.EqualRevision(oldRevision, revision)) + hash := historycontrol.GetRevision(revision.Labels) + state := []slurmapi.V0044NodeState{slurmapi.V0044NodeStateIDLE, slurmapi.V0044NodeStateDRAIN} + if change.busy { + state = []slurmapi.V0044NodeState{slurmapi.V0044NodeStateALLOCATED} + } + sclient := newFakeClientList(sinterceptor.Funcs{}, &slurmtypes.V0044NodeList{Items: []slurmtypes.V0044Node{{V0044Node: slurmapi.V0044Node{ + Name: ptr.To(oldName), State: &state, + }}}}) + reconciler := newNodeSetController(kclient, newClientMap("slurm", sclient)) + mismatches, err := reconciler.getHostnameMismatches(ctx, nodeset, []*corev1.Pod{pod}) + require.NoError(t, err) + require.Empty(t, mismatches, "placement updates must use updateStrategy") + nodesetutils.UpdateIdentity(nodeset, pod) + require.Equal(t, oldName, nodesetutils.GetSlurmNodeName(pod)) + require.NoError(t, reconciler.syncUpdate(ctx, nodeset, []*corev1.Pod{pod}, hash)) + current := &corev1.Pod{} + err = kclient.Get(ctx, client.ObjectKeyFromObject(pod), current) + if strategy != slinkyv1beta1.RollingUpdateNodeSetStrategyType || change.busy { + require.NoError(t, err) + require.Equal(t, oldName, nodesetutils.GetSlurmNodeName(current)) + if strategy == slinkyv1beta1.RollingUpdateNodeSetStrategyType { + require.Equal(t, "true", current.Annotations[slinkyv1beta1.AnnotationPodCordon]) + } + return + } + require.True(t, apierrors.IsNotFound(err)) + require.NoError(t, reconciler.syncNodeSetPods(ctx, nodeset, nil, hash)) + require.NoError(t, kclient.Get(ctx, client.ObjectKeyFromObject(pod), current)) + want := "workers-0" + if change.newPinned && !change.newOversubscribe { + want = "gpu-a" + } + require.Equal(t, want, nodesetutils.GetSlurmNodeName(current)) + require.Equal(t, pod.Spec.Volumes, current.Spec.Volumes) + }) + } + } +} + +func TestStatefulSetHostnameOverrideReplacement(t *testing.T) { + for _, test := range []struct { + name, oldOverride, newOverride string + noPin, podMode, busy, wantErr bool + wantReplace bool + }{ + {name: "added", newOverride: "gpu-a", wantReplace: true}, + {name: "changed", oldOverride: "gpu-a", newOverride: "gpu-b", wantReplace: true}, + {name: "removed", oldOverride: "gpu-a", wantReplace: true}, + {name: "unchanged", oldOverride: "gpu-a", newOverride: "gpu-a"}, + {name: "added override matches current hostname", newOverride: "worker-a"}, + {name: "removed override matches default hostname", oldOverride: "worker-a"}, + {name: "no override"}, + {name: "wait for initial pin", newOverride: "gpu-a", noPin: true}, + {name: "disabled preference ignores override", newOverride: "gpu-a", podMode: true}, + {name: "busy old identity is drained", oldOverride: "gpu-a", newOverride: "gpu-b", busy: true, wantReplace: true}, + {name: "invalid override preserves pod", oldOverride: "gpu-a", newOverride: "not.valid", wantErr: true}, + } { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + nodeset := newNodeSet("workers", "slurm", 1) + nodeset.UID = "workers-uid" + nodeset.Spec.PinToNode = true + nodeset.Spec.PreferKubernetesNodeName = ptr.To(true) + nodeset.Spec.UpdateStrategy.Type = slinkyv1beta1.OnDeleteNodeSetStrategyType + nodeset.Status.OrdinalToNode = map[string]string{"0": "worker-a"} + if test.podMode { + nodeset.Spec.PreferKubernetesNodeName = ptr.To(false) + } + if test.noPin { + nodeset.Status.OrdinalToNode = nil + } + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "worker-a", Annotations: map[string]string{ + slinkyv1beta1.AnnotationNodeHostnameOverride: test.oldOverride, + }}} + controller := &slinkyv1beta1.Controller{ObjectMeta: metav1.ObjectMeta{Name: "slurm", Namespace: nodeset.Namespace}} + kclient := fake.NewFakeClient(nodeset, node, controller) + pod := nodesetutils.NewNodeSetStatefulSetPod(kclient, nodeset, controller, 0, "") + pod.Spec.NodeName = node.Name + pod.Status.Phase = corev1.PodRunning + require.NoError(t, kclient.Create(ctx, pod)) + oldName := nodesetutils.GetSlurmNodeName(pod) + node.Annotations[slinkyv1beta1.AnnotationNodeHostnameOverride] = test.newOverride + require.NoError(t, kclient.Update(ctx, node)) + state := []slurmapi.V0044NodeState{slurmapi.V0044NodeStateIDLE, slurmapi.V0044NodeStateDRAIN} + if test.busy { + state = []slurmapi.V0044NodeState{slurmapi.V0044NodeStateALLOCATED} + } + sclient := newFakeClientList(sinterceptor.Funcs{}, &slurmtypes.V0044NodeList{Items: []slurmtypes.V0044Node{{ + V0044Node: slurmapi.V0044Node{Name: ptr.To(oldName), State: &state}, + }}}) + reconciler := newNodeSetController(kclient, newClientMap("slurm", sclient)) + mismatches, err := reconciler.getHostnameMismatches(ctx, nodeset, []*corev1.Pod{pod}) + if test.wantErr { + require.ErrorContains(t, err, "invalid hostname override") + require.Error(t, reconciler.syncNodeSetPods(ctx, nodeset, []*corev1.Pod{pod}, "")) + require.NoError(t, kclient.Get(ctx, client.ObjectKeyFromObject(pod), &corev1.Pod{})) + return + } + require.NoError(t, err) + require.Equal(t, test.wantReplace, len(mismatches) > 0) + if !test.wantReplace { + current := &corev1.Pod{} + require.NoError(t, kclient.Get(ctx, client.ObjectKeyFromObject(pod), current)) + require.Equal(t, oldName, nodesetutils.GetSlurmNodeName(current)) + return + } + require.NoError(t, reconciler.syncNodeSetPods(ctx, nodeset, []*corev1.Pod{pod}, "")) + current := &corev1.Pod{} + err = kclient.Get(ctx, client.ObjectKeyFromObject(pod), current) + if test.busy { + require.NoError(t, err) + require.Equal(t, "true", current.Annotations[slinkyv1beta1.AnnotationPodCordon]) + require.Equal(t, oldName, nodesetutils.GetSlurmNodeName(current)) + return + } + require.True(t, apierrors.IsNotFound(err)) + require.NoError(t, reconciler.syncNodeSetPods(ctx, nodeset, nil, "")) + require.NoError(t, kclient.Get(ctx, client.ObjectKeyFromObject(pod), current)) + want := test.newOverride + if want == "" { + want = "worker-a" + } + require.Equal(t, pod.Spec.Hostname, current.Spec.Hostname) + require.Equal(t, want, nodesetutils.GetSlurmNodeName(current)) + require.Equal(t, pod.Name, current.Name) + require.Equal(t, pod.Spec.Volumes, current.Spec.Volumes) + mismatches, err = reconciler.getHostnameMismatches(ctx, nodeset, []*corev1.Pod{current}) + require.NoError(t, err) + require.Empty(t, mismatches) + }) + } +} + +func TestStatefulSetHostnameOverrideRecords(t *testing.T) { + for _, test := range []struct { + name, recordName, override string + wantPruned bool + unpinned, oversubscribed bool + }{ + {name: "override restart", recordName: "gpu-a", override: "gpu-a"}, + {name: "default restart", recordName: "worker-a"}, + {name: "changed override", recordName: "gpu-a", override: "gpu-b", wantPruned: true}, + {name: "removed override", recordName: "gpu-a", wantPruned: true}, + {name: "added override", recordName: "worker-a", override: "gpu-a", wantPruned: true}, + {name: "unpinning removes old node record", recordName: "gpu-a", unpinned: true, wantPruned: true}, + {name: "unpinning retains fallback record", recordName: "workers-0", unpinned: true}, + {name: "oversubscription removes old node record", recordName: "gpu-a", oversubscribed: true, wantPruned: true}, + {name: "oversubscription retains fallback record", recordName: "workers-0", oversubscribed: true}, + } { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + nodeset := newNodeSet("workers", "slurm", 1) + nodeset.UID = "workers-uid" + nodeset.Spec.PinToNode = !test.unpinned + nodeset.Spec.OversubscribeNode = test.oversubscribed + nodeset.Spec.PreferKubernetesNodeName = ptr.To(true) + nodeset.Spec.PruneSlurmNodeRecords = slinkyv1beta1.NodeSetPruneNodeRecordTypeNodeNotFound + nodeset.Status.OrdinalToNode = map[string]string{"0": "worker-a"} + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "worker-a", Annotations: map[string]string{ + slinkyv1beta1.AnnotationNodeHostnameOverride: test.override, + }}} + info := podinfo.PodInfo{Namespace: nodeset.Namespace, PodName: "workers-0", Node: node.Name, NodeSetName: nodeset.Name, NodeSetUID: string(nodeset.UID)} + sclient := newFakeClientList(sinterceptor.Funcs{}, &slurmtypes.V0044NodeList{Items: []slurmtypes.V0044Node{{ + V0044Node: slurmapi.V0044Node{Name: ptr.To(test.recordName), Comment: ptr.To(info.ToString()), + State: ptr.To([]slurmapi.V0044NodeState{slurmapi.V0044NodeStateDOWN, slurmapi.V0044NodeStateNOTRESPONDING})}, + }}}) + reconciler := newNodeSetController(fake.NewFakeClient(nodeset, node), newClientMap("slurm", sclient)) + require.NoError(t, reconciler.syncSlurmNodeRecords(ctx, nodeset)) + require.NoError(t, reconciler.syncSlurmNodeRecords(ctx, nodeset)) + err := sclient.Get(ctx, slurmclient.ObjectKey(test.recordName), &slurmtypes.V0044Node{}) + if test.wantPruned { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestNodeNamedSlurmNodeRecordLifecycle(t *testing.T) { + for _, test := range []struct { + name string + podNode string + oldNodeExists bool + pinNoLongerMatches bool + podHostnameMode bool + foreign bool + never bool + wantPruned bool + pinLookupErr error + wantErr bool + }{ + {name: "restart retains valid pin", oldNodeExists: true}, + {name: "running pod retains identity", oldNodeExists: true, podNode: "worker-a"}, + {name: "running pod skips pin lookup", oldNodeExists: true, podNode: "worker-a", pinLookupErr: errors.New("pin lookup failed")}, + {name: "no defunct records skip pin lookup", oldNodeExists: true, foreign: true, pinLookupErr: errors.New("pin lookup failed")}, + {name: "missing pod propagates pin lookup failure", oldNodeExists: true, pinLookupErr: errors.New("pin lookup failed"), wantErr: true}, + {name: "deleted backing node", wantPruned: true}, + {name: "pin no longer matches template", oldNodeExists: true, pinNoLongerMatches: true, wantPruned: true}, + {name: "replacement pod changes identity", oldNodeExists: true, podNode: "worker-b", wantPruned: true}, + {name: "unpinned pod hostname mode preserves records", podHostnameMode: true}, + {name: "foreign record preserved", foreign: true}, + {name: "never preserves records", never: true}, + } { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + nodeset := newNodeSet("workers", "slurm", 1) + nodeset.UID = "nodeset-uid" + nodeset.Spec.PinToNode = true + nodeset.Spec.PreferKubernetesNodeName = ptr.To(true) + nodeset.Spec.PruneSlurmNodeRecords = slinkyv1beta1.NodeSetPruneNodeRecordTypeNodeNotFound + nodeset.Status.OrdinalToNode = map[string]string{"0": "worker-a"} + if test.never { + nodeset.Spec.PruneSlurmNodeRecords = slinkyv1beta1.NodeSetPruneNodeRecordTypeNever + } + if test.podHostnameMode { + nodeset.Spec.PreferKubernetesNodeName = ptr.To(false) + nodeset.Spec.PinToNode = false + } + if test.pinNoLongerMatches { + nodeset.Spec.Template.PodSpecWrapper.NodeSelector = map[string]string{"pool": "replacement"} + } + objects := []client.Object{nodeset} + if test.oldNodeExists { + objects = append(objects, &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "worker-a"}}) + } + if test.podNode != "" { + pod := newNodeSetPodWithStatus(nodeset, &slinkyv1beta1.Controller{ObjectMeta: metav1.ObjectMeta{Name: "slurm"}}, 0, corev1.PodRunning, nil) + pod.Spec.NodeName = test.podNode + pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = test.podNode + objects = append(objects, pod) + } + info := podinfo.PodInfo{Namespace: nodeset.Namespace, PodName: "workers-0", Node: "worker-a", NodeSetName: nodeset.Name, NodeSetUID: string(nodeset.UID)} + if test.foreign { + info.NodeSetUID = "other-uid" + } + slurmNode := slurmtypes.V0044Node{V0044Node: slurmapi.V0044Node{ + Name: ptr.To("worker-a"), Comment: ptr.To(info.ToString()), + State: ptr.To([]slurmapi.V0044NodeState{slurmapi.V0044NodeStateDOWN, slurmapi.V0044NodeStateNOTRESPONDING}), + }} + slurmClient := newFakeClientList(sinterceptor.Funcs{}, &slurmtypes.V0044NodeList{Items: []slurmtypes.V0044Node{slurmNode}}) + kubeClient := fake.NewClientBuilder().WithObjects(objects...).WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, kubeClient client.WithWatch, key client.ObjectKey, object client.Object, opts ...client.GetOption) error { + if _, isNode := object.(*corev1.Node); isNode && test.pinLookupErr != nil { + return test.pinLookupErr + } + return kubeClient.Get(ctx, key, object, opts...) + }, + }).Build() + reconciler := newNodeSetController(kubeClient, newClientMap("slurm", slurmClient)) + for range 2 { + err := reconciler.syncSlurmNodeRecords(ctx, nodeset) + if test.wantErr { + require.ErrorIs(t, err, test.pinLookupErr) + } else { + require.NoError(t, err) + } + } + err := slurmClient.Get(ctx, slurmclient.ObjectKey("worker-a"), &slurmtypes.V0044Node{}) + if test.wantPruned { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + func TestNodeSetReconciler_syncSlurmNodeRecords(t *testing.T) { controller := &slinkyv1beta1.Controller{ ObjectMeta: metav1.ObjectMeta{ @@ -5160,6 +5642,33 @@ func TestNodeSetReconciler_syncSlurmNodeRecords(t *testing.T) { return []runtime.Object{pod, kubeNode}, nodes, []string{existingSlurmName}, []string{"foo-ghost"} }, }, + { + name: "statefulset prunes stale identity but keeps current identity for the same pod name", + scalingMode: slinkyv1beta1.ScalingModeStatefulset, + pruneSlurmRecords: slinkyv1beta1.NodeSetPruneNodeRecordTypeNodeNotFound, + setup: func(ns *slinkyv1beta1.NodeSet) ([]runtime.Object, []slurmtypes.V0044Node, []string, []string) { + ns.Spec.PinToNode = true + ns.Spec.PreferKubernetesNodeName = ptr.To(true) + ns.Status.OrdinalToNode = map[string]string{"0": "worker-b"} + pod := newNodeSetPodWithStatus(ns, controller, 0, corev1.PodRunning, []corev1.PodConditionType{corev1.PodReady}) + pod.Spec.NodeName = "worker-b" + pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = "worker-b" + kubeNode := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "worker-b"}} + nodes := []slurmtypes.V0044Node{ + {V0044Node: slurmapi.V0044Node{ + Name: ptr.To("worker-a"), + State: defunctNodeState, + Comment: podInfo(ns, pod.Name, "worker-a"), + }}, + {V0044Node: slurmapi.V0044Node{ + Name: ptr.To("worker-b"), + State: defunctNodeState, + Comment: podInfo(ns, pod.Name, "worker-b"), + }}, + } + return []runtime.Object{pod, kubeNode}, nodes, []string{"worker-b"}, []string{"worker-a"} + }, + }, { name: "skips when kube node still maps to slurm node by default (no selectors)", scalingMode: slinkyv1beta1.ScalingModeDaemonset, @@ -5296,10 +5805,11 @@ func TestNodeSetReconciler_syncSlurmNodeRecords(t *testing.T) { }, }, { - name: "skips for statefulset scaling mode", + name: "skips when statefulset naming preference is disabled", scalingMode: "", pruneSlurmRecords: slinkyv1beta1.NodeSetPruneNodeRecordTypeNodeNotFound, setup: func(ns *slinkyv1beta1.NodeSet) ([]runtime.Object, []slurmtypes.V0044Node, []string, []string) { + ns.Spec.PreferKubernetesNodeName = ptr.To(false) defunctPodName := nodesetutils.GetOrdinalPodName(ns, 1) nodes := []slurmtypes.V0044Node{ {V0044Node: slurmapi.V0044Node{ diff --git a/internal/controller/nodeset/slurmcontrol/slurmcontrol.go b/internal/controller/nodeset/slurmcontrol/slurmcontrol.go index cc4b14dd..b67c7315 100644 --- a/internal/controller/nodeset/slurmcontrol/slurmcontrol.go +++ b/internal/controller/nodeset/slurmcontrol/slurmcontrol.go @@ -106,6 +106,28 @@ func (r *realSlurmControl) RefreshNodeCache(ctx context.Context, nodeset *slinky return nil } +func getSlurmNodeForPod(ctx context.Context, slurmClient slurmclient.Client, pod *corev1.Pod) (*slurmtypes.V0044Node, error) { + name := nodesetutils.GetSlurmNodeName(pod) + if name == "" { + return nil, slurmerrors.ErrNotFound + } + node := &slurmtypes.V0044Node{} + if err := slurmClient.Get(ctx, slurmobject.ObjectKey(name), node); err != nil { + return nil, err + } + return node, nil +} + +func slurmNodeNamesForPods(pods []*corev1.Pod) set.Set[string] { + names := set.New[string]() + for _, pod := range pods { + if name := nodesetutils.GetSlurmNodeName(pod); name != "" { + names.Insert(name) + } + } + return names +} + // UpdateNodeWithPodInfo implements SlurmControlInterface. func (r *realSlurmControl) UpdateNodeWithPodInfo(ctx context.Context, nodeset *slinkyv1beta1.NodeSet, pod *corev1.Pod) error { logger := log.FromContext(ctx) @@ -117,9 +139,8 @@ func (r *realSlurmControl) UpdateNodeWithPodInfo(ctx context.Context, nodeset *s return ErrNoSlurmClient } - slurmNode := &slurmtypes.V0044Node{} - key := slurmobject.ObjectKey(nodesetutils.GetSlurmNodeName(pod)) - if err := slurmClient.Get(ctx, key, slurmNode); err != nil { + slurmNode, err := getSlurmNodeForPod(ctx, slurmClient, pod) + if err != nil { if errors.Is(err, slurmerrors.ErrNotFound) { return nil } @@ -179,9 +200,8 @@ func (r *realSlurmControl) UpdateNodeTopology(ctx context.Context, nodeset *slin return ErrNoSlurmClient } - slurmNode := &slurmtypes.V0044Node{} - key := slurmobject.ObjectKey(nodesetutils.GetSlurmNodeName(pod)) - if err := slurmClient.Get(ctx, key, slurmNode); err != nil { + slurmNode, err := getSlurmNodeForPod(ctx, slurmClient, pod) + if err != nil { if errors.Is(err, slurmerrors.ErrNotFound) { return nil } @@ -220,9 +240,8 @@ func (r *realSlurmControl) UpdateNodeFeatures(ctx context.Context, nodeset *slin return ErrNoSlurmClient } - slurmNode := &slurmtypes.V0044Node{} - key := slurmobject.ObjectKey(nodesetutils.GetSlurmNodeName(pod)) - if err := slurmClient.Get(ctx, key, slurmNode); err != nil { + slurmNode, err := getSlurmNodeForPod(ctx, slurmClient, pod) + if err != nil { if errors.Is(err, slurmerrors.ErrNotFound) { return nil } @@ -294,9 +313,8 @@ func (r *realSlurmControl) MakeNodeDrain(ctx context.Context, nodeset *slinkyv1b return ErrNoSlurmClient } - slurmNode := &slurmtypes.V0044Node{} - key := slurmobject.ObjectKey(nodesetutils.GetSlurmNodeName(pod)) - if err := slurmClient.Get(ctx, key, slurmNode); err != nil { + slurmNode, err := getSlurmNodeForPod(ctx, slurmClient, pod) + if err != nil { if errors.Is(err, slurmerrors.ErrNotFound) { return nil } @@ -347,9 +365,8 @@ func (r *realSlurmControl) MakeNodeUndrain(ctx context.Context, nodeset *slinkyv return ErrNoSlurmClient } - slurmNode := &slurmtypes.V0044Node{} - key := slurmobject.ObjectKey(nodesetutils.GetSlurmNodeName(pod)) - if err := slurmClient.Get(ctx, key, slurmNode); err != nil { + slurmNode, err := getSlurmNodeForPod(ctx, slurmClient, pod) + if err != nil { if errors.Is(err, slurmerrors.ErrNotFound) { return nil } @@ -396,9 +413,8 @@ func (r *realSlurmControl) IsNodeDrain(ctx context.Context, nodeset *slinkyv1bet return true, ErrNoSlurmClient } - slurmNode := &slurmtypes.V0044Node{} - key := slurmobject.ObjectKey(nodesetutils.GetSlurmNodeName(pod)) - if err := slurmClient.Get(ctx, key, slurmNode); err != nil { + slurmNode, err := getSlurmNodeForPod(ctx, slurmClient, pod) + if err != nil { if errors.Is(err, slurmerrors.ErrNotFound) { return true, nil } @@ -420,9 +436,8 @@ func (r *realSlurmControl) IsNodeDrained(ctx context.Context, nodeset *slinkyv1b return true, ErrNoSlurmClient } - slurmNode := &slurmtypes.V0044Node{} - key := slurmobject.ObjectKey(nodesetutils.GetSlurmNodeName(pod)) - if err := slurmClient.Get(ctx, key, slurmNode); err != nil { + slurmNode, err := getSlurmNodeForPod(ctx, slurmClient, pod) + if err != nil { if errors.Is(err, slurmerrors.ErrNotFound) { return true, nil } @@ -449,9 +464,8 @@ func (r *realSlurmControl) IsNodeDownForUnresponsive(ctx context.Context, nodese return true, ErrNoSlurmClient } - slurmNode := &slurmtypes.V0044Node{} - key := slurmobject.ObjectKey(nodesetutils.GetSlurmNodeName(pod)) - if err := slurmClient.Get(ctx, key, slurmNode); err != nil { + slurmNode, err := getSlurmNodeForPod(ctx, slurmClient, pod) + if err != nil { if errors.Is(err, slurmerrors.ErrNotFound) { return true, nil } @@ -478,9 +492,8 @@ func (r *realSlurmControl) IsNodeReasonOurs(ctx context.Context, nodeset *slinky return true, ErrNoSlurmClient } - slurmNode := &slurmtypes.V0044Node{} - key := slurmobject.ObjectKey(nodesetutils.GetSlurmNodeName(pod)) - if err := slurmClient.Get(ctx, key, slurmNode); err != nil { + slurmNode, err := getSlurmNodeForPod(ctx, slurmClient, pod) + if err != nil { if errors.Is(err, slurmerrors.ErrNotFound) { return true, nil } @@ -529,12 +542,16 @@ func (r *realSlurmControl) CalculateNodeStatus(ctx context.Context, nodeset *sli status := SlurmNodeStatus{ NodeStates: make(map[string][]corev1.PodCondition), } + podNodeNameSet := slurmNodeNamesForPods(pods) slurmClient := r.lookupClient(nodeset) if slurmClient == nil { logger.V(2).Info("no client for nodeset, cannot do CalculateNodeStatus()") return status, ErrNoSlurmClient } + if len(podNodeNameSet) == 0 { + return status, nil + } nodeList := &slurmtypes.V0044NodeList{} if err := slurmClient.List(ctx, nodeList); err != nil { @@ -544,12 +561,6 @@ func (r *realSlurmControl) CalculateNodeStatus(ctx context.Context, nodeset *sli return status, err } - podNodeNameSet := set.New[string]() - for _, pod := range pods { - podNodeName := nodesetutils.GetSlurmNodeName(pod) - podNodeNameSet.Insert(podNodeName) - } - for _, node := range nodeList.Items { nodeName := ptr.Deref(node.Name, "") if !podNodeNameSet.Has(nodeName) { @@ -639,17 +650,15 @@ const infiniteDuration = time.Duration(math.MaxInt64) func (r *realSlurmControl) GetNodeDeadlines(ctx context.Context, nodeset *slinkyv1beta1.NodeSet, pods []*corev1.Pod) (*timestore.TimeStore, error) { logger := log.FromContext(ctx) ts := timestore.NewTimeStore(timestore.Greater) + slurmNodeNamesSet := slurmNodeNamesForPods(pods) slurmClient := r.lookupClient(nodeset) if slurmClient == nil { logger.V(2).Info("no client for nodeset, cannot do GetNodeDeadlines()") return ts, ErrNoSlurmClient } - - slurmNodeNamesSet := set.New[string]() - for _, pod := range pods { - slurmNodeName := nodesetutils.GetSlurmNodeName(pod) - slurmNodeNamesSet.Insert(slurmNodeName) + if len(slurmNodeNamesSet) == 0 { + return ts, nil } jobList := &slurmtypes.V0044JobInfoList{} @@ -693,25 +702,22 @@ func (r *realSlurmControl) GetNodeDeadlines(ctx context.Context, nodeset *slinky // GetNodesForPods implements SlurmControlInterface. func (r *realSlurmControl) GetNodesForPods(ctx context.Context, nodeset *slinkyv1beta1.NodeSet, pods []*corev1.Pod) ([]string, error) { logger := log.FromContext(ctx) + podNodeNameSet := slurmNodeNamesForPods(pods) slurmClient := r.lookupClient(nodeset) if slurmClient == nil { logger.V(2).Info("no client for nodeset, cannot do GetNodesForPods()") return nil, ErrNoSlurmClient } + if len(podNodeNameSet) == 0 { + return []string{}, nil + } nodeList := &slurmtypes.V0044NodeList{} if err := slurmClient.List(ctx, nodeList); err != nil { return nil, err } - // Expected Slurm nodes backed by NodeSet pods - podNodeNameSet := set.New[string]() - for _, pod := range pods { - podNodeName := nodesetutils.GetSlurmNodeName(pod) - podNodeNameSet.Insert(podNodeName) - } - // Actual Slurm nodes given NodeSet pods slurmNodeNames := []string{} for _, node := range nodeList.Items { @@ -854,6 +860,9 @@ func (r *realSlurmControl) GetPodsUnderReservation(ctx context.Context, nodeset logger.V(2).Info("no client for nodeset, cannot do GetPodsUnderReservation()") return nil, ErrNoSlurmClient } + if len(slurmNodeNamesForPods(pods)) == 0 { + return nil, nil + } reservation := new(slurmtypes.V0044ReservationInfo) key := slurmobject.ObjectKey("SlurmOperatorMaint-" + nodeset.Name) @@ -866,11 +875,11 @@ func (r *realSlurmControl) GetPodsUnderReservation(ctx context.Context, nodeset // For each pod, determine if the associated Slurm node is actively under the NodeSet's reservation for _, pod := range pods { - nodename := nodesetutils.GetSlurmNodeName(pod) - - slurmNode := new(slurmtypes.V0044Node) - key := slurmobject.ObjectKey(nodename) - if err := slurmClient.Get(ctx, key, slurmNode); err != nil && !errors.Is(err, slurmerrors.ErrNotFound) { + slurmNode, err := getSlurmNodeForPod(ctx, slurmClient, pod) + if errors.Is(err, slurmerrors.ErrNotFound) { + continue + } + if err != nil { return nil, err } if slurmNode.State != nil && slurmNode.Reservation != nil { @@ -919,6 +928,9 @@ func (r *realSlurmControl) SyncReservationForNodeSet(ctx context.Context, nodese logger.V(2).Info("no client for nodeset, cannot do SyncReservationForNodeSet()") return ErrNoSlurmClient } + if len(slurmNodeNamesForPods(pods)) == 0 { + return nil + } name := "SlurmOperatorMaint-" + nodeset.Name @@ -934,6 +946,9 @@ func (r *realSlurmControl) SyncReservationForNodeSet(ctx context.Context, nodese } return err } + if len(slurmNodes) == 0 { + return nil + } slurmNodeHostList, err := hostlist.Compress(slurmNodes) if err != nil { return err diff --git a/internal/controller/nodeset/slurmcontrol/slurmcontrol_test.go b/internal/controller/nodeset/slurmcontrol/slurmcontrol_test.go index 789f0339..2e244113 100644 --- a/internal/controller/nodeset/slurmcontrol/slurmcontrol_test.go +++ b/internal/controller/nodeset/slurmcontrol/slurmcontrol_test.go @@ -86,6 +86,152 @@ func newNodeSet(name, controllerName string, replicas int32) *slinkyv1beta1.Node } } +func TestUnresolvedPodSlurmIdentity(t *testing.T) { + ctx := context.Background() + nodeset := newNodeSet("workers", "slurm", 1) + nodeset.Spec.PinToNode = true + nodeset.Spec.PreferKubernetesNodeName = ptr.To(true) + pod := nodesetutils.NewNodeSetStatefulSetPod(kubefake.NewFakeClient(), nodeset, &slinkyv1beta1.Controller{}, 0, "") + require.Empty(t, pod.Spec.NodeName) + require.Empty(t, nodesetutils.GetSlurmNodeName(pod)) + + for _, test := range []struct { + name string + run func(*testing.T, SlurmControlInterface) + }{ + {name: "pod info", run: func(t *testing.T, control SlurmControlInterface) { + require.NoError(t, control.UpdateNodeWithPodInfo(ctx, nodeset, pod)) + }}, + {name: "topology", run: func(t *testing.T, control SlurmControlInterface) { + require.NoError(t, control.UpdateNodeTopology(ctx, nodeset, pod, "rack-a")) + }}, + {name: "features", run: func(t *testing.T, control SlurmControlInterface) { + require.NoError(t, control.UpdateNodeFeatures(ctx, nodeset, pod, "node-", []string{"gpu"})) + }}, + {name: "drain", run: func(t *testing.T, control SlurmControlInterface) { + require.NoError(t, control.MakeNodeDrain(ctx, nodeset, pod, "scale down", true)) + }}, + {name: "undrain", run: func(t *testing.T, control SlurmControlInterface) { + require.NoError(t, control.MakeNodeUndrain(ctx, nodeset, pod, "")) + }}, + {name: "is drain", run: func(t *testing.T, control SlurmControlInterface) { + value, err := control.IsNodeDrain(ctx, nodeset, pod) + require.NoError(t, err) + require.True(t, value) + }}, + {name: "is drained permits cancellation", run: func(t *testing.T, control SlurmControlInterface) { + value, err := control.IsNodeDrained(ctx, nodeset, pod) + require.NoError(t, err) + require.True(t, value) + }}, + {name: "is unresponsive", run: func(t *testing.T, control SlurmControlInterface) { + value, err := control.IsNodeDownForUnresponsive(ctx, nodeset, pod) + require.NoError(t, err) + require.True(t, value) + }}, + {name: "reason ownership", run: func(t *testing.T, control SlurmControlInterface) { + value, err := control.IsNodeReasonOurs(ctx, nodeset, pod) + require.NoError(t, err) + require.True(t, value) + }}, + {name: "status", run: func(t *testing.T, control SlurmControlInterface) { + status, err := control.CalculateNodeStatus(ctx, nodeset, []*corev1.Pod{pod}) + require.NoError(t, err) + require.Zero(t, status.Total) + require.Empty(t, status.NodeStates) + }}, + {name: "deadlines", run: func(t *testing.T, control SlurmControlInterface) { + deadlines, err := control.GetNodeDeadlines(ctx, nodeset, []*corev1.Pod{pod}) + require.NoError(t, err) + require.True(t, deadlines.Peek("").IsZero()) + }}, + {name: "registered nodes", run: func(t *testing.T, control SlurmControlInterface) { + names, err := control.GetNodesForPods(ctx, nodeset, []*corev1.Pod{pod}) + require.NoError(t, err) + require.Empty(t, names) + }}, + {name: "reservation membership", run: func(t *testing.T, control SlurmControlInterface) { + pods, err := control.GetPodsUnderReservation(ctx, nodeset, []*corev1.Pod{pod}) + require.NoError(t, err) + require.Empty(t, pods) + }}, + {name: "reservation creation", run: func(t *testing.T, control SlurmControlInterface) { + require.NoError(t, control.SyncReservationForNodeSet(ctx, nodeset, []*corev1.Pod{pod})) + }}, + } { + t.Run(test.name, func(t *testing.T) { + lookups := 0 + slurmClient := fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{ + Get: func(context.Context, object.ObjectKey, object.Object, ...client.GetOption) error { + lookups++ + return errors.New("unexpected lookup for unresolved Slurm identity") + }, + List: func(context.Context, object.ObjectList, ...client.ListOption) error { + lookups++ + return errors.New("unexpected list for unresolved Slurm identity") + }, + }).Build() + control := NewSlurmControl(testutils.NewClientMap("slurm", nodeset.Namespace, slurmClient)) + test.run(t, control) + require.Zero(t, lookups) + }) + } +} + +func TestSlurmOperationsWithPendingAndResolvedPods(t *testing.T) { + ctx := context.Background() + nodeset := newNodeSet("workers", "slurm", 3) + nodeset.UID = "workers-uid" + nodeset.Spec.PinToNode = true + nodeset.Spec.PreferKubernetesNodeName = ptr.To(true) + pending := nodesetutils.NewNodeSetStatefulSetPod(kubefake.NewFakeClient(), nodeset, &slinkyv1beta1.Controller{}, 0, "") + registered := pending.DeepCopy() + registered.Name = "workers-1" + registered.Spec.NodeName = "worker-a" + registered.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = "gpu-a" + unregistered := pending.DeepCopy() + unregistered.Name = "workers-2" + unregistered.Spec.NodeName = "worker-b" + unregistered.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = "gpu-b" + pods := []*corev1.Pod{pending, registered, unregistered} + reservation := &types.V0044ReservationInfo{V0044ReservationInfo: api.V0044ReservationInfo{ + Name: ptr.To("SlurmOperatorMaint-workers"), NodeList: ptr.To("gpu-a"), + }} + node := &types.V0044Node{V0044Node: api.V0044Node{ + Name: ptr.To("gpu-a"), State: ptr.To([]api.V0044NodeState{api.V0044NodeStateIDLE, api.V0044NodeStateMAINTENANCE}), + Reservation: reservation.Name, + }} + baseClient := fake.NewClientBuilder().WithUpdateFn(slurmUpdateFn).WithObjects(node, reservation).Build() + slurmClient := interceptor.NewClient(baseClient, interceptor.Funcs{ + Get: func(ctx context.Context, key object.ObjectKey, obj object.Object, opts ...client.GetOption) error { + require.NotEmpty(t, key, "pending workers must not generate empty Slurm lookup keys") + return baseClient.Get(ctx, key, obj, opts...) + }, + }) + control := NewSlurmControl(testutils.NewClientMap("slurm", nodeset.Namespace, slurmClient)) + + status, err := control.CalculateNodeStatus(ctx, nodeset, pods) + require.NoError(t, err) + require.Equal(t, int32(1), status.Total) + require.Equal(t, int32(1), status.Idle) + require.Contains(t, status.NodeStates, "gpu-a") + require.NotContains(t, status.NodeStates, "") + names, err := control.GetNodesForPods(ctx, nodeset, pods) + require.NoError(t, err) + require.Equal(t, []string{"gpu-a"}, names) + reserved, err := control.GetPodsUnderReservation(ctx, nodeset, pods) + require.NoError(t, err) + require.Equal(t, []*corev1.Pod{registered}, reserved) + for _, pod := range pods { + require.NoError(t, control.UpdateNodeWithPodInfo(ctx, nodeset, pod)) + } + require.NoError(t, baseClient.Get(ctx, object.ObjectKey("gpu-a"), node)) + info := &podinfo.PodInfo{} + require.NoError(t, podinfo.ParseIntoPodInfo(node.Comment, info)) + require.Equal(t, registered.Name, info.PodName) + require.Equal(t, registered.Spec.NodeName, info.Node) +} + func Test_realSlurmControl_UpdateNodeWithPodInfo(t *testing.T) { ctx := context.Background() controller := &slinkyv1beta1.Controller{ diff --git a/internal/controller/nodeset/utils/utils.go b/internal/controller/nodeset/utils/utils.go index 0fb3b09b..8605eecb 100644 --- a/internal/controller/nodeset/utils/utils.go +++ b/internal/controller/nodeset/utils/utils.go @@ -60,7 +60,11 @@ func NewNodeSetStatefulSetPod( // Ensure recreated pods are pinned to their node, but only if they still match their Node. if nodeset.Spec.PinToNode { - pinPodToNode(client, nodeset.Status.OrdinalToNode, pod, ordinal) + if node := pinPodToNode(client, nodeset.Status.OrdinalToNode, pod, ordinal); node != nil { + if nodeset.Spec.EffectiveSlurmNodeNameMode() == slinkyv1beta1.SlurmNodeNameModeKubernetesNode { + pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = GetDaemonSetPodHostname(node.Name, node.Annotations[slinkyv1beta1.AnnotationNodeHostnameOverride]) + } + } } // WARNING: Do not use the spec.NodeName otherwise the Pod scheduler will @@ -71,23 +75,24 @@ func NewNodeSetStatefulSetPod( } // pinPodToNode will modify the input Pod with its Node affinity if the pin is valid -func pinPodToNode(kclient client.Client, ordinalToNode map[string]string, pod *corev1.Pod, ordinal int) { +func pinPodToNode(kclient client.Client, ordinalToNode map[string]string, pod *corev1.Pod, ordinal int) *corev1.Node { nodeName, ok := ordinalToNode[strconv.Itoa(ordinal)] if !ok { - return + return nil } ctx := context.TODO() node := &corev1.Node{} nodeKey := types.NamespacedName{Name: nodeName} if err := kclient.Get(ctx, nodeKey, node); err != nil { - return + return nil } if shouldRun, _ := PodShouldRunOnNode(ctx, pod, node); !shouldRun { - return + return nil } pod.Spec.Affinity = daemonutils.ReplaceDaemonSetPodNodeNameNodeAffinity(pod.Spec.Affinity, nodeName) + return node } func NewNodeSetDaemonSetPod( @@ -175,8 +180,14 @@ func initIdentity(nodeset *slinkyv1beta1.NodeSet, pod *corev1.Pod) { pod.Spec.Hostname = pod.Name } } - pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = GetSlurmNodeName(pod) pod.Labels[slinkyv1beta1.LabelNodeSetScalingMode] = string(nodeset.Spec.ScalingMode) + if nodeset.Spec.ScalingMode != slinkyv1beta1.ScalingModeDaemonset && nodeset.Spec.EffectiveSlurmNodeNameMode() == slinkyv1beta1.SlurmNodeNameModeKubernetesNode { + pod.Labels[slinkyv1beta1.LabelNodeSetSlurmNodeNameMode] = string(slinkyv1beta1.SlurmNodeNameModeKubernetesNode) + pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = "" + } else { + pod.Labels[slinkyv1beta1.LabelNodeSetSlurmNodeNameMode] = string(slinkyv1beta1.SlurmNodeNameModePodHostname) + pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = GetSlurmNodeName(pod) + } } // UpdateIdentity updates pod's labels. @@ -330,12 +341,13 @@ func GetOrdinalPodName(nodeset *slinkyv1beta1.NodeSet, ordinal int) string { return fmt.Sprintf("%s-%s", nodeset.Name, paddedOrdinal) } -// GetSlurmNodeName returns the Slurm node name. +// GetSlurmNodeName returns the Pod's resolved Slurm identity, or "" if unresolved. +// A resolved identity does not imply that the Pod is scheduled or registered in Slurm. func GetSlurmNodeName(pod *corev1.Pod) string { + if pod.Labels[slinkyv1beta1.LabelNodeSetSlurmNodeNameMode] == string(slinkyv1beta1.SlurmNodeNameModeKubernetesNode) { + return pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] + } if pod.Labels[slinkyv1beta1.LabelNodeSetScalingMode] == string(slinkyv1beta1.ScalingModeStatefulset) { - if pod.Spec.HostNetwork { - return pod.Spec.NodeName - } if pod.Spec.Hostname != "" { return pod.Spec.Hostname } diff --git a/internal/controller/nodeset/utils/utils_test.go b/internal/controller/nodeset/utils/utils_test.go index 8be9226c..7582b936 100644 --- a/internal/controller/nodeset/utils/utils_test.go +++ b/internal/controller/nodeset/utils/utils_test.go @@ -424,6 +424,20 @@ func TestGetSlurmNodeName(t *testing.T) { }, want: "bar-1", }, + { + name: "configured hostname takes precedence over Pod name", + args: args{ + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "workers-0", Labels: map[string]string{ + slinkyv1beta1.LabelNodeSetScalingMode: string(slinkyv1beta1.ScalingModeStatefulset), + }}, + Spec: corev1.PodSpec{ + Hostname: "compute-0", + }, + }, + }, + want: "compute-0", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -433,6 +447,149 @@ func TestGetSlurmNodeName(t *testing.T) { } } +func TestStatefulSetHostnameOverride(t *testing.T) { + for _, test := range []struct { + name string + disabled bool + noPin, missingNode, mismatch, oversubscribe bool + wantOverride bool + }{ + {name: "preference enabled", wantOverride: true}, + {name: "default preserves pod hostname", disabled: true}, + {name: "no pin", noPin: true}, + {name: "deleted node", missingNode: true}, + {name: "node no longer matches", mismatch: true}, + {name: "oversubscription", oversubscribe: true}, + } { + t.Run(test.name, func(t *testing.T) { + nodeset := newNodeSet("workers") + nodeset.Spec.PreferKubernetesNodeName = ptr.To(!test.disabled) + nodeset.Spec.PinToNode = !test.noPin + nodeset.Spec.OversubscribeNode = test.oversubscribe + nodeset.Spec.Template.PodSpecWrapper.Hostname = "compute-" + nodeset.Status.OrdinalToNode = map[string]string{"3": "worker-a"} + if test.mismatch { + nodeset.Spec.Template.PodSpecWrapper.NodeSelector = map[string]string{"pool": "other"} + } + kclient := fake.NewFakeClient() + if !test.missingNode { + require.NoError(t, kclient.Create(context.Background(), &corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: "worker-a", Annotations: map[string]string{slinkyv1beta1.AnnotationNodeHostnameOverride: "gpu-01"}, + }})) + } + controller := &slinkyv1beta1.Controller{ObjectMeta: metav1.ObjectMeta{Name: "slurm"}} + pod := NewNodeSetStatefulSetPod(kclient, nodeset, controller, 3, "") + want := "compute-3" + if test.wantOverride { + want = "gpu-01" + require.Equal(t, want, pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname]) + require.Equal(t, want, GetSlurmNodeName(pod)) + } else if nodeset.Spec.EffectiveSlurmNodeNameMode() == slinkyv1beta1.SlurmNodeNameModePodHostname { + require.Equal(t, want, pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname]) + require.Equal(t, want, GetSlurmNodeName(pod)) + } else { + require.Empty(t, GetSlurmNodeName(pod)) + } + require.Equal(t, "compute-3", pod.Spec.Hostname) + require.Equal(t, "workers-3", pod.Name) + require.Empty(t, pod.Spec.NodeName) + require.Equal(t, "datadir-workers-3", GetPersistentVolumeClaims(nodeset, pod)["datadir"].Name) + require.Equal(t, pod.Spec, NewNodeSetStatefulSetPod(kclient, nodeset, controller, 3, "").Spec) + }) + } +} + +func TestStatefulSetNodeNamingMatchesDaemonSet(t *testing.T) { + for _, override := range []string{"", "gpu-01"} { + t.Run(override, func(t *testing.T) { + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "worker-a.example.com", Annotations: map[string]string{ + slinkyv1beta1.AnnotationNodeHostnameOverride: override, + }}} + kclient := fake.NewFakeClient(node) + nodeset := newNodeSet("workers") + nodeset.Spec.PinToNode = true + nodeset.Spec.PreferKubernetesNodeName = ptr.To(true) + controller := &slinkyv1beta1.Controller{ObjectMeta: metav1.ObjectMeta{Name: "slurm"}} + initialPod := NewNodeSetStatefulSetPod(kclient, nodeset, controller, 0, "") + require.Equal(t, "workers-0", initialPod.Spec.Hostname) + require.Empty(t, GetSlurmNodeName(initialPod)) + nodeset.Status.OrdinalToNode = map[string]string{"0": node.Name} + statefulPod := NewNodeSetStatefulSetPod(kclient, nodeset, controller, 0, "") + nodeset.Spec.ScalingMode = slinkyv1beta1.ScalingModeDaemonset + daemonPod := NewNodeSetDaemonSetPod(kclient, nodeset, controller, node.Name, override, "") + want := "worker-a" + if override != "" { + want = override + } + require.Equal(t, want, GetSlurmNodeName(statefulPod)) + require.Equal(t, GetSlurmNodeName(daemonPod), GetSlurmNodeName(statefulPod)) + require.Equal(t, want, daemonPod.Spec.Hostname) + require.Equal(t, initialPod.Spec.Hostname, statefulPod.Spec.Hostname) + require.Equal(t, initialPod.Name, statefulPod.Name) + require.Equal(t, initialPod.Spec.Volumes, statefulPod.Spec.Volumes) + }) + } +} + +func TestSlurmNodeNameModeIdentityPreserved(t *testing.T) { + nodeset := &slinkyv1beta1.NodeSet{ + ObjectMeta: metav1.ObjectMeta{Name: "workers", Namespace: "slurm"}, + Spec: slinkyv1beta1.NodeSetSpec{ + ScalingMode: slinkyv1beta1.ScalingModeStatefulset, + PreferKubernetesNodeName: ptr.To(true), + PinToNode: true, + }, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "workers-0", Namespace: "slurm", Labels: map[string]string{ + slinkyv1beta1.LabelNodeSetPodName: "workers-0", + slinkyv1beta1.LabelNodeSetSlurmNodeNameMode: string(slinkyv1beta1.SlurmNodeNameModeKubernetesNode), + slinkyv1beta1.LabelNodeSetPodHostname: "gpu-01", + }}, + Spec: corev1.PodSpec{NodeName: "kube-node"}, + } + require.True(t, IsIdentityMatch(nodeset, pod)) + UpdateIdentity(nodeset, pod) + require.True(t, IsIdentityMatch(nodeset, pod)) + require.Equal(t, "gpu-01", GetSlurmNodeName(pod)) + nodeset.Spec.PinToNode = false + UpdateIdentity(nodeset, pod) + require.True(t, IsIdentityMatch(nodeset, pod)) + require.Equal(t, "gpu-01", pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname]) +} + +func TestSlurmNodeNameModeIdentity(t *testing.T) { + for name, prefer := range map[string]bool{"disabled": false, "enabled": true} { + t.Run(name, func(t *testing.T) { + nodeset := newNodeSet("workers") + nodeset.Spec.PreferKubernetesNodeName = ptr.To(prefer) + nodeset.Spec.PinToNode = true + nodeset.Spec.Template.Metadata.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = "inherited-name" + controller := &slinkyv1beta1.Controller{ObjectMeta: metav1.ObjectMeta{Name: "slurm"}} + pod := NewNodeSetStatefulSetPod(fake.NewFakeClient(), nodeset, controller, 3, "") + if prefer { + require.Equal(t, string(slinkyv1beta1.SlurmNodeNameModeKubernetesNode), pod.Labels[slinkyv1beta1.LabelNodeSetSlurmNodeNameMode]) + require.Empty(t, GetSlurmNodeName(pod)) + } else { + require.Equal(t, string(slinkyv1beta1.SlurmNodeNameModePodHostname), pod.Labels[slinkyv1beta1.LabelNodeSetSlurmNodeNameMode]) + require.Equal(t, "workers-3", GetSlurmNodeName(pod)) + } + pod.Spec.NodeName = "worker-a.example.com" + want := "workers-3" + if prefer { + require.Empty(t, GetSlurmNodeName(pod)) + want = "worker-a" + pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = want + } + require.Equal(t, want, GetSlurmNodeName(pod)) + require.Equal(t, "workers-3", pod.Name) + UpdateIdentity(nodeset, pod) + require.Equal(t, want, GetSlurmNodeName(pod)) + require.Equal(t, want, pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname]) + }) + } +} + func TestIsIdentityMatch(t *testing.T) { controller := &slinkyv1beta1.Controller{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/defaults/nodeset.go b/internal/defaults/nodeset.go index 2a6cbedc..94b89a18 100644 --- a/internal/defaults/nodeset.go +++ b/internal/defaults/nodeset.go @@ -13,6 +13,7 @@ import ( // Default values for NodeSet Spec fields when unspecified. const ( DefaultNodeSetReplicas int32 = 1 + DefaultNodeSetPreferKubernetesNodeName bool = true DefaultNodeSetWorkloadDisruptionProtection bool = true DefaultNodeSetScalingMode slinkyv1beta1.ScalingModeType = slinkyv1beta1.ScalingModeStatefulset DefaultNodeSetUpdateStrategyType slinkyv1beta1.NodeSetUpdateStrategyType = slinkyv1beta1.RollingUpdateNodeSetStrategyType @@ -38,6 +39,10 @@ func SetNodeSetDefaults(nodeset *slinkyv1beta1.NodeSet) { s.ScalingMode = slinkyv1beta1.ScalingModeStatefulset } + if s.PreferKubernetesNodeName == nil { + s.PreferKubernetesNodeName = ptr.To(DefaultNodeSetPreferKubernetesNodeName) + } + if s.WorkloadDisruptionProtection == nil { s.WorkloadDisruptionProtection = ptr.To(DefaultNodeSetWorkloadDisruptionProtection) } diff --git a/internal/defaults/nodeset_test.go b/internal/defaults/nodeset_test.go index 4c5bf2bc..ec3d4080 100644 --- a/internal/defaults/nodeset_test.go +++ b/internal/defaults/nodeset_test.go @@ -4,6 +4,7 @@ package defaults import ( + "encoding/json" "testing" "github.com/stretchr/testify/require" @@ -13,6 +14,58 @@ import ( slinkyv1beta1 "github.com/SlinkyProject/slurm-operator/api/v1beta1" ) +func TestEffectiveSlurmNodeNameMode(t *testing.T) { + for _, test := range []struct { + name string + prefer *bool + scaling slinkyv1beta1.ScalingModeType + pinned, oversubscribe bool + want slinkyv1beta1.SlurmNodeNameModeType + }{ + {name: "preferred pinned", prefer: ptr.To(true), pinned: true, want: slinkyv1beta1.SlurmNodeNameModeKubernetesNode}, + {name: "default pinned uses node naming", pinned: true, want: slinkyv1beta1.SlurmNodeNameModeKubernetesNode}, + {name: "explicit false preserves pod naming", prefer: ptr.To(false), pinned: true, want: slinkyv1beta1.SlurmNodeNameModePodHostname}, + {name: "default unpinned uses pod naming", want: slinkyv1beta1.SlurmNodeNameModePodHostname}, + {name: "preferred unpinned falls back", prefer: ptr.To(true), want: slinkyv1beta1.SlurmNodeNameModePodHostname}, + {name: "default oversubscribed falls back", pinned: true, oversubscribe: true, want: slinkyv1beta1.SlurmNodeNameModePodHostname}, + {name: "preferred oversubscribed falls back", prefer: ptr.To(true), pinned: true, oversubscribe: true, want: slinkyv1beta1.SlurmNodeNameModePodHostname}, + {name: "daemon default unchanged", scaling: slinkyv1beta1.ScalingModeDaemonset, want: slinkyv1beta1.SlurmNodeNameModeKubernetesNode}, + {name: "daemon preferred unchanged", prefer: ptr.To(true), scaling: slinkyv1beta1.ScalingModeDaemonset, want: slinkyv1beta1.SlurmNodeNameModeKubernetesNode}, + {name: "daemon explicit false unchanged", prefer: ptr.To(false), scaling: slinkyv1beta1.ScalingModeDaemonset, want: slinkyv1beta1.SlurmNodeNameModeKubernetesNode}, + } { + t.Run(test.name, func(t *testing.T) { + spec := slinkyv1beta1.NodeSetSpec{ScalingMode: test.scaling, PreferKubernetesNodeName: test.prefer, PinToNode: test.pinned, OversubscribeNode: test.oversubscribe} + require.Equal(t, test.want, spec.EffectiveSlurmNodeNameMode()) + }) + } +} + +func TestPreferKubernetesNodeNameDefaulting(t *testing.T) { + for _, test := range []struct { + name, input string + want bool + }{ + {name: "omitted", input: `{"spec":{}}`, want: true}, + {name: "null", input: `{"spec":{"preferKubernetesNodeName":null}}`, want: true}, + {name: "true", input: `{"spec":{"preferKubernetesNodeName":true}}`, want: true}, + {name: "false", input: `{"spec":{"preferKubernetesNodeName":false}}`}, + } { + t.Run(test.name, func(t *testing.T) { + nodeset := &slinkyv1beta1.NodeSet{} + require.NoError(t, json.Unmarshal([]byte(test.input), nodeset)) + for range 2 { + SetNodeSetDefaults(nodeset) + require.Equal(t, ptr.To(test.want), nodeset.Spec.PreferKubernetesNodeName) + } + data, err := json.Marshal(nodeset) + require.NoError(t, err) + roundTripped := &slinkyv1beta1.NodeSet{} + require.NoError(t, json.Unmarshal(data, roundTripped)) + require.Equal(t, ptr.To(test.want), roundTripped.Spec.PreferKubernetesNodeName) + }) + } +} + func TestSetNodeSetDefaults(t *testing.T) { t.Run("nil nodeset is a no-op", func(t *testing.T) { SetNodeSetDefaults(nil) @@ -24,6 +77,7 @@ func TestSetNodeSetDefaults(t *testing.T) { require.Equal(t, ptr.To(DefaultNodeSetReplicas), ns.Spec.Replicas) require.Equal(t, DefaultNodeSetScalingMode, ns.Spec.ScalingMode) + require.Equal(t, ptr.To(DefaultNodeSetPreferKubernetesNodeName), ns.Spec.PreferKubernetesNodeName) require.Equal(t, ptr.To(DefaultNodeSetWorkloadDisruptionProtection), ns.Spec.WorkloadDisruptionProtection) require.Equal(t, DefaultNodeSetUpdateStrategyType, ns.Spec.UpdateStrategy.Type) require.NotNil(t, ns.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable) @@ -36,6 +90,7 @@ func TestSetNodeSetDefaults(t *testing.T) { ns := &slinkyv1beta1.NodeSet{} ns.Spec.Replicas = ptr.To(int32(3)) ns.Spec.ScalingMode = slinkyv1beta1.ScalingModeDaemonset + ns.Spec.PreferKubernetesNodeName = ptr.To(false) ns.Spec.UpdateStrategy.Type = slinkyv1beta1.OnDeleteNodeSetStrategyType maxUnavailable := intstr.FromString("57%") ns.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable = ptr.To(maxUnavailable) @@ -46,6 +101,7 @@ func TestSetNodeSetDefaults(t *testing.T) { require.Equal(t, ptr.To(int32(3)), ns.Spec.Replicas) require.Equal(t, slinkyv1beta1.ScalingModeDaemonset, ns.Spec.ScalingMode) + require.Equal(t, ptr.To(false), ns.Spec.PreferKubernetesNodeName) require.Equal(t, ptr.To(maxUnavailable), ns.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable) require.Equal(t, slinkyv1beta1.OnDeleteNodeSetStrategyType, ns.Spec.UpdateStrategy.Type) require.Equal(t, slinkyv1beta1.DeletePersistentVolumeClaimRetentionPolicyType, ns.Spec.PersistentVolumeClaimRetentionPolicy.WhenDeleted) diff --git a/internal/webhook/nodeset_webhook.go b/internal/webhook/nodeset_webhook.go index df89657e..34609be4 100644 --- a/internal/webhook/nodeset_webhook.go +++ b/internal/webhook/nodeset_webhook.go @@ -15,6 +15,7 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/klog/v2" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -61,6 +62,9 @@ func (r *NodeSetWebhook) ValidateUpdate(ctx context.Context, oldNodeSet, newNode if !apiequality.Semantic.DeepEqual(newNodeSet.Spec.VolumeClaimTemplates, oldNodeSet.Spec.VolumeClaimTemplates) { errs = append(errs, errors.New("cannot change volumeClaimTemplates after deployment")) } + if ptr.Deref(oldNodeSet.Spec.PreferKubernetesNodeName, true) != ptr.Deref(newNodeSet.Spec.PreferKubernetesNodeName, true) { + errs = append(errs, errors.New("preferKubernetesNodeName is immutable")) + } return warns, utilerrors.NewAggregate(errs) } @@ -80,6 +84,10 @@ func (r *NodeSetWebhook) validateNodeSet(nodeset *slinkyv1beta1.NodeSet) (admiss errs = append(errs, errors.New("controllerRef.name must not be empty")) } + if _, ok := nodeset.Spec.Template.Metadata.Labels[slinkyv1beta1.LabelNodeSetSlurmNodeNameMode]; ok { + errs = append(errs, errors.New("the Slurm node naming mode Pod label is reserved for the operator")) + } + if mu := nodeset.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable; mu != nil { switch mu.Type { case intstr.Int: diff --git a/internal/webhook/nodeset_webhook_test.go b/internal/webhook/nodeset_webhook_test.go index 37333ade..a5c19637 100644 --- a/internal/webhook/nodeset_webhook_test.go +++ b/internal/webhook/nodeset_webhook_test.go @@ -4,17 +4,264 @@ package webhook import ( + "context" + "fmt" + "testing" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + slinkyv1beta1 "github.com/SlinkyProject/slurm-operator/api/v1beta1" + "github.com/SlinkyProject/slurm-operator/internal/builder/labels" + nodesetutils "github.com/SlinkyProject/slurm-operator/internal/controller/nodeset/utils" "github.com/SlinkyProject/slurm-operator/internal/utils/testutils" ) +func TestPreferKubernetesNodeNameAdmission(t *testing.T) { + for _, test := range []struct { + name string + mutate func(*slinkyv1beta1.NodeSet) + wantErr string + }{ + {name: "valid"}, + {name: "allows unpinning", mutate: func(ns *slinkyv1beta1.NodeSet) { ns.Spec.PinToNode = false }}, + {name: "allows oversubscription", mutate: func(ns *slinkyv1beta1.NodeSet) { ns.Spec.OversubscribeNode = true }}, + {name: "accepts daemonset", mutate: func(ns *slinkyv1beta1.NodeSet) { ns.Spec.ScalingMode = slinkyv1beta1.ScalingModeDaemonset }}, + {name: "unpinned accepts custom arguments", mutate: func(ns *slinkyv1beta1.NodeSet) { + ns.Spec.PinToNode = false + ns.Spec.Slurmd.Args = []string{"-Nother"} + }}, + {name: "oversubscribed accepts custom arguments", mutate: func(ns *slinkyv1beta1.NodeSet) { + ns.Spec.OversubscribeNode = true + ns.Spec.Slurmd.Args = []string{"-Nother"} + }}, + {name: "accepts name argument", mutate: func(ns *slinkyv1beta1.NodeSet) { + ns.Spec.Slurmd.Args = []string{"-Nother"} + }}, + {name: "accepts node name env", mutate: func(ns *slinkyv1beta1.NodeSet) { + ns.Spec.Slurmd.Env = []corev1.EnvVar{{Name: "SLURM_NODE_NAME", Value: "other"}} + }}, + {name: "accepts options env", mutate: func(ns *slinkyv1beta1.NodeSet) { + ns.Spec.Slurmd.Env = []corev1.EnvVar{{Name: "SLURMD_OPTIONS", Value: "-Nother"}} + }}, + {name: "accepts command", mutate: func(ns *slinkyv1beta1.NodeSet) { + ns.Spec.Slurmd.Command = []string{"custom"} + }}, + {name: "rejects mode label", mutate: func(ns *slinkyv1beta1.NodeSet) { + ns.Spec.Template.Metadata.Labels = map[string]string{slinkyv1beta1.LabelNodeSetSlurmNodeNameMode: "KubernetesNode"} + }, wantErr: "the Slurm node naming mode Pod label is reserved"}, + } { + t.Run(test.name, func(t *testing.T) { + nodeset := &slinkyv1beta1.NodeSet{Spec: slinkyv1beta1.NodeSetSpec{ + ControllerRef: corev1.LocalObjectReference{Name: "slurm"}, + ScalingMode: slinkyv1beta1.ScalingModeStatefulset, + PinToNode: true, + PreferKubernetesNodeName: ptr.To(true), + }} + old := nodeset.DeepCopy() + if test.mutate != nil { + test.mutate(nodeset) + } + webhook := &NodeSetWebhook{} + createWarnings, createErr := webhook.ValidateCreate(context.Background(), nodeset) + updateWarnings, updateErr := webhook.ValidateUpdate(context.Background(), old, nodeset) + require.Empty(t, createWarnings) + require.Empty(t, updateWarnings) + if test.wantErr != "" { + require.ErrorContains(t, createErr, test.wantErr) + require.ErrorContains(t, updateErr, test.wantErr) + } else { + require.NoError(t, createErr) + require.NoError(t, updateErr) + warnings, err := webhook.ValidateUpdate(context.Background(), nodeset, old) + require.NoError(t, err) + require.Empty(t, warnings) + } + }) + } +} + +func TestPreferKubernetesNodeNameImmutable(t *testing.T) { + preferences := []struct { + name string + value *bool + }{ + {name: "omitted"}, + {name: "false", value: ptr.To(false)}, + {name: "true", value: ptr.To(true)}, + } + for _, oldPreference := range preferences { + for _, newPreference := range preferences { + t.Run(fmt.Sprintf("%s to %s", oldPreference.name, newPreference.name), func(t *testing.T) { + old := &slinkyv1beta1.NodeSet{Spec: slinkyv1beta1.NodeSetSpec{ + ControllerRef: corev1.LocalObjectReference{Name: "slurm"}, + ScalingMode: slinkyv1beta1.ScalingModeStatefulset, + PinToNode: true, PreferKubernetesNodeName: oldPreference.value, + }} + updated := old.DeepCopy() + updated.Spec.PreferKubernetesNodeName = newPreference.value + updated.Spec.Replicas = ptr.To[int32](2) + warns, err := (&NodeSetWebhook{}).ValidateUpdate(context.Background(), old, updated) + if ptr.Deref(oldPreference.value, true) != ptr.Deref(newPreference.value, true) { + require.ErrorContains(t, err, "preferKubernetesNodeName is immutable") + } else { + require.NoError(t, err) + } + require.Empty(t, warns) + }) + } + } +} + var _ = Describe("NodeSet Webhook", func() { + It("resolves an autoscaled worker name when its Node appears and preserves it after binding", func(ctx SpecContext) { + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "binding-worker.example.com", Annotations: map[string]string{ + slinkyv1beta1.AnnotationNodeHostnameOverride: "gpu-01", + }}} + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "binding-worker-0", Namespace: "default", Labels: map[string]string{ + labels.AppLabel: labels.WorkerApp, + slinkyv1beta1.LabelNodeSetSlurmNodeNameMode: string(slinkyv1beta1.SlurmNodeNameModeKubernetesNode), + }}, + Spec: corev1.PodSpec{Hostname: "binding-worker-0", Containers: []corev1.Container{{ + Name: "slurmd", Image: "slurmd", Args: []string{"-Z", "-N", "$(SLURM_NODE_NAME)"}, + Env: []corev1.EnvVar{{Name: "SLURM_NODE_NAME", ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.labels['" + slinkyv1beta1.LabelNodeSetPodHostname + "']"}, + }}}, + }}}, + } + Expect(k8sClient.Create(ctx, pod)).To(Succeed()) + DeferCleanup(func(ctx SpecContext) { Expect(k8sClient.Delete(ctx, pod, client.GracePeriodSeconds(0))).To(Succeed()) }) + Expect(pod.Spec.NodeName).To(BeEmpty()) + Expect(nodesetutils.GetSlurmNodeName(pod)).To(BeEmpty()) + binding := &corev1.Binding{ObjectMeta: metav1.ObjectMeta{Name: pod.Name, Namespace: pod.Namespace}, Target: corev1.ObjectReference{Kind: "Node", Name: node.Name}} + Expect(k8sClient.SubResource("binding").Create(ctx, pod, binding)).NotTo(Succeed()) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(pod), pod)).To(Succeed()) + Expect(pod.Spec.NodeName).To(BeEmpty()) + Expect(nodesetutils.GetSlurmNodeName(pod)).To(BeEmpty()) + Expect(k8sClient.Create(ctx, node)).To(Succeed()) + DeferCleanup(func(ctx SpecContext) { Expect(k8sClient.Delete(ctx, node)).To(Succeed()) }) + Expect(k8sClient.SubResource("binding").Create(ctx, pod, binding)).To(Succeed()) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(pod), pod)).To(Succeed()) + Expect(pod.Spec.NodeName).To(Equal(node.Name)) + Expect(pod.Spec.Hostname).To(Equal("binding-worker-0")) + Expect(pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname]).To(Equal("gpu-01")) + Expect(nodesetutils.GetSlurmNodeName(pod)).To(Equal("gpu-01")) + node.Annotations[slinkyv1beta1.AnnotationNodeHostnameOverride] = "gpu-02" + Expect(k8sClient.Update(ctx, node)).To(Succeed()) + Expect(k8sClient.SubResource("binding").Create(ctx, pod, binding)).NotTo(Succeed()) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(pod), pod)).To(Succeed()) + Expect(nodesetutils.GetSlurmNodeName(pod)).To(Equal("gpu-01")) + }) + + Context("Slurm naming schema", func() { + It("defaults null to true in either scaling mode", func(ctx SpecContext) { + for _, scalingMode := range []string{"StatefulSet", "DaemonSet"} { + nodeset := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": slinkyv1beta1.GroupVersion.String(), + "kind": "NodeSet", + "metadata": map[string]any{"generateName": "preferred-naming-", "namespace": "default"}, + "spec": map[string]any{ + "controllerRef": map[string]any{"name": "slurm"}, + "preferKubernetesNodeName": nil, "scalingMode": scalingMode, + }, + }} + Expect(k8sClient.Create(ctx, nodeset)).To(Succeed()) + DeferCleanup(func(ctx SpecContext) { Expect(k8sClient.Delete(ctx, nodeset)).To(Succeed()) }) + prefer, _, err := unstructured.NestedBool(nodeset.Object, "spec", "preferKubernetesNodeName") + Expect(err).NotTo(HaveOccurred()) + Expect(prefer).To(BeTrue()) + Expect(unstructured.SetNestedField(nodeset.Object, false, "spec", "preferKubernetesNodeName")).To(Succeed()) + Expect(apierrors.IsInvalid(k8sClient.Update(ctx, nodeset))).To(BeTrue()) + Expect(unstructured.SetNestedField(nodeset.Object, true, "spec", "preferKubernetesNodeName")).To(Succeed()) + Expect(k8sClient.Update(ctx, nodeset)).To(Succeed()) + } + }) + It("defaults an omitted preference to true while placement remains mutable", func(ctx SpecContext) { + nodeset := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": slinkyv1beta1.GroupVersion.String(), + "kind": "NodeSet", + "metadata": map[string]any{"name": "naming-schema", "namespace": "default"}, + "spec": map[string]any{"controllerRef": map[string]any{"name": "slurm"}}, + }} + Expect(k8sClient.Create(ctx, nodeset)).To(Succeed()) + DeferCleanup(func(ctx SpecContext) { Expect(k8sClient.Delete(ctx, nodeset)).To(Succeed()) }) + prefer, _, err := unstructured.NestedBool(nodeset.Object, "spec", "preferKubernetesNodeName") + Expect(err).NotTo(HaveOccurred()) + Expect(prefer).To(BeTrue()) + unstructured.RemoveNestedField(nodeset.Object, "spec", "preferKubernetesNodeName") + Expect(unstructured.SetNestedField(nodeset.Object, true, "spec", "pinToNode")).To(Succeed()) + Expect(k8sClient.Update(ctx, nodeset)).To(Succeed()) + Expect(unstructured.SetNestedField(nodeset.Object, true, "spec", "oversubscribeNode")).To(Succeed()) + Expect(k8sClient.Update(ctx, nodeset)).To(Succeed()) + Expect(unstructured.SetNestedField(nodeset.Object, false, "spec", "pinToNode")).To(Succeed()) + Expect(unstructured.SetNestedField(nodeset.Object, false, "spec", "oversubscribeNode")).To(Succeed()) + Expect(k8sClient.Update(ctx, nodeset)).To(Succeed()) + Expect(unstructured.SetNestedField(nodeset.Object, false, "spec", "preferKubernetesNodeName")).To(Succeed()) + Expect(apierrors.IsInvalid(k8sClient.Update(ctx, nodeset))).To(BeTrue()) + }) + It("preserves an explicit false from a Go client", func(ctx SpecContext) { + nodeset := &slinkyv1beta1.NodeSet{ + ObjectMeta: metav1.ObjectMeta{GenerateName: "pod-naming-", Namespace: "default"}, + Spec: slinkyv1beta1.NodeSetSpec{ + ControllerRef: corev1.LocalObjectReference{Name: "slurm"}, + PinToNode: true, + PreferKubernetesNodeName: ptr.To(false), + }, + } + Expect(k8sClient.Create(ctx, nodeset)).To(Succeed()) + DeferCleanup(func(ctx SpecContext) { Expect(k8sClient.Delete(ctx, nodeset)).To(Succeed()) }) + Expect(nodeset.Spec.PreferKubernetesNodeName).To(Equal(ptr.To(false))) + Expect(nodeset.Spec.EffectiveSlurmNodeNameMode()).To(Equal(slinkyv1beta1.SlurmNodeNameModePodHostname)) + nodeset.Spec.Replicas = ptr.To[int32](2) + Expect(k8sClient.Update(ctx, nodeset)).To(Succeed()) + Expect(nodeset.Spec.PreferKubernetesNodeName).To(Equal(ptr.To(false))) + for _, preference := range []*bool{nil, ptr.To(true)} { + updated := nodeset.DeepCopy() + updated.Spec.PreferKubernetesNodeName = preference + err := k8sClient.Update(ctx, updated) + Expect(apierrors.IsInvalid(err)).To(BeTrue()) + Expect(err.Error()).To(ContainSubstring("preferKubernetesNodeName is immutable")) + } + }) + It("keeps the preference immutable without restricting placement changes", func(ctx SpecContext) { + nodeset := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": slinkyv1beta1.GroupVersion.String(), + "kind": "NodeSet", + "metadata": map[string]any{"name": "node-naming-schema", "namespace": "default"}, + "spec": map[string]any{ + "controllerRef": map[string]any{"name": "slurm"}, + "preferKubernetesNodeName": true, "pinToNode": true, + }, + }} + Expect(k8sClient.Create(ctx, nodeset)).To(Succeed()) + DeferCleanup(func(ctx SpecContext) { Expect(k8sClient.Delete(ctx, nodeset)).To(Succeed()) }) + Expect(unstructured.SetNestedField(nodeset.Object, int64(2), "spec", "replicas")).To(Succeed()) + Expect(k8sClient.Update(ctx, nodeset)).To(Succeed()) + for _, setting := range []string{"pinToNode", "oversubscribeNode"} { + for _, value := range []bool{false, true, false, true} { + Expect(unstructured.SetNestedField(nodeset.Object, value, "spec", setting)).To(Succeed()) + Expect(k8sClient.Update(ctx, nodeset)).To(Succeed()) + } + } + updated := nodeset.DeepCopy() + Expect(unstructured.SetNestedField(updated.Object, false, "spec", "preferKubernetesNodeName")).To(Succeed()) + err := k8sClient.Update(ctx, updated) + Expect(apierrors.IsInvalid(err)).To(BeTrue()) + Expect(err.Error()).To(ContainSubstring("preferKubernetesNodeName is immutable")) + unstructured.RemoveNestedField(nodeset.Object, "spec", "preferKubernetesNodeName") + Expect(k8sClient.Update(ctx, nodeset)).To(Succeed()) + }) + }) + Context("When Creating a NodeSet with Validating Webhook", func() { It("Should deny if controllerRef.name is empty", func(ctx SpecContext) { nodeset := testutils.NewNodeset("test-nodeset", nil, 1) diff --git a/internal/webhook/pod_binding_webhook.go b/internal/webhook/pod_binding_webhook.go index 9631cf1a..bdb5281b 100644 --- a/internal/webhook/pod_binding_webhook.go +++ b/internal/webhook/pod_binding_webhook.go @@ -10,6 +10,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -18,6 +19,7 @@ import ( slinkyv1beta1 "github.com/SlinkyProject/slurm-operator/api/v1beta1" "github.com/SlinkyProject/slurm-operator/internal/builder/labels" + nodesetutils "github.com/SlinkyProject/slurm-operator/internal/controller/nodeset/utils" "github.com/SlinkyProject/slurm-operator/internal/utils/objectutils" ) @@ -66,19 +68,35 @@ func (r *PodBindingWebhook) Default(ctx context.Context, binding *corev1.Binding bindinglog.V(1).Info("ignoring pod", "pod", klog.KObj(pod)) return nil } + nodeNamed := podLabels[slinkyv1beta1.LabelNodeSetSlurmNodeNameMode] == string(slinkyv1beta1.SlurmNodeNameModeKubernetesNode) + if nodeNamed && pod.Spec.NodeName != "" { + return nil + } node := &corev1.Node{} nodeKey := types.NamespacedName{Name: binding.Target.Name} if err := r.Get(ctx, nodeKey, node); err != nil { - if apierrors.IsNotFound(err) { + if apierrors.IsNotFound(err) && !nodeNamed { return nil } return err } topologySpec := node.Annotations[slinkyv1beta1.AnnotationNodeTopologySpec] + slurmName := nodesetutils.GetDaemonSetPodHostname(node.Name, node.Annotations[slinkyv1beta1.AnnotationNodeHostnameOverride]) + if nodeNamed { + if problems := utilvalidation.IsDNS1123Label(slurmName); len(problems) != 0 { + return fmt.Errorf("slurm node name %q is not a valid hostname: %v", slurmName, problems) + } + } mutateFn := func(pod *corev1.Pod) error { + if pod.Annotations == nil { + pod.Annotations = make(map[string]string) + } pod.Annotations[slinkyv1beta1.AnnotationNodeTopologySpec] = topologySpec + if nodeNamed { + pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname] = slurmName + } return nil } if err := objectutils.PatchObject(r.Client, ctx, pod, mutateFn); err != nil { diff --git a/internal/webhook/pod_binding_webhook_test.go b/internal/webhook/pod_binding_webhook_test.go index 6b362b9b..7a50f2ff 100644 --- a/internal/webhook/pod_binding_webhook_test.go +++ b/internal/webhook/pod_binding_webhook_test.go @@ -6,6 +6,7 @@ package webhook import ( "context" "net/http" + "strings" "testing" "github.com/stretchr/testify/require" @@ -22,6 +23,55 @@ import ( "github.com/SlinkyProject/slurm-operator/internal/builder/labels" ) +func TestSlurmNodeNameModeBinding(t *testing.T) { + for _, test := range []struct { + name string + nodeName string + override string + wantName string + legacy bool + dryRun bool + wantErr bool + }{ + {name: "node name", nodeName: "kube-node", wantName: "kube-node"}, + {name: "full domain name", nodeName: "kube-node.example.com", wantName: "kube-node"}, + {name: "hostname override", nodeName: "kube-node.example.com", override: "gpu-01", wantName: "gpu-01"}, + {name: "invalid override", nodeName: "kube-node", override: "invalid.name", wantErr: true}, + {name: "maximum label length", nodeName: strings.Repeat("a", 63), wantName: strings.Repeat("a", 63)}, + {name: "reject long name", nodeName: strings.Repeat("a", 64), wantErr: true}, + {name: "legacy identity unchanged", nodeName: "kube-node", legacy: true}, + {name: "dry run does not mutate", nodeName: "kube-node", dryRun: true}, + } { + t.Run(test.name, func(t *testing.T) { + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "worker-0", Namespace: "slurm", Labels: map[string]string{ + labels.AppLabel: labels.WorkerApp, + slinkyv1beta1.LabelNodeSetSlurmNodeNameMode: string(slinkyv1beta1.SlurmNodeNameModeKubernetesNode), + }}} + if test.legacy { + delete(pod.Labels, slinkyv1beta1.LabelNodeSetSlurmNodeNameMode) + } + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: test.nodeName, Annotations: map[string]string{slinkyv1beta1.AnnotationNodeHostnameOverride: test.override}}} + kubeClient := fake.NewClientBuilder().WithObjects(pod, node).Build() + webhook := &PodBindingWebhook{Client: kubeClient} + ctx := admission.NewContextWithRequest(context.Background(), admission.Request{AdmissionRequest: v1.AdmissionRequest{DryRun: &test.dryRun}}) + binding := &corev1.Binding{ObjectMeta: pod.ObjectMeta, Target: corev1.ObjectReference{Name: node.Name}} + if test.wantErr { + require.Error(t, webhook.Default(ctx, binding)) + return + } + require.NoError(t, webhook.Default(ctx, binding)) + require.NoError(t, webhook.Default(ctx, binding)) + require.NoError(t, kubeClient.Get(ctx, client.ObjectKeyFromObject(pod), pod)) + if test.dryRun || test.legacy { + require.Empty(t, pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname]) + } else { + require.Equal(t, test.wantName, pod.Labels[slinkyv1beta1.LabelNodeSetPodHostname]) + require.Equal(t, map[string]string{slinkyv1beta1.AnnotationNodeTopologySpec: ""}, pod.Annotations) + } + }) + } +} + func TestPodBindingWebhook_Default(t *testing.T) { workerPod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{