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

VPA: Create event for VPA object when Pod is evicted #7413

Merged
merged 5 commits into from
Nov 6, 2024
Merged

VPA: Create event for VPA object when Pod is evicted #7413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ced9d5
VPA: Create event for VPA object when Pod is evicted
adrianmoisey Oct 27, 2024
2896a69
Catch error when adding scheme
adrianmoisey Oct 23, 2024
953c8d0
Tidy up test a bit and add a context
adrianmoisey Oct 23, 2024
32f7183
DRY up VPA a little
adrianmoisey Oct 23, 2024
8137019
DRY up getBasicVpa() a bit more
adrianmoisey Oct 23, 2024
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
7 changes: 5 additions & 2 deletions vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const (
type PodsEvictionRestriction interface {
// Evict sends eviction instruction to the api client.
// Returns error if pod cannot be evicted or if client returned error.
Evict(pod *apiv1.Pod, eventRecorder record.EventRecorder) error
Evict(pod *apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler, eventRecorder record.EventRecorder) error
// CanEvict checks if pod can be safely evicted
CanEvict(pod *apiv1.Pod) bool
}
Expand Down Expand Up @@ -122,7 +122,7 @@ func (e *podsEvictionRestrictionImpl) CanEvict(pod *apiv1.Pod) bool {

// Evict sends eviction instruction to api client. Returns error if pod cannot be evicted or if client returned error
// Does not check if pod was actually evicted after eviction grace period.
func (e *podsEvictionRestrictionImpl) Evict(podToEvict *apiv1.Pod, eventRecorder record.EventRecorder) error {
func (e *podsEvictionRestrictionImpl) Evict(podToEvict *apiv1.Pod, vpa *vpa_types.VerticalPodAutoscaler, eventRecorder record.EventRecorder) error {
cr, present := e.podToReplicaCreatorMap[getPodID(podToEvict)]
if !present {
return fmt.Errorf("pod not suitable for eviction %s/%s: not in replicated pods map", podToEvict.Namespace, podToEvict.Name)
Expand All @@ -146,6 +146,9 @@ func (e *podsEvictionRestrictionImpl) Evict(podToEvict *apiv1.Pod, eventRecorder
eventRecorder.Event(podToEvict, apiv1.EventTypeNormal, "EvictedByVPA",
"Pod was evicted by VPA Updater to apply resource recommendation.")

eventRecorder.Event(vpa, apiv1.EventTypeNormal, "EvictedPod",
"VPA Updater evicted Pod "+podToEvict.Name+" to apply resource recommendation.")

if podToEvict.Status.Phase != apiv1.PodPending {
singleGroupStats, present := e.creatorToSingleGroupStatsMap[cr]
if !present {
Expand Down
141 changes: 119 additions & 22 deletions vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"

appsv1 "k8s.io/api/apps/v1"
batchv1 "k8s.io/api/batch/v1"
apiv1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -281,15 +283,14 @@ func TestEvictReplicatedByController(t *testing.T) {
assert.Equalf(t, p.canEvict, eviction.CanEvict(p.pod), "TC %v - unexpected CanEvict result for pod-%v %#v", testCase.name, i, p.pod)
}
for i, p := range testCase.pods {
err := eviction.Evict(p.pod, test.FakeEventRecorder())
err := eviction.Evict(p.pod, testCase.vpa, test.FakeEventRecorder())
if p.evictionSuccess {
assert.NoErrorf(t, err, "TC %v - unexpected Evict result for pod-%v %#v", testCase.name, i, p.pod)
} else {
assert.Errorf(t, err, "TC %v - unexpected Evict result for pod-%v %#v", testCase.name, i, p.pod)
}
}
}

}

func TestEvictReplicatedByReplicaSet(t *testing.T) {
Expand All @@ -314,19 +315,20 @@ func TestEvictReplicatedByReplicaSet(t *testing.T) {
pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rs.ObjectMeta, &rs.TypeMeta).Get()
}

basicVpa := getBasicVpa()
factory, _ := getEvictionRestrictionFactory(nil, &rs, nil, nil, 2, 0.5)
eviction := factory.NewPodsEvictionRestriction(pods, getBasicVpa())
eviction := factory.NewPodsEvictionRestriction(pods, basicVpa)

for _, pod := range pods {
assert.True(t, eviction.CanEvict(pod))
}

for _, pod := range pods[:2] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Nil(t, err, "Should evict with no error")
}
for _, pod := range pods[2:] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Error(t, err, "Error expected")
}
}
Expand All @@ -353,19 +355,20 @@ func TestEvictReplicatedByStatefulSet(t *testing.T) {
pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&ss.ObjectMeta, &ss.TypeMeta).Get()
}

basicVpa := getBasicVpa()
factory, _ := getEvictionRestrictionFactory(nil, nil, &ss, nil, 2, 0.5)
eviction := factory.NewPodsEvictionRestriction(pods, getBasicVpa())
eviction := factory.NewPodsEvictionRestriction(pods, basicVpa)

for _, pod := range pods {
assert.True(t, eviction.CanEvict(pod))
}

adrianmoisey marked this conversation as resolved.
Show resolved Hide resolved
for _, pod := range pods[:2] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Nil(t, err, "Should evict with no error")
}
for _, pod := range pods[2:] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Error(t, err, "Error expected")
}
}
Expand All @@ -390,19 +393,21 @@ func TestEvictReplicatedByDaemonSet(t *testing.T) {
for i := range pods {
pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&ds.ObjectMeta, &ds.TypeMeta).Get()
}

