fix: release dangling node lock

[peachest]

What type of PR is this?

/kind bug

Which issue(s) this PR fixes:
Fixes #714
Fixes #810
Fixes #1244

Background

The background details of the above issues and the history of the fixes are summarised here:

The root of the problem originates from the poor design of the device-plugin framework provided by k8s. In the device-plugin framework, the device-plugin can report the extended resources on the node through the ListAndWatch interface, and handle the initialization of the device through the Allocate interface, as well as mounting the necessary contents for the container to run, including the device file, the control device file, the driver directory, the command line tools, the environment variables, etc. Kubelet will call the Allocate interface of the device-plugin after the Pod is bound to the node. After the Pod is bound to a node, Kubelet calls the Allocate interface of the device-plugin to set up and mount the response for the container based on the returned information.

If the device plugin only needs to manage physical hardware devices, and each device is exclusive to the container (i.e., a device can only be mounted to one container at a time), then all of the above works very well. Because in this case each device is stateless, each device corresponds to a unique extension resource reported by the device plugin via ListAndWatch and has a unique ID; when the Allocate interface is called, kubelet selects the appropriate resource among the unallocated resources and passes the corresponding ID List as a request parameter. The plugin simply selects the corresponding hardware devices based on the ID List for container.

Until the need to manage virtualised device resources arises, at which point each physical device is no longer stateless, using HAMi's nvidia share as an example. Eeach Nvidia device is divided into 100 for sharing. When scheduling Pods, the scheduler not only needs to determine the Node for Pod, but also needs to decide the specific physical device to be allocated to each container based on the remaining available resources of each device on the node, so the actual device that the container needs to mount is no longer determined by the kubelet or the device plugin, but rather by the scheduler's scheduling results. The scheduler can easily record the scheduling result to the corresponding Pod, e.g. by writing it to the Pod annotation during the bind phase, as implemented in HAMi.

On the one hand, as mentioned above, the interface of the device plugin framework is stateless from the very beginning of its design, and device plugins cannot get any information about the Pod or container that needs to be assigned a device through the parameters of the Allocate interface provided by the framework. On the other hand, the k8s scheduling mechanism divides Pod scheduling into a serial Filter phase and a parallel Bind phase, where different Pods may be bound to the same node at the same time, so the device plugin can't get the Pod info which is currently being allocated by kubelet by geting pods bound to the node.

HAMi chooses to use NodeLock to serialise Pod bindings and device Allocating, by adding a NodeLock to Node Annotations so that at most one Pod is bound to the node at a time and processed by the device plugin called by Kubelet. The NodeLock records the Pod, timestamp, and other information, and the device plugin parses it to get the Pod that is currently being processed and gets the scheduler's scheduling result through the Pod annotation to complete the allocation of the device. When the device plugin finishes allocating, it removes the NodeLock. So that next pod can be bound by scheduler and allocated by device plugin.

The initial NodeLock, while recording the Pod that set the NodeLock, did not restrict which pod can release the lock and thus could lead to Pod B accidentally releasing the nodeLock set by Pod A, and further lead to multiple Pods pending. The PR #714 adds an owner check for the lock. Also, it allows skipping the owner check and releasing directly in case the lock expires. So after the PR #714 fix, the only way Pod B can release a lock set by Pod A is if the lock expires.

Subsequently, Issue #810 proposed that PR #714 would cause Pod B to be pending for a long time waiting for Pod A's NodeLock to be expired and released. Therefore, in order to reduce the waiting time, PR #1244 proposes to allow the NodeLock timeout to be set via environment variables, and change the original 5min to user configurable.

Problem Description

The NodeLock will become dangling due to the owner Pod being accidentally deleted between the time the scheduler sets a NodeLock with this Pod as the owner in the bind phase and the time device plugin finish allocating Pod and release nodelock.
All subsequent Pods that are scheduled to the same node are unable to release the dangling NodeLock by any means other than waiting for the NodeLock to expire.

