diff --git a/pkg/yurthub/cachemanager/cache_manager.go b/pkg/yurthub/cachemanager/cache_manager.go index 633c7b499e9..cc2de399bfc 100644 --- a/pkg/yurthub/cachemanager/cache_manager.go +++ b/pkg/yurthub/cachemanager/cache_manager.go @@ -325,15 +325,20 @@ func (cm *cacheManager) prepareGvkForListObj(gvr schema.GroupVersionResource) (s } func completeListObjWithObjs(listObj runtime.Object, objs []runtime.Object) error { - listRv := 0 - rvStr := "" - rvInt := 0 + var listRv uint64 accessor := meta.NewAccessor() for i := range objs { - rvStr, _ = accessor.ResourceVersion(objs[i]) - rvInt, _ = strconv.Atoi(rvStr) - if rvInt > listRv { - listRv = rvInt + rvStr, err := accessor.ResourceVersion(objs[i]) + if err != nil || len(rvStr) == 0 { + continue + } + rvUint, err := strconv.ParseUint(rvStr, 10, 64) + if err != nil { + klog.Warningf("failed to parse resourceVersion %q: %v", rvStr, err) + continue + } + if rvUint > listRv { + listRv = rvUint } } @@ -341,7 +346,7 @@ func completeListObjWithObjs(listObj runtime.Object, objs []runtime.Object) erro return fmt.Errorf("could not meta set list with %d objects, %v", len(objs), err) } - return accessor.SetResourceVersion(listObj, strconv.Itoa(listRv)) + return accessor.SetResourceVersion(listObj, strconv.FormatUint(listRv, 10)) } func generateEmptyListObjOfGVK(listGvk schema.GroupVersionKind) (runtime.Object, error) { diff --git a/pkg/yurthub/cachemanager/cache_manager_test.go b/pkg/yurthub/cachemanager/cache_manager_test.go index ac46af22a1e..5d4937096b9 100644 --- a/pkg/yurthub/cachemanager/cache_manager_test.go +++ b/pkg/yurthub/cachemanager/cache_manager_test.go @@ -3455,3 +3455,60 @@ func TestIsListRequestWithNameFieldSelector(t *testing.T) { } // TODO: in-memory cache unit tests + +func Test_completeListObjWithObjs(t *testing.T) { + testcases := map[string]struct { + objs []runtime.Object + expectedRv string + }{ + "normal resourceVersion numbers": { + objs: []runtime.Object{ + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", ResourceVersion: "100"}}, + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod2", ResourceVersion: "250"}}, + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod3", ResourceVersion: "150"}}, + }, + expectedRv: "250", + }, + "large uint64 resourceVersion exceeding 32-bit signed int max": { + objs: []runtime.Object{ + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", ResourceVersion: "2147483647"}}, + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod2", ResourceVersion: "3000000000"}}, + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod3", ResourceVersion: "2500000000"}}, + }, + expectedRv: "3000000000", + }, + "very large 64-bit unsigned int resourceVersion": { + objs: []runtime.Object{ + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", ResourceVersion: "18446744073709551600"}}, + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod2", ResourceVersion: "100"}}, + }, + expectedRv: "18446744073709551600", + }, + "empty and invalid resourceVersions handled safely": { + objs: []runtime.Object{ + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", ResourceVersion: ""}}, + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod2", ResourceVersion: "invalid"}}, + &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod3", ResourceVersion: "500"}}, + }, + expectedRv: "500", + }, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + listObj := &v1.PodList{} + err := completeListObjWithObjs(listObj, tc.objs) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + accessor := meta.NewAccessor() + rv, err := accessor.ResourceVersion(listObj) + if err != nil { + t.Fatalf("failed to get list object resourceVersion: %v", err) + } + if rv != tc.expectedRv { + t.Errorf("expected list resourceVersion %q, got %q", tc.expectedRv, rv) + } + }) + } +} diff --git a/pkg/yurthub/gc/gc.go b/pkg/yurthub/gc/gc.go index a5d8a9ce373..915e5eb25e2 100644 --- a/pkg/yurthub/gc/gc.go +++ b/pkg/yurthub/gc/gc.go @@ -141,6 +141,7 @@ func (m *GCManager) gcPodsWhenRestart() { Namespace: ns, Name: name, Resources: "pods", + Version: "v1", }) if err != nil { klog.Errorf("could not get pod key for %s/%s, %v", ns, name, err) diff --git a/pkg/yurthub/gc/gc_test.go b/pkg/yurthub/gc/gc_test.go new file mode 100644 index 00000000000..e8c3e27a557 --- /dev/null +++ b/pkg/yurthub/gc/gc_test.go @@ -0,0 +1,161 @@ +/* +Copyright 2025 The OpenYurt Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package gc + +import ( + "path/filepath" + "testing" + + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/openyurtio/openyurt/pkg/yurthub/storage" + diskstorage "github.com/openyurtio/openyurt/pkg/yurthub/storage/disk" +) + +// TestGcPodKeyConsistency proves that the keys built in gcPodsWhenRestart +// (when constructing currentPodKeys from apiserver response) must include +// Version and Group to match the keys returned by ListResourceKeysOfComponent. +// +// Before the fix, gcPodsWhenRestart called KeyFunc without Version/Group, +// producing "pods..core" in enhancement mode instead of "pods.v1.core", +// causing all cached pod keys to appear deleted. +func TestGcPodKeyConsistency(t *testing.T) { + // Create a disk storage in enhancement mode by ensuring no legacy + // resource dirs exist. A fresh temp dir produces enhancement mode. + tmpDir := t.TempDir() + store, err := diskstorage.NewDiskStorage(filepath.Join(tmpDir, "cache")) + if err != nil { + t.Fatalf("failed to create disk storage: %v", err) + } + + // Simulate what ListResourceKeysOfComponent does: + // Build a key with full GVR info (like gc.go:98-102) + listKey, err := store.KeyFunc(storage.KeyBuildInfo{ + Component: "kubelet", + Resources: "pods", + Group: "", + Version: "v1", + Namespace: "default", + Name: "test-pod", + }) + if err != nil { + t.Fatalf("failed to build list key: %v", err) + } + + // Simulate the BUGGY code from gc.go:139-144 (without Version/Group) + buggyKey, err := store.KeyFunc(storage.KeyBuildInfo{ + Component: "kubelet", + Namespace: "default", + Name: "test-pod", + Resources: "pods", + // Version and Group intentionally omitted — this is the bug + }) + if err != nil { + t.Fatalf("failed to build buggy key: %v", err) + } + + // Simulate the FIXED code (with Version and Group) + fixedKey, err := store.KeyFunc(storage.KeyBuildInfo{ + Component: "kubelet", + Namespace: "default", + Name: "test-pod", + Resources: "pods", + Version: "v1", + }) + if err != nil { + t.Fatalf("failed to build fixed key: %v", err) + } + + t.Logf("List key (from ListResourceKeysOfComponent): %q", listKey.Key()) + t.Logf("Buggy key (no Version/Group): %q", buggyKey.Key()) + t.Logf("Fixed key (with Version): %q", fixedKey.Key()) + + // The buggy key should NOT match the list key (proving the bug exists) + if buggyKey.Key() == listKey.Key() { + t.Logf("Keys match even without Version — storage may be in legacy mode, bug is not applicable in this mode") + } else { + t.Logf("CONFIRMED: Buggy key %q != list key %q — pod GC key mismatch exists in enhancement mode", buggyKey.Key(), listKey.Key()) + } + + // The fixed key MUST match the list key + if fixedKey.Key() != listKey.Key() { + t.Errorf("Fixed key %q does not match list key %q — fix is incorrect", fixedKey.Key(), listKey.Key()) + } else { + t.Logf("VERIFIED: Fixed key matches list key — fix is correct") + } +} + +// TestGcPodKeyMapLookup simulates the actual map lookup that gcPodsWhenRestart +// performs to determine which pods should be garbage collected. +func TestGcPodKeyMapLookup(t *testing.T) { + tmpDir := t.TempDir() + store, err := diskstorage.NewDiskStorage(filepath.Join(tmpDir, "cache")) + if err != nil { + t.Fatalf("failed to create disk storage: %v", err) + } + + gvr := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"} + + // Create a cached pod entry (simulating what ListResourceKeysOfComponent returns) + cachedKey, err := store.KeyFunc(storage.KeyBuildInfo{ + Component: "kubelet", + Resources: gvr.Resource, + Group: gvr.Group, + Version: gvr.Version, + Namespace: "kube-system", + Name: "coredns-abc123", + }) + if err != nil { + t.Fatalf("failed to build cached key: %v", err) + } + + // Build the "current pods" map the way gcPodsWhenRestart does it + // BUGGY: no Version/Group + currentPodKeysBuggy := make(map[storage.Key]struct{}) + buggyKey, _ := store.KeyFunc(storage.KeyBuildInfo{ + Component: "kubelet", + Namespace: "kube-system", + Name: "coredns-abc123", + Resources: "pods", + }) + currentPodKeysBuggy[buggyKey] = struct{}{} + + // FIXED: with Version + currentPodKeysFixed := make(map[storage.Key]struct{}) + fixedKey, _ := store.KeyFunc(storage.KeyBuildInfo{ + Component: "kubelet", + Namespace: "kube-system", + Name: "coredns-abc123", + Resources: "pods", + Version: "v1", + }) + currentPodKeysFixed[fixedKey] = struct{}{} + + // Check buggy lookup — this should MISS (the bug) + if _, found := currentPodKeysBuggy[cachedKey]; found { + t.Logf("Buggy code found the cached key (storage in legacy mode)") + } else { + t.Logf("CONFIRMED BUG: Buggy code did NOT find cached key %q in currentPodKeys — pod would be incorrectly marked for GC", cachedKey.Key()) + } + + // Check fixed lookup — this MUST find the key + if _, found := currentPodKeysFixed[cachedKey]; !found { + t.Errorf("Fixed code did NOT find cached key %q in currentPodKeys — fix is broken", cachedKey.Key()) + } else { + t.Logf("VERIFIED FIX: Fixed code correctly found cached key in currentPodKeys") + } +} diff --git a/pkg/yurtmanager/controller/hubleader/hubleader_controller.go b/pkg/yurtmanager/controller/hubleader/hubleader_controller.go index c387937ac64..810c46aadce 100644 --- a/pkg/yurtmanager/controller/hubleader/hubleader_controller.go +++ b/pkg/yurtmanager/controller/hubleader/hubleader_controller.go @@ -176,8 +176,10 @@ func (r *ReconcileHubLeader) reconcileHubLeader(ctx context.Context, nodepool *a // Set match labels matchLabels := make(map[string]string) if nodepool.Spec.LeaderElectionStrategy == string(appsv1beta2.ElectionStrategyMark) { - // Add mark strategy match labels - matchLabels = nodepool.Spec.LeaderNodeLabelSelector + // Add mark strategy match labels safely without overwriting matchLabels with a potential nil map + for k, v := range nodepool.Spec.LeaderNodeLabelSelector { + matchLabels[k] = v + } } matchLabels[projectinfo.GetNodePoolLabel()] = nodepool.GetName() diff --git a/pkg/yurtmanager/controller/hubleader/hubleader_controller_test.go b/pkg/yurtmanager/controller/hubleader/hubleader_controller_test.go index b347b967c79..db8e0ad0ffe 100644 --- a/pkg/yurtmanager/controller/hubleader/hubleader_controller_test.go +++ b/pkg/yurtmanager/controller/hubleader/hubleader_controller_test.go @@ -328,6 +328,48 @@ func TestReconcile(t *testing.T) { }, expectErr: false, }, + "mark election strategy with nil LeaderNodeLabelSelector": { + pool: &appsv1beta2.NodePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hangzhou", + }, + Spec: appsv1beta2.NodePoolSpec{ + Type: appsv1beta2.Edge, + Labels: map[string]string{ + "region": "hangzhou", + }, + LeaderElectionStrategy: string(appsv1beta2.ElectionStrategyMark), + LeaderNodeLabelSelector: nil, + LeaderReplicas: 1, + EnableLeaderElection: true, + }, + }, + expectedNodePool: &appsv1beta2.NodePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hangzhou", + }, + Spec: appsv1beta2.NodePoolSpec{ + Type: appsv1beta2.Edge, + Labels: map[string]string{ + "region": "hangzhou", + }, + LeaderElectionStrategy: string(appsv1beta2.ElectionStrategyMark), + LeaderNodeLabelSelector: nil, + EnableLeaderElection: true, + LeaderReplicas: 1, + }, + Status: appsv1beta2.NodePoolStatus{ + LeaderEndpoints: []appsv1beta2.Leader{ + { + NodeName: "ready with internal IP", + Address: "10.0.0.1", + }, + }, + LeaderNum: 1, + }, + }, + expectErr: false, + }, "no potential leaders in hangzhou with mark strategy": { pool: &appsv1beta2.NodePool{ ObjectMeta: metav1.ObjectMeta{