basicVpa := getBasicVpa()
factory, _ := getEvictionRestrictionFactory(nil, nil, nil, &ds, 2, 0.5)
eviction := factory.NewPodsEvictionRestriction(pods, getBasicVpa())
eviction := factory.NewPodsEvictionRestriction(pods, basicVpa)

for _, pod := range pods {
assert.True(t, eviction.CanEvict(pod))
}

for _, pod := range pods[:2] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Nil(t, err, "Should evict with no error")
}
for _, pod := range pods[2:] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Error(t, err, "Error expected")
}
}
Expand All @@ -425,19 +430,20 @@ func TestEvictReplicatedByJob(t *testing.T) {
pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&job.ObjectMeta, &job.TypeMeta).Get()
}

basicVpa := getBasicVpa()
factory, _ := getEvictionRestrictionFactory(nil, nil, nil, nil, 2, 0.5)
eviction := factory.NewPodsEvictionRestriction(pods, getBasicVpa())
eviction := factory.NewPodsEvictionRestriction(pods, basicVpa)

for _, pod := range pods {
assert.True(t, eviction.CanEvict(pod))
}

for _, pod := range pods[:2] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Nil(t, err, "Should evict with no error")
}
for _, pod := range pods[2:] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Error(t, err, "Error expected")
}
}
Expand All @@ -464,15 +470,16 @@ func TestEvictTooFewReplicas(t *testing.T) {
pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get()
}

basicVpa := getBasicVpa()
factory, _ := getEvictionRestrictionFactory(&rc, nil, nil, nil, 10, 0.5)
eviction := factory.NewPodsEvictionRestriction(pods, getBasicVpa())
eviction := factory.NewPodsEvictionRestriction(pods, basicVpa)

for _, pod := range pods {
assert.False(t, eviction.CanEvict(pod))
}

for _, pod := range pods {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Error(t, err, "Error expected")
}
}
Expand Down Expand Up @@ -500,19 +507,20 @@ func TestEvictionTolerance(t *testing.T) {
pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get()
}

basicVpa := getBasicVpa()
factory, _ := getEvictionRestrictionFactory(&rc, nil, nil, nil, 2 /*minReplicas*/, tolerance)
eviction := factory.NewPodsEvictionRestriction(pods, getBasicVpa())
eviction := factory.NewPodsEvictionRestriction(pods, basicVpa)

for _, pod := range pods {
assert.True(t, eviction.CanEvict(pod))
}

for _, pod := range pods[:4] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Nil(t, err, "Should evict with no error")
}
for _, pod := range pods[4:] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Error(t, err, "Error expected")
}
}
Expand Down Expand Up @@ -540,23 +548,112 @@ func TestEvictAtLeastOne(t *testing.T) {
pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta).Get()
}

basicVpa := getBasicVpa()
factory, _ := getEvictionRestrictionFactory(&rc, nil, nil, nil, 2, tolerance)
eviction := factory.NewPodsEvictionRestriction(pods, getBasicVpa())
eviction := factory.NewPodsEvictionRestriction(pods, basicVpa)

for _, pod := range pods {
assert.True(t, eviction.CanEvict(pod))
}

for _, pod := range pods[:1] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Nil(t, err, "Should evict with no error")
}
for _, pod := range pods[1:] {
err := eviction.Evict(pod, test.FakeEventRecorder())
err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
assert.Error(t, err, "Error expected")
}
}

