Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add some retries when getObjectOf pod's owner #1807

Merged
merged 1 commit into from
Oct 31, 2024
Merged

add some retries when getObjectOf pod's owner #1807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions pkg/util/workloadspread/workloadspread.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,16 @@ func (h *Handler) HandlePodCreation(pod *corev1.Pod) (skip bool, err error) {
continue
}
// determine if the reference of workloadSpread and pod is equal
if h.isReferenceEqual(ws.Spec.TargetReference, ref, pod.Namespace) {
if ok, err := h.isReferenceEqual(ws.Spec.TargetReference, ref, pod.Namespace); ok {
matchedWS = &ws
// pod has at most one matched workloadSpread
break
} else if err != nil {
klog.ErrorS(err, "failed to determine whether workloadspread refers pod's owner",
"pod", klog.KObj(pod), "workloadspread", klog.KObj(&ws))
if errors.IsNotFound(err) {
return true, err
}
}
}
// not found matched workloadSpread
Expand Down Expand Up @@ -687,35 +693,34 @@ func (h *Handler) getSuitableSubset(subsetStatuses []appsv1alpha1.WorkloadSpread
return nil
}

func (h *Handler) isReferenceEqual(target *appsv1alpha1.TargetReference, owner *metav1.OwnerReference, namespace string) bool {
func (h *Handler) isReferenceEqual(target *appsv1alpha1.TargetReference, owner *metav1.OwnerReference, namespace string) (bool, error) {
if owner == nil {
return false
return false, nil
}

targetGv, err := schema.ParseGroupVersion(target.APIVersion)
if err != nil {
klog.ErrorS(err, "parse TargetReference apiVersion failed", "apiVersion", target.APIVersion)
return false
return false, err
}

ownerGv, err := schema.ParseGroupVersion(owner.APIVersion)
if err != nil {
klog.ErrorS(err, "parse OwnerReference apiVersion failed", "apiVersion", owner.APIVersion)
return false
return false, err
}

if targetGv.Group == ownerGv.Group && target.Kind == owner.Kind && target.Name == owner.Name {
return true
return true, nil
}

if match, err := matchReference(owner); err != nil || !match {
return false
return false, err
}

ownerObject, err := h.getObjectOf(owner, namespace)
if err != nil {
klog.ErrorS(err, "Failed to get owner object", "owner", owner)
return false
return false, err
}

return h.isReferenceEqual(target, metav1.GetControllerOfNoCopy(ownerObject), namespace)
Expand Down
107 changes: 98 additions & 9 deletions pkg/util/workloadspread/workloadspread_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,13 @@ import (
"fmt"
"reflect"
"strconv"
"strings"
"testing"
"time"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand All @@ -53,6 +55,13 @@ import (
var (
scheme *runtime.Scheme

cloneSetDemo = &appsv1alpha1.CloneSet{
ObjectMeta: metav1.ObjectMeta{
Name: "cloneset-test",
Namespace: "default",
},
}

podDemo = &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "test-pod-1",
Expand Down Expand Up @@ -472,10 +481,14 @@ func TestWorkloadSpreadCreatePodWithoutFullName(t *testing.T) {
}

func TestWorkloadSpreadMutatingPod(t *testing.T) {
defaultErrorHandler := func(err error) bool {
return err == nil
}
cases := []struct {
name string
getPod func() *corev1.Pod
getWorkloadSpread func() *appsv1alpha1.WorkloadSpread
errorHandler func(err error) bool // errorHandler returns true means the error is expected
getOperation func() Operation
expectPod func() *corev1.Pod
expectWorkloadSpread func() *appsv1alpha1.WorkloadSpread
Expand Down Expand Up @@ -1080,6 +1093,31 @@ func TestWorkloadSpreadMutatingPod(t *testing.T) {
return workloadSpread
},
},
{
name: "operation = create, pod owner reference replicaset not found",
getPod: func() *corev1.Pod {
pod := podDemo.DeepCopy()
pod.OwnerReferences[0].Name = "not-exist"
return pod
},
getWorkloadSpread: func() *appsv1alpha1.WorkloadSpread {
return workloadSpreadDemo.DeepCopy()
},
getOperation: func() Operation {
return CreateOperation
},
expectPod: func() *corev1.Pod {
pod := podDemo.DeepCopy()
pod.OwnerReferences[0].Name = "not-exist"
return pod
},
expectWorkloadSpread: func() *appsv1alpha1.WorkloadSpread {
return workloadSpreadDemo.DeepCopy()
},
errorHandler: func(err error) bool {
return errors.IsNotFound(err)
},
},
}
for _, cs := range cases {
t.Run(cs.name, func(t *testing.T) {
Expand All @@ -1088,7 +1126,7 @@ func TestWorkloadSpreadMutatingPod(t *testing.T) {
expectWS := cs.expectWorkloadSpread()
podExpect := cs.expectPod()
fakeClient := fake.NewClientBuilder().WithScheme(scheme).
WithObjects(workloadSpreadIn).WithStatusSubresource(&appsv1alpha1.WorkloadSpread{}).Build()
WithObjects(workloadSpreadIn, cloneSetDemo).WithStatusSubresource(&appsv1alpha1.WorkloadSpread{}).Build()
handler := NewWorkloadSpreadHandler(fakeClient)

var err error
Expand All @@ -1100,8 +1138,12 @@ func TestWorkloadSpreadMutatingPod(t *testing.T) {
case EvictionOperation:
err = handler.HandlePodDeletion(podIn, EvictionOperation)
}
if err != nil {
t.Fatalf("WorkloadSpreadMutatingPod failed: %s", err.Error())
errHandler := cs.errorHandler
if errHandler == nil {
errHandler = defaultErrorHandler
}
if !errHandler(err) {
t.Fatalf("WorkloadSpreadMutatingPod errHandler failed: %v", err)
}
podInBy, _ := json.Marshal(podIn)
expectPodBy, _ := json.Marshal(podExpect)
Expand Down Expand Up @@ -1178,10 +1220,11 @@ func compareTimeMap(actual, expect map[string]metav1.Time) bool {

func TestIsReferenceEqual(t *testing.T) {
cases := []struct {
name string
getTargetRef func() *appsv1alpha1.TargetReference
getOwnerRef func() *metav1.OwnerReference
expectEqual bool
name string
getTargetRef func() *appsv1alpha1.TargetReference
getOwnerRef func() *metav1.OwnerReference
expectEqual bool
errorExpected func(err error) bool
}{
{
name: "ApiVersion, Kind, Name equals",
Expand Down Expand Up @@ -1255,14 +1298,60 @@ func TestIsReferenceEqual(t *testing.T) {
},
expectEqual: false,
},
{
name: "target ApiVersion parse failed",
getTargetRef: func() *appsv1alpha1.TargetReference {
return &appsv1alpha1.TargetReference{
APIVersion: "apps.kruise.io/v1alpha1/yahaha",
Kind: "CloneSet",
Name: "test-1",
}
},
getOwnerRef: func() *metav1.OwnerReference {
return &metav1.OwnerReference{
APIVersion: "apps.kruise.io/v1alpha1",
Kind: "CloneSet",
Name: "test-2",
}
},
expectEqual: false,
errorExpected: func(err error) bool {
return strings.Contains(err.Error(), "unexpected GroupVersion string: apps.kruise.io/v1alpha1/yahaha")
},
},
{
name: "owner ApiVersion parse failed",
getTargetRef: func() *appsv1alpha1.TargetReference {
return &appsv1alpha1.TargetReference{
APIVersion: "apps.kruise.io/v1alpha1",
Kind: "CloneSet",
Name: "test-1",
}
},
getOwnerRef: func() *metav1.OwnerReference {
return &metav1.OwnerReference{
APIVersion: "apps.kruise.io/v1alpha1/yahaha",
Kind: "CloneSet",
Name: "test-2",
}
},
expectEqual: false,
errorExpected: func(err error) bool {
return strings.Contains(err.Error(), "unexpected GroupVersion string: apps.kruise.io/v1alpha1/yahaha")
},
},
}

for _, cs := range cases {
t.Run(cs.name, func(t *testing.T) {
h := Handler{fake.NewClientBuilder().Build()}
if h.isReferenceEqual(cs.getTargetRef(), cs.getOwnerRef(), "") != cs.expectEqual {
ok, err := h.isReferenceEqual(cs.getTargetRef(), cs.getOwnerRef(), "")
if ok != cs.expectEqual {
t.Fatalf("isReferenceEqual failed")
}
if cs.errorExpected != nil && !cs.errorExpected(err) {
t.Fatalf("isReferenceEqual failed with error: %v", err)
}
})
}
}
Expand Down Expand Up @@ -1400,7 +1489,7 @@ func TestIsReferenceEqual2(t *testing.T) {
handler := &Handler{Client: cli}
workloadsInWhiteListInitialized = false
initializeWorkloadsInWhiteList(cli)
result := handler.isReferenceEqual(&ref, metav1.GetControllerOf(pod), pod.GetNamespace())
result, _ := handler.isReferenceEqual(&ref, metav1.GetControllerOf(pod), pod.GetNamespace())
if result != cs.Expect {
t.Fatalf("got unexpected result")
}
Expand Down
Loading
Loading