What this PR does

This PR fixes the problem described above.
There are two main fixes:

1. add goto ReleaseNodeLocks to the PatchPodAnnotations step in the Bind function implementation to clear the NodeLock when an error occurs
2. allow removal of the dangling NodeLock even if not expired

NodeLock can now be deleted by non-holders (Pods) under timeout or dangling conditions.

Problem Reproducing

Since PatchPodAnnotation does not release the NodeLock when an error occurs, if a pod is deleted immediately after the NodeLock is set at the time of binding, and a new pod needs to be bound to the same node within 5 minutes, the new pod will be pending, and the event binding reject: node <nodename> has been locked within 5 minutes will arise when inspecting pod via kubectl describe

The above problem can be reproduced stably using the following code. This code deletes the owner Pod as soon as a NodeLock is detected, so that the NodeLock will become dangling and persist on the node untile expire, and other Pods have to wait for its timeout.

package main

import (
	"context"
	"flag"
	"fmt"
	"os"
	"strings"
	"time"

	"github.com/pkg/errors"
	"github.com/sirupsen/logrus"
	corev1 "k8s.io/api/core/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/client-go/informers"
	"k8s.io/client-go/kubernetes"
	"k8s.io/client-go/tools/cache"
	ctrl "sigs.k8s.io/controller-runtime"
)

const (
	NodeLockKey  = "hami.io/mutex.lock"
	MaxLockRetry = 5
	NodeLockSep  = ","
)

// Using Global Variables to Simplify Code
var (
	// NodeName is the name of the node to watch for locks
	NodeName   string
	logger     *logrus.Logger
	kubeClient kubernetes.Interface
	ctx        context.Context
)

func main() {
	flag.StringVar(&NodeName, "node", "zzdev39", "The node name to handle")
	flag.Parse()

	fmt.Printf("Node lock watcher started for node: %s\n", NodeName)

	logger = logrus.New()
	SetupInformer()

	// Clean possible node lock before starting
	if err := releaseNodeLock(); err != nil {
		logger.WithError(err).Errorf("Failed to release node lock for node %s", NodeName)
		os.Exit(1)
	}

	// Block until CTRL-C
	<-ctx.Done()
	logger.Info("Shutting down gracefully...")

	// Release node lock before exiting
	if err := releaseNodeLock(); err != nil {
		logger.WithError(err).Errorf("Failed to release node lock for node %s", NodeName)
		os.Exit(1)
	}
	os.Exit(0)
}

func SetupInformer() {
	config := ctrl.GetConfigOrDie()
	kubeClient = kubernetes.NewForConfigOrDie(config)
	kubeFactory := informers.NewSharedInformerFactory(kubeClient, 10*time.Minute)
	nodeInformer := kubeFactory.Core().V1().Nodes()

	ctx = ctrl.SetupSignalHandler()

	// Listen to nodelock added
	nodeInformer.Informer().AddEventHandler(GetNodeLockUpdateHandler(ctx))

	kubeFactory.Start(ctx.Done())
	if !cache.WaitForCacheSync(
		ctx.Done(),
		nodeInformer.Informer().HasSynced,
	) {
		logger.Errorf("failed to sync informers")
		os.Exit(1)
	}
}

