Skip to content

Commit 65281ec

Browse files
committed
refactor(pool): improve pod recycle and allocation logic with finalizers
- Add FinalizerPoolRecycle for pool mode BatchSandbox with restart policy - Implement ensureFinalizer helper to manage finalizers robustly - Handle pool recycle process before task cleanup on BatchSandbox deletion - Enhance canAllocate to exclude pods not ready after recycle confirmation - Modify handlePodRecycle to support restart timeout from pool annotations - Adjust PoolReconciler to process pod recycle before scheduling and allocation - Introduce needsRecycleConfirmation to detect pods needing recycle handling - Count recycling pods in pool scaling decisions instead of restarting pods - Update allocator to skip pods that cannot allocate (e.g., still recycling) - Add unit tests for canAllocate logic on pod labels and annotations - Update e2e test to verify Delete policy deletes pods and pool replenishment - Remove deprecated InitialRestartCounts from PodRecycleMeta for clarity - Refactor restartTracker to remove embedded restartTimeout field - Update restartTracker HandleRestart call to accept timeout parameter - Clean up logging and error handling for finalizer and pod recycle operations
1 parent 6255351 commit 65281ec

13 files changed

Lines changed: 976 additions & 902 deletions

kubernetes/cmd/controller/main.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,13 +266,14 @@ func main() {
266266
os.Exit(1)
267267
}
268268
kubeClient := kubernetes.NewForConfigOrDie(mgr.GetConfig())
269-
restartTracker := controller.NewRestartTracker(mgr.GetClient(), kubeClient, mgr.GetConfig(), restartTimeout)
269+
restartTracker := controller.NewRestartTracker(mgr.GetClient(), kubeClient, mgr.GetConfig())
270270
if err := (&controller.PoolReconciler{
271271
Client: mgr.GetClient(),
272272
Scheme: mgr.GetScheme(),
273273
Recorder: mgr.GetEventRecorderFor("pool-controller"),
274274
Allocator: controller.NewDefaultAllocator(mgr.GetClient()),
275275
RestartTracker: restartTracker,
276+
RestartTimeout: restartTimeout,
276277
}).SetupWithManager(mgr); err != nil {
277278
setupLog.Error(err, "unable to create controller", "controller", "Pool")
278279
os.Exit(1)

kubernetes/internal/controller/allocator.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,6 @@ func NewDefaultAllocator(client client.Client) Allocator {
214214

215215
func (allocator *defaultAllocator) Schedule(ctx context.Context, spec *AllocSpec) (*AllocStatus, []SandboxSyncInfo, bool, error) {
216216
log := logf.FromContext(ctx)
217-
log.Info("Schedule started", "pool", spec.Pool.Name, "totalPods", len(spec.Pods), "sandboxes", len(spec.Sandboxes))
218217
status, err := allocator.initAllocation(ctx, spec)
219218
if err != nil {
220219
return nil, nil, false, err
@@ -224,15 +223,14 @@ func (allocator *defaultAllocator) Schedule(ctx context.Context, spec *AllocSpec
224223
if _, ok := status.PodAllocation[pod.Name]; ok {
225224
continue
226225
}
227-
if pod.Status.Phase != corev1.PodRunning {
226+
if !canAllocate(pod) {
228227
continue
229228
}
230-
if isRestarting(pod) {
229+
if pod.Status.Phase != corev1.PodRunning {
231230
continue
232231
}
233232
availablePods = append(availablePods, pod.Name)
234233
}
235-
log.V(1).Info("Schedule init", "existingAllocations", len(status.PodAllocation), "availablePods", len(availablePods))
236234
sandboxToPods := make(map[string][]string)
237235
for podName, sandboxName := range status.PodAllocation {
238236
sandboxToPods[sandboxName] = append(sandboxToPods[sandboxName], podName)

kubernetes/internal/controller/allocator_test.go

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,73 @@ func TestAllocatorSchedule(t *testing.T) {
299299
PodsToRecycle: []string{"pod1"},
300300
},
301301
},
302+
{
303+
name: "pod with deallocated-from label is excluded",
304+
spec: &AllocSpec{
305+
Pods: []*corev1.Pod{
306+
{
307+
ObjectMeta: metav1.ObjectMeta{
308+
Name: "pod-normal",
309+
},
310+
Status: corev1.PodStatus{
311+
Phase: corev1.PodRunning,
312+
},
313+
},
314+
{
315+
ObjectMeta: metav1.ObjectMeta{
316+
Name: "pod-deallocated",
317+
Labels: map[string]string{
318+
"pool.opensandbox.io/deallocated-from": "bsx-uid-123",
319+
},
320+
},
321+
Status: corev1.PodStatus{
322+
Phase: corev1.PodRunning,
323+
},
324+
},
325+
},
326+
Pool: &sandboxv1alpha1.Pool{
327+
ObjectMeta: metav1.ObjectMeta{
328+
Name: "pool1",
329+
},
330+
},
331+
Sandboxes: []*sandboxv1alpha1.BatchSandbox{
332+
{
333+
ObjectMeta: metav1.ObjectMeta{
334+
Name: "sbx1",
335+
},
336+
Spec: sandboxv1alpha1.BatchSandboxSpec{
337+
PoolRef: "pool1",
338+
Replicas: &replica1,
339+
},
340+
},
341+
{
342+
ObjectMeta: metav1.ObjectMeta{
343+
Name: "sbx2",
344+
},
345+
Spec: sandboxv1alpha1.BatchSandboxSpec{
346+
PoolRef: "pool1",
347+
Replicas: &replica1,
348+
},
349+
},
350+
},
351+
},
352+
poolAlloc: &PoolAllocation{
353+
PodAllocation: map[string]string{},
354+
},
355+
sandboxAlloc: &SandboxAllocation{
356+
Pods: []string{},
357+
},
358+
release: &AllocationRelease{
359+
Pods: []string{},
360+
},
361+
wantStatus: &AllocStatus{
362+
PodAllocation: map[string]string{
363+
"pod-normal": "sbx1",
364+
},
365+
PodSupplement: 1, // sbx2 needs a pod but only normal pod available
366+
PodsToRecycle: []string{},
367+
},
368+
},
302369
}
303370
for _, c := range cases {
304371
t.Run(c.name, func(t *testing.T) {

kubernetes/internal/controller/apis.go

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,15 @@ const (
3636
AnnoPodRecycleMeta = "pool.opensandbox.io/recycle-meta"
3737

3838
FinalizerTaskCleanup = "batch-sandbox.sandbox.opensandbox.io/task-cleanup"
39+
FinalizerPoolRecycle = "batch-sandbox.sandbox.opensandbox.io/pool-recycle"
40+
41+
// Value is the BatchSandbox UID.
42+
LabelPodDeallocatedFrom = "pool.opensandbox.io/deallocated-from"
43+
// LabelPodRecycleConfirmed marks that Pool has confirmed recycling.
44+
// Value is the BatchSandbox UID from deallocated-from label.
45+
LabelPodRecycleConfirmed = "pool.opensandbox.io/recycle-confirmed"
46+
47+
LabelPodRecycleTimeoutSec = "pool.opensandbox.io/recycle-timeout-sec"
3948
)
4049

4150
// PodRecycleState defines the state of Pod recycle.
@@ -57,10 +66,6 @@ type PodRecycleMeta struct {
5766

5867
// TriggeredAt: Restart trigger timestamp (milliseconds)
5968
TriggeredAt int64 `json:"triggeredAt"`
60-
61-
// InitialRestartCounts: Restart counts of containers when restart was triggered.
62-
// Used to verify that containers have actually restarted in this cycle.
63-
InitialRestartCounts map[string]int32 `json:"initialRestartCounts,omitempty"`
6469
}
6570

6671
// parsePodRecycleMeta parses the recycle metadata from Pod annotations.
@@ -82,12 +87,38 @@ func setPodRecycleMeta(obj metav1.Object, meta *PodRecycleMeta) {
8287
obj.GetAnnotations()[AnnoPodRecycleMeta] = utils.DumpJSON(meta)
8388
}
8489

90+
// canAllocate checks if a pod is eligible for allocation.
91+
// A pod can be allocated if:
92+
// 1. No deallocated-from label (normal pod), OR
93+
// 2. Has recycle-confirmed label AND no recycle-meta annotation (recycling completed)
94+
func canAllocate(pod *corev1.Pod) bool {
95+
deallocatedFrom := pod.Labels[LabelPodDeallocatedFrom]
96+
if deallocatedFrom == "" {
97+
return true // Normal pod, no deallocation marker
98+
}
99+
100+
// Has deallocated-from, check if recycling is confirmed and completed
101+
recycleConfirmed := pod.Labels[LabelPodRecycleConfirmed]
102+
meta := pod.Annotations[AnnoPodRecycleMeta]
103+
104+
// Can allocate only if recycling is confirmed AND not in restarting state
105+
return recycleConfirmed != "" && meta == ""
106+
}
107+
85108
func isRestarting(pod *corev1.Pod) bool {
86-
meta, err := parsePodRecycleMeta(pod)
87-
if err != nil {
109+
// - recycle-confirmed is set when restart starts
110+
// - recycle-confirmed is KEPT as a receipt after restart completes
111+
// - recycle-meta is cleared when restart completes
112+
meta := pod.Annotations[AnnoPodRecycleMeta]
113+
if meta == "" {
114+
return false
115+
}
116+
// Parse to verify it's in Restarting state (not just stale data)
117+
var recycleMeta PodRecycleMeta
118+
if err := json.Unmarshal([]byte(meta), &recycleMeta); err != nil {
88119
return false
89120
}
90-
return meta.State == RecycleStateRestarting
121+
return recycleMeta.State == RecycleStateRestarting
91122
}
92123

93124
// AnnotationSandboxEndpoints Use the exported constant from pkg/utils
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
// Copyright 2025 Alibaba Group Holding Ltd.
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+
"testing"
19+
20+
"github.com/stretchr/testify/assert"
21+
corev1 "k8s.io/api/core/v1"
22+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
23+
)
24+
25+
func TestCanAllocate(t *testing.T) {
26+
tests := []struct {
27+
name string
28+
pod *corev1.Pod
29+
expected bool
30+
}{
31+
{
32+
name: "normal pod without labels",
33+
pod: &corev1.Pod{
34+
ObjectMeta: metav1.ObjectMeta{
35+
Name: "pod-normal",
36+
},
37+
},
38+
expected: true,
39+
},
40+
{
41+
name: "pod with deallocated-from but no recycle-confirmed",
42+
pod: &corev1.Pod{
43+
ObjectMeta: metav1.ObjectMeta{
44+
Name: "pod-deallocated",
45+
Labels: map[string]string{
46+
LabelPodDeallocatedFrom: "bsx-uid-123",
47+
},
48+
},
49+
},
50+
expected: false,
51+
},
52+
{
53+
name: "pod with deallocated-from and recycle-confirmed, no recycle-meta",
54+
pod: &corev1.Pod{
55+
ObjectMeta: metav1.ObjectMeta{
56+
Name: "pod-confirmed",
57+
Labels: map[string]string{
58+
LabelPodDeallocatedFrom: "bsx-uid-123",
59+
LabelPodRecycleConfirmed: "bsx-uid-123",
60+
},
61+
},
62+
},
63+
expected: true,
64+
},
65+
{
66+
name: "pod with deallocated-from and recycle-confirmed and recycle-meta (still restarting)",
67+
pod: &corev1.Pod{
68+
ObjectMeta: metav1.ObjectMeta{
69+
Name: "pod-restarting",
70+
Labels: map[string]string{
71+
LabelPodDeallocatedFrom: "bsx-uid-123",
72+
LabelPodRecycleConfirmed: "bsx-uid-123",
73+
},
74+
Annotations: map[string]string{
75+
AnnoPodRecycleMeta: `{"state":"Restarting","triggeredAt":1234567890}`,
76+
},
77+
},
78+
},
79+
expected: false,
80+
},
81+
{
82+
name: "pod with only recycle-confirmed (edge case)",
83+
pod: &corev1.Pod{
84+
ObjectMeta: metav1.ObjectMeta{
85+
Name: "pod-only-confirmed",
86+
Labels: map[string]string{
87+
LabelPodRecycleConfirmed: "bsx-uid-123",
88+
},
89+
},
90+
},
91+
expected: true, // No deallocated-from means normal pod
92+
},
93+
}
94+
95+
for _, tt := range tests {
96+
t.Run(tt.name, func(t *testing.T) {
97+
result := canAllocate(tt.pod)
98+
assert.Equal(t, tt.expected, result)
99+
})
100+
}
101+
}

0 commit comments

Comments
 (0)