func TestEvictEmitEvent(t *testing.T) {
rc := apiv1.ReplicationController{
ObjectMeta: metav1.ObjectMeta{
Name: "rc",
Namespace: "default",
},
TypeMeta: metav1.TypeMeta{
Kind: "ReplicationController",
},
}

index := 0
generatePod := func() test.PodBuilder {
index++
return test.Pod().WithName(fmt.Sprintf("test-%v", index)).WithCreator(&rc.ObjectMeta, &rc.TypeMeta)
}

basicVpa := getBasicVpa()

testCases := []struct {
name string
replicas int32
evictionTolerance float64
vpa *vpa_types.VerticalPodAutoscaler
pods []podWithExpectations
}{
{
name: "Pods that can be evicted",
replicas: 4,
evictionTolerance: 0.5,
vpa: basicVpa,
pods: []podWithExpectations{
{
pod: generatePod().WithPhase(apiv1.PodPending).Get(),
canEvict: true,
evictionSuccess: true,
},
{
pod: generatePod().WithPhase(apiv1.PodPending).Get(),
canEvict: true,
evictionSuccess: true,
},
},
},
{
name: "Pod that can not be evicted",
replicas: 4,
evictionTolerance: 0.5,
vpa: basicVpa,
pods: []podWithExpectations{

{
pod: generatePod().Get(),
canEvict: false,
evictionSuccess: false,
},
},
},
}

for _, testCase := range testCases {
rc.Spec = apiv1.ReplicationControllerSpec{
Replicas: &testCase.replicas,
}
pods := make([]*apiv1.Pod, 0, len(testCase.pods))
for _, p := range testCase.pods {
pods = append(pods, p.pod)
}
factory, _ := getEvictionRestrictionFactory(&rc, nil, nil, nil, 2, testCase.evictionTolerance)
eviction := factory.NewPodsEvictionRestriction(pods, testCase.vpa)

for _, p := range testCase.pods {
mockRecorder := test.MockEventRecorder()
mockRecorder.On("Event", mock.Anything, apiv1.EventTypeNormal, "EvictedByVPA", mock.Anything).Return()
mockRecorder.On("Event", mock.Anything, apiv1.EventTypeNormal, "EvictedPod", mock.Anything).Return()

eviction.Evict(p.pod, testCase.vpa, mockRecorder)

if p.canEvict {
mockRecorder.AssertNumberOfCalls(t, "Event", 2)

} else {
mockRecorder.AssertNumberOfCalls(t, "Event", 0)
}
}
}
}

func getEvictionRestrictionFactory(rc *apiv1.ReplicationController, rs *appsv1.ReplicaSet,
ss *appsv1.StatefulSet, ds *appsv1.DaemonSet, minReplicas int,
evictionToleranceFraction float64) (PodsEvictionRestrictionFactory, error) {
Expand Down
16 changes: 13 additions & 3 deletions vertical-pod-autoscaler/pkg/updater/logic/updater.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ import (
"k8s.io/apimachinery/pkg/labels"
kube_client "k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/fake"
"k8s.io/client-go/kubernetes/scheme"

corescheme "k8s.io/client-go/kubernetes/scheme"
clientv1 "k8s.io/client-go/kubernetes/typed/core/v1"
v1lister "k8s.io/client-go/listers/core/v1"
"k8s.io/client-go/tools/cache"
Expand All @@ -38,6 +39,7 @@ import (

vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
vpa_clientset "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned/scheme"
vpa_lister "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/listers/autoscaling.k8s.io/v1"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target"
controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher"
Expand Down Expand Up @@ -225,7 +227,7 @@ func (u *updater) RunOnce(ctx context.Context) {
return
}
klog.V(2).InfoS("Evicting pod", "pod", klog.KObj(pod))
evictErr := evictionLimiter.Evict(pod, u.eventRecorder)
evictErr := evictionLimiter.Evict(pod, vpa, u.eventRecorder)
if evictErr != nil {
klog.V(0).InfoS("Eviction failed", "error", evictErr, "pod", klog.KObj(pod))
} else {
Expand Down Expand Up @@ -310,6 +312,14 @@ func newEventRecorder(kubeClient kube_client.Interface) record.EventRecorder {
eventBroadcaster.StartLogging(klog.V(4).InfoS)
if _, isFake := kubeClient.(*fake.Clientset); !isFake {
eventBroadcaster.StartRecordingToSink(&clientv1.EventSinkImpl{Interface: clientv1.New(kubeClient.CoreV1().RESTClient()).Events("")})
} else {
eventBroadcaster.StartRecordingToSink(&clientv1.EventSinkImpl{Interface: kubeClient.CoreV1().Events("")})
}
return eventBroadcaster.NewRecorder(scheme.Scheme, apiv1.EventSource{Component: "vpa-updater"})

vpascheme := scheme.Scheme
if err := corescheme.AddToScheme(vpascheme); err != nil {
klog.Fatalf("Error adding core scheme: %v", err)
}

return eventBroadcaster.NewRecorder(vpascheme, apiv1.EventSource{Component: "vpa-updater"})
}
Loading
Loading