Skip to content

Commit 6fa177a

Browse files
authored
Merge branch 'main' into fix/fq-retain-taints-during-validation
2 parents 3ab6346 + 439858a commit 6fa177a

12 files changed

Lines changed: 1015 additions & 96 deletions

‎commons/pkg/distributedlock/nodelock.go‎

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -67,20 +67,41 @@ type NodeLock interface {
6767

6868
// NewNodeLock creates a new NodeLock instance. scheme is used to resolve
6969
// the GVK of maintenance objects for owner references. metrics may be nil.
70-
func NewNodeLock(c client.Client, scheme *runtime.Scheme, namespace string, metrics LockMetrics) NodeLock {
71-
return &nodeLock{
70+
func NewNodeLock(
71+
c client.Client, scheme *runtime.Scheme, namespace string, metrics LockMetrics, opts ...Option,
72+
) NodeLock {
73+
lock := &nodeLock{
7274
Client: c,
7375
scheme: scheme,
7476
namespace: namespace,
7577
metrics: metrics,
7678
}
79+
80+
for _, opt := range opts {
81+
opt(lock)
82+
}
83+
84+
return lock
85+
}
86+
87+
// Option configures optional NodeLock behaviour.
88+
type Option func(*nodeLock)
89+
90+
// WithLeaseName derives the lock lease name from the node name. Without it the
91+
// lease is named after the node, which is the lock every janitor controller
92+
// shares; a lock that must not contend with those uses a distinct name.
93+
func WithLeaseName(leaseName func(nodeName string) string) Option {
94+
return func(lock *nodeLock) {
95+
lock.leaseName = leaseName
96+
}
7797
}
7898

7999
type nodeLock struct {
80100
client.Client
81101
scheme *runtime.Scheme
82102
namespace string
83103
metrics LockMetrics
104+
leaseName func(nodeName string) string
84105
}
85106

86107
func (lock *nodeLock) LockNode(ctx context.Context, maintenanceObject client.Object, nodeName string) bool {
@@ -186,7 +207,7 @@ func (lock *nodeLock) CheckUnlock(ctx context.Context,
186207

187208
err = lock.Delete(ctx, lease)
188209
if err != nil {
189-
return lock.handleNotFoundError(err, nodeName, nodeName)
210+
return lock.handleNotFoundError(err, nodeLockName, nodeName)
190211
}
191212

192213
slog.InfoContext(ctx, "Node lock successfully released for maintenance resource",
@@ -202,26 +223,35 @@ func (lock *nodeLock) CheckUnlock(ctx context.Context,
202223
func (lock *nodeLock) getNodeLockLease(
203224
ctx context.Context, nodeName string,
204225
) (string, *coordinationv1.Lease, error) {
226+
leaseName := lock.leaseNameFor(nodeName)
205227
nodeLockNamespaceName := types.NamespacedName{
206-
Name: nodeName,
228+
Name: leaseName,
207229
Namespace: lock.namespace,
208230
}
209231

210232
var lease coordinationv1.Lease
211233

212234
err := lock.Get(ctx, nodeLockNamespaceName, &lease)
213235
if err != nil {
214-
return nodeName, nil, err
236+
return leaseName, nil, err
215237
}
216238

217239
ownerReferences := lease.GetOwnerReferences()
218240
if len(ownerReferences) != 1 {
219-
return "", nil, fmt.Errorf(
241+
return leaseName, nil, fmt.Errorf(
220242
"found an unexpected number of owner references on lock %s: %d",
221-
nodeName, len(ownerReferences))
243+
leaseName, len(ownerReferences))
244+
}
245+
246+
return leaseName, &lease, err
247+
}
248+
249+
func (lock *nodeLock) leaseNameFor(nodeName string) string {
250+
if lock.leaseName == nil {
251+
return nodeName
222252
}
223253

224-
return nodeName, &lease, err
254+
return lock.leaseName(nodeName)
225255
}
226256

227257
// resolveGVK extracts the API version and kind from a maintenance object.

‎commons/pkg/distributedlock/nodelock_test.go‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,56 @@ func TestNodeLock_GetHolder_ExpectedLeaseOwnerReference(t *testing.T) {
7878
assert.Equal(t, expected, *holder)
7979
}
8080

81+
func TestNodeLock_WithLeaseName_DoesNotContendWithDefaultLease(t *testing.T) {
82+
t.Parallel()
83+
84+
testScheme := runtime.NewScheme()
85+
require.NoError(t, corev1.AddToScheme(testScheme))
86+
require.NoError(t, coordinationv1.AddToScheme(testScheme))
87+
88+
const nodeName = "test-node"
89+
90+
defaultLease := &coordinationv1.Lease{
91+
ObjectMeta: metav1.ObjectMeta{
92+
Name: nodeName,
93+
Namespace: testNamespace,
94+
OwnerReferences: []metav1.OwnerReference{{
95+
APIVersion: "janitor.dgxc.nvidia.com/v1alpha1",
96+
Kind: "RebootNode",
97+
Name: "other-reboot",
98+
UID: "other-uid",
99+
}},
100+
},
101+
}
102+
resource := newTestResource("test-resource")
103+
kubeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(defaultLease, resource).Build()
104+
lock := NewNodeLock(kubeClient, testScheme, testNamespace, nil,
105+
WithLeaseName(func(nodeName string) string { return "claim." + nodeName }))
106+
ctx := context.Background()
107+
108+
require.True(t, lock.LockNode(ctx, resource, nodeName),
109+
"a renamed lock must not be blocked by the default node lease")
110+
111+
var claim coordinationv1.Lease
112+
require.NoError(t, kubeClient.Get(ctx, types.NamespacedName{
113+
Name: "claim." + nodeName, Namespace: testNamespace,
114+
}, &claim))
115+
116+
holder, err := lock.GetHolder(ctx, nodeName)
117+
require.NoError(t, err)
118+
assert.Equal(t, resource.GetUID(), holder.UID)
119+
120+
require.False(t, lock.CheckUnlock(ctx, resource, nodeName))
121+
122+
err = kubeClient.Get(ctx, types.NamespacedName{Name: "claim." + nodeName, Namespace: testNamespace}, &claim)
123+
assert.True(t, apierrors.IsNotFound(err), "CheckUnlock must delete the renamed lease")
124+
125+
var untouched coordinationv1.Lease
126+
require.NoError(t, kubeClient.Get(ctx, types.NamespacedName{Name: nodeName, Namespace: testNamespace}, &untouched),
127+
"the default node lease must be left alone")
128+
assert.Equal(t, types.UID("other-uid"), untouched.OwnerReferences[0].UID)
129+
}
130+
81131
// testResource is a simple object that satisfies client.Object for tests.
82132
// We use ConfigMap because it's registered in the corev1 scheme.
83133
func newTestResource(name string) *corev1.ConfigMap {

‎docs/configuration/lifecycle-manager.md‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ The validating webhook rejects a request that:
122122
- leaves `version` at zero
123123
- names a node that does not exist
124124
- sets `startTime` in the past
125+
- names a node that already has an open MR
125126
- changes any stored field of `healthEvent` after creation
126127

127128
The mutating webhook fills in `id` and `generatedTimestamp` if absent. It sets the publishing agent to `lifecycle-manager`, preserves the supplied agent in `maintenanceRequestRequesterAgent`, and always sets the `maintenanceRequestName` metadata key.
@@ -130,10 +131,22 @@ The mutating webhook fills in `id` and `generatedTimestamp` if absent. It sets t
130131

131132
The controller sets a `HealthEventEmitted` condition once the opening event has been accepted
132133
by platform-connector, and takes a finalizer on the resource. On deletion it publishes the
133-
clearing event, releases the node lock, and only then drops the finalizer — so a failed
134+
clearing event, releases its leases, and only then drops the finalizer — so a failed
134135
clearing publish leaves the object in `Terminating` and retries, rather than losing the clear
135136
and leaving the node cordoned indefinitely.
136137

138+
The controller uses two Lease objects in the lifecycle-manager namespace:
139+
140+
- **Claim** (`mr-claim.<node-name>`): the MR takes it before the opening event and holds it until
141+
deletion, so a node has at most one open MR. If a second MR for the node passes the webhook, the controller
142+
sets `HealthEventEmitted=False` with reason `Rejected` and does not retry it. If the node name
143+
is too long for a Lease name, the end of the name is replaced with a short hash.
144+
- **Janitor node lock** (`<node-name>`): the lock that janitor operations such as RebootNode
145+
share. The MR holds it only while it publishes the opening event, so the event does not start
146+
maintenance while a janitor operation runs. If the lock is held, the controller sets reason
147+
`Blocked` and retries every 30 seconds. The MR releases the lock right after it publishes, so
148+
the janitor operation that its event starts can take the lock.
149+
137150
If the clearing event can never be delivered, the resource stays in `Terminating`. Removing
138151
the finalizer by hand is the escape hatch, at the cost of the node staying cordoned until an
139152
operator clears it.

‎docs/tutorials/requesting-node-maintenance.md‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,7 @@ Error from server (Forbidden): error when creating "STDIN": admission webhook "v
324324
| `spec.healthEvent.version is required` | Set `version: 1`. |
325325
| `spec.healthEvent.isHealthy must be false for an opening event` | Set `isHealthy: false`. |
326326
| `spec.startTime must be in the future` | Set a time in the future, or remove `startTime`. |
327+
| `node "<node-name>" already has an open MaintenanceRequest "<name>"; delete it before creating another` | A node can have only one open MaintenanceRequest. Wait for the open MaintenanceRequest to finish and delete it, then create the new one. |
327328
| `spec.healthEvent is immutable after creation`, or `spec.healthEvent.<field> is immutable after creation` | Delete the MaintenanceRequest, then create a new one. |
328329

329330
> **Note:** When you delete the MaintenanceRequest, NVSentinel uncordons the node. Pods can start on the node before you create the new MaintenanceRequest.
@@ -335,13 +336,14 @@ Read the reason and the message of the condition:
335336
```bash
336337
kubectl get maintenancerequest "maintenance-${NODE}" \
337338
-o jsonpath='{range .status.conditions[*]}{.type}={.status} reason={.reason} message={.message}{"\n"}{end}'
338-
# Expected when another operation holds the node:
339-
# HealthEventEmitted=False reason=Blocked message=Node <node-name> is locked by another maintenance operation.
339+
# Expected when a janitor operation holds the node:
340+
# HealthEventEmitted=False reason=Blocked message=Node <node-name> is locked by another maintenance operation (RebootNode/<name>).
340341
```
341342

342343
| Reason | Meaning | Fix |
343344
| --- | --- | --- |
344-
| `Blocked` | Another operation holds the lock on the node. Examples are a second MaintenanceRequest or a janitor reboot on the same node. lifecycle-manager tries again every 30 seconds. | Wait for the other operation to complete, or delete the second MaintenanceRequest. |
345+
| `Blocked` | A janitor operation, such as a reboot, holds the lock on the node. Or a MaintenanceRequest for the node is being deleted and has not yet released the node. lifecycle-manager tries again every 30 seconds. | Wait for the other operation to complete. |
346+
| `Rejected` | The node already has an open MaintenanceRequest. lifecycle-manager does not send the health event and does not try again. | Delete this MaintenanceRequest. Create it again after the open MaintenanceRequest is deleted. |
345347
| `EmitFailed` | lifecycle-manager could not publish the health event to platform-connector. lifecycle-manager tries again. | Make sure that platform-connector runs on the same node as lifecycle-manager. |
346348

347349
lifecycle-manager sends the health event through a socket on its own node. Compare the `NODE` column of the two pods:
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// Copyright (c) 2026, NVIDIA CORPORATION. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package controller
16+
17+
import (
18+
"regexp"
19+
"strings"
20+
"testing"
21+
22+
"github.com/stretchr/testify/assert"
23+
"github.com/stretchr/testify/require"
24+
"k8s.io/apimachinery/pkg/util/validation"
25+
)
26+
27+
func TestClaimLeaseName_FitsAndStaysReadable(t *testing.T) {
28+
t.Parallel()
29+
30+
// "mr-claim." plus the kept prefix fills 244 characters before "-<8 hex>",
31+
// so node[234] is the last character kept from a node name that is too long.
32+
const lastKept = 234
33+
34+
hashSuffix := regexp.MustCompile(`-[0-9a-f]{8}$`)
35+
36+
tests := []struct {
37+
name string
38+
nodeName string
39+
want string
40+
wantHashed bool
41+
}{
42+
{
43+
name: "typical node name is prefixed unchanged",
44+
nodeName: "gpu-node-7",
45+
want: "mr-claim.gpu-node-7",
46+
},
47+
{
48+
name: "name that exactly fits 253 characters is unchanged",
49+
nodeName: strings.Repeat("a", validation.DNS1123SubdomainMaxLength-len(claimLeasePrefix)),
50+
want: claimLeasePrefix + strings.Repeat("a", validation.DNS1123SubdomainMaxLength-len(claimLeasePrefix)),
51+
},
52+
{
53+
name: "name one character too long keeps its start and gains a hash",
54+
nodeName: strings.Repeat("a", validation.DNS1123SubdomainMaxLength-len(claimLeasePrefix)+1),
55+
wantHashed: true,
56+
},
57+
{
58+
name: "cut landing on a dot drops the dot",
59+
nodeName: strings.Repeat("a", lastKept) + "." + strings.Repeat("b", 20),
60+
wantHashed: true,
61+
},
62+
{
63+
name: "cut landing on a dash drops the dash",
64+
nodeName: strings.Repeat("a", lastKept) + "-" + strings.Repeat("b", 20),
65+
wantHashed: true,
66+
},
67+
}
68+
69+
for _, tt := range tests {
70+
t.Run(tt.name, func(t *testing.T) {
71+
t.Parallel()
72+
73+
got := ClaimLeaseName(tt.nodeName)
74+
75+
assert.Empty(t, validation.IsDNS1123Subdomain(got), "claim name %q must be a valid lease name", got)
76+
assert.NotEqual(t, tt.nodeName, got, "claim must not collide with the janitor lock for the same node")
77+
78+
if !tt.wantHashed {
79+
assert.Equal(t, tt.want, got)
80+
81+
return
82+
}
83+
84+
assert.LessOrEqual(t, len(got), validation.DNS1123SubdomainMaxLength)
85+
assert.Regexp(t, hashSuffix, got)
86+
assert.True(t, strings.HasPrefix(got, claimLeasePrefix+strings.Repeat("a", lastKept)),
87+
"the readable start of the node name must be kept")
88+
assert.NotContains(t, got, ".-")
89+
assert.NotContains(t, got, "--")
90+
})
91+
}
92+
}
93+
94+
func TestClaimLeaseName_LongNamesWithSharedStartDoNotCollide(t *testing.T) {
95+
t.Parallel()
96+
97+
shared := strings.Repeat("a", 250)
98+
99+
first := ClaimLeaseName(shared + "-1")
100+
second := ClaimLeaseName(shared + "-2")
101+
102+
require.NotEqual(t, first, second)
103+
}

0 commit comments

Comments
 (0)