// GetNodeLockUpdateHandler returns a ResourceEventHandler that
// delete the pod when the node lock annotation added to trigger NodeLock 5min issue
func GetNodeLockUpdateHandler(ctx context.Context) cache.ResourceEventHandler {
	return cache.ResourceEventHandlerFuncs{
		UpdateFunc: func(oldObj, newObj any) {
			oldNode := oldObj.(*corev1.Node)
			newNode := newObj.(*corev1.Node)

			if oldNode.Name != NodeName {
				return // Ignore updates for other nodes
			}

			if oldNode.ResourceVersion == newNode.ResourceVersion {
				return // No change
			}

			var oldVal, newVal string
			var oldExists, newExists bool
			if oldNode.Annotations != nil {
				oldVal, oldExists = oldNode.Annotations[NodeLockKey]
			}

			if newNode.Annotations != nil {
				newVal, newExists = newNode.Annotations[NodeLockKey]
			}

			if oldVal == newVal {
				return // No change in lock annotation
			}
			logger.WithFields(logrus.Fields{
				"old_lock": oldVal,
				"new_lock": newVal,
				"node":     newNode.Name,
			}).Info("Node lock annotation changed")

			if !oldExists && newExists {
				// New lock added
				_, ns, name, err := ParseNodeLock(newVal)
				if err != nil {
					logger.WithError(err).WithField("node", newNode.Name).
						Error("Failed to parse new lock annotation")
					return
				}
				// Parse lock info and delete pod
				kubeClient.CoreV1().Pods(ns).Delete(ctx, name, metav1.DeleteOptions{})
			}
		},
	}
}

// Parse Node Lock like:
//
// hami.io/mutex.lock: 2025-07-24T10:12:33Z,hyx,ubuntu-deployment-86d59cb8d-7xpnm
//
// Refer to: https://github.com/Project-HAMi/HAMi/blob/master/pkg/util/nodelock/nodelock.go#L132-L155
func ParseNodeLock(value string) (lockTime time.Time, ns, name string, err error) {
	if !strings.Contains(value, NodeLockSep) {
		lockTime, err = time.Parse(time.RFC3339, value)
		return lockTime, "", "", err
	}
	s := strings.Split(value, NodeLockSep)
	if len(s) != 3 {
		lockTime, err = time.Parse(time.RFC3339, value)
		return lockTime, "", "", err
	}
	lockTime, err = time.Parse(time.RFC3339, s[0])
	return lockTime, s[1], s[2], err
}

// releaseNodeLock releases the lock on the node by removing the annotation
func releaseNodeLock() error {
	// Release node
	var node *corev1.Node
	var err = errors.New("")
	ctx := context.Background()
	for i := 0; i < MaxLockRetry && err != nil; i++ {
		node, err = kubeClient.CoreV1().Nodes().Get(ctx, NodeName, metav1.GetOptions{})
		if err != nil {
			logger.WithError(err).Errorf("Failed to get node %s", NodeName)
			time.Sleep(100 * time.Millisecond)
			continue
		}
		delete(node.Annotations, NodeLockKey)
		_, err = kubeClient.CoreV1().Nodes().Update(ctx, node, metav1.UpdateOptions{})
	}
	logger.Info("Node lock released successfully")
	return err
}

Usage：

go build -o test-node-lock main.go
# Use sudo to read KUBECONFIG
sudo ./test-node-lock --node=<your node>

Check the UUID of GPUs on the node:

nvidia-smi -L
# or
nvidia-smi --query-gpu=uuid --format=csv

Then deploy using Depolyment and limit the scheduling scope to the corresponding node with use-uuid:

apiVersion: apps/v1
kind: Deployment
metadata:
    name: ubuntu-deployment
spec:
    replicas: 2
    selector:
        matchLabels:
            app: ubuntu
    template:
        metadata:
            labels:
                app: ubuntu
                nvidia.com/use-gpuuuid: <GPU1>,<GPU2>
        spec:
            containers:
              - name: ubuntu-cnt-0
                image: ubuntu:22.04
                imagePullPolicy: IfNotPresent
                command: [ "sh", "-c", "sleep infinity" ]
                resources:
                    limits:
                       nvidia.com/gpu: 1

第一个 Pod 被调度时，会在设置 NodeLock 时被删除，NodeLock 会保持在 Node 上没有被删除。然后 Deployment 自动创建出新的 Pod，这些新的 Pod 必定会处于 pending 状态，并且出现事件 binding reject: node <nodename> has been locked within 5 minutes

When the first pod is scheduled and nodeLock has been set, the code will delete the pod immediately, and new pods will be pending, and the event binding reject: node <nodename> has been locked within 5 minutes will arise when inspecting pod via kubectl describe

Notes

Special notes for your reviewer:

This PR only eliminate the problem of NodeLock becoming dangling due to an accidental deletion of a Pod, resulting in other Pods having to wait for the NodeLock to timeout and remain in a pending state.

There are still other scenarios that can cause a Pod to be pending due to NodeLock, such as having a large number of Pods on the same node that need to be bound at the same time, each of which needs to wait for the previous Pod to be processed before it can enter its own binding process.

Does this PR introduce a user-facing change?

No, this PR doesn't change any user-facing nor function signature.
However, we do change the name of a single parameter from timeout to skipNodeLockOwnerCheck in function ReleaseNodeLock for better readability and comprehensibility

[gemini-code-assist]

Summary of Changes

Hello @peachest, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical issue where Kubernetes NodeLocks could become "dangling" if the Pod that set the lock was unexpectedly deleted before releasing it. This situation would block subsequent Pods from being scheduled on the affected node until the lock naturally expired. The changes introduce mechanisms to proactively clear these dangling locks, improving node resource utilization and preventing unnecessary Pod pending states.

Highlights

• Error Handling in Scheduler Binding: Modified the scheduler's Bind function to ensure that if an error occurs during the patching of Pod annotations (e.g., setting the NodeLock), the NodeLock is immediately released instead of leaving it in a potentially stuck state.
• Proactive Dangling NodeLock Release: Enhanced the LockNode function to detect and release NodeLocks that are either expired or associated with a Pod that no longer exists (i.e., a "dangling" lock). This allows new Pods to acquire the lock without waiting for a timeout.
• Improved Readability for Lock Release: Renamed the timeout parameter to skipNodeLockOwnerCheck in the ReleaseNodeLock function for better readability and comprehensibility.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a critical bug where a NodeLock could become dangling if the owner pod is deleted, preventing other pods from being scheduled on that node. The proposed solution, which involves releasing the lock on error during pod annotation patching and allowing the removal of dangling locks, is well-reasoned and thoroughly explained. The changes are logical and directly address the issue. I've identified one potential issue in the implementation of the dangling lock check that could cause problems with older lock formats and have provided a suggestion for a fix.

[wawa0210]

/gemini review

[gemini-code-assist]

Code Review

This pull request effectively addresses the issue of dangling node locks by ensuring locks are released on error during the pod binding process and by allowing the removal of locks belonging to deleted pods. The changes are logical and well-implemented. I've provided a couple of suggestions to further improve code clarity and efficiency.

[archlitchi]

CC @Shouren

[wawa0210]

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿

---

cc @s

[archlitchi]

please fix the UT

[Shouren]

/lgtm

[Shouren]

@archlitchi This PR seems fine to me. And I think we can create a new issue with 'good first issue' label to see if anyone can implement the optimization of release lock by handling delete event of pod in scheduler.

[archlitchi]

@archlitchi This PR seems fine to me. And I think we can create a new issue with 'good first issue' label to see if anyone can implement the optimization of release lock by handling delete event of pod in scheduler.

agree, you could open that issue and see if anyone is interested

[codecov]

Codecov Report

❌ Patch coverage is 68.18182% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/util/nodelock/nodelock.go	71.42%	4 Missing and 2 partials ⚠️
pkg/scheduler/scheduler.go	0.00%	1 Missing ⚠️

Flag	Coverage Δ
unittests	65.33% <68.18%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
pkg/scheduler/scheduler.go	44.32% <0.00%> (+0.11%)	⬆️
pkg/util/nodelock/nodelock.go	52.98% <71.42%> (+2.15%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[archlitchi]

please resolve this conflict, we're ready to go:)

[peachest]

We've synchronised to the latest master branch and resolved the conflict.

[hami-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, peachest, Shouren

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [archlitchi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[archlitchi]

/lgtm