Skip to content

Commit

Permalink
Rebased fix
Browse files Browse the repository at this point in the history
  • Loading branch information
adrianmoisey committed Jun 16, 2024
1 parent 131d12b commit 0423980
Show file tree
Hide file tree
Showing 10 changed files with 172 additions and 13 deletions.
8 changes: 7 additions & 1 deletion vertical-pod-autoscaler/pkg/admission-controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ import (
"time"

apiv1 "k8s.io/api/core/v1"
cacheddiscovery "k8s.io/client-go/discovery/cached/memory"
"k8s.io/client-go/informers"
kube_client "k8s.io/client-go/kubernetes"
"k8s.io/client-go/restmapper"
kube_flag "k8s.io/component-base/cli/flag"
"k8s.io/klog/v2"

Expand Down Expand Up @@ -95,6 +97,10 @@ func main() {
factory := informers.NewSharedInformerFactory(kubeClient, defaultResyncPeriod)
targetSelectorFetcher := target.NewVpaTargetSelectorFetcher(config, kubeClient, factory)
controllerFetcher := controllerfetcher.NewControllerFetcher(config, kubeClient, factory, scaleCacheEntryFreshnessTime, scaleCacheEntryLifetime, scaleCacheEntryJitterFactor)

discoveryClient := cacheddiscovery.NewMemCacheClient(kubeClient)
mapper := restmapper.NewDeferredDiscoveryRESTMapper(discoveryClient)

podPreprocessor := pod.NewDefaultPreProcessor()
vpaPreprocessor := vpa.NewDefaultPreProcessor()
var limitRangeCalculator limitrange.LimitRangeCalculator
Expand All @@ -104,7 +110,7 @@ func main() {
limitRangeCalculator = limitrange.NewNoopLimitsCalculator()
}
recommendationProvider := recommendation.NewProvider(limitRangeCalculator, vpa_api_util.NewCappingRecommendationProcessor(limitRangeCalculator))
vpaMatcher := vpa.NewMatcher(vpaLister, targetSelectorFetcher, controllerFetcher)
vpaMatcher := vpa.NewMatcher(vpaLister, targetSelectorFetcher, controllerFetcher, mapper)

hostname, err := os.Hostname()
if err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package vpa

import (
core "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/klog/v2"

Expand All @@ -38,15 +39,18 @@ type matcher struct {
vpaLister vpa_lister.VerticalPodAutoscalerLister
selectorFetcher target.VpaTargetSelectorFetcher
controllerFetcher controllerfetcher.ControllerFetcher
restMapper meta.RESTMapper
}

// NewMatcher returns a new VPA matcher.
func NewMatcher(vpaLister vpa_lister.VerticalPodAutoscalerLister,
selectorFetcher target.VpaTargetSelectorFetcher,
controllerFetcher controllerfetcher.ControllerFetcher) Matcher {
controllerFetcher controllerfetcher.ControllerFetcher,
restMapper meta.RESTMapper) Matcher {
return &matcher{vpaLister: vpaLister,
selectorFetcher: selectorFetcher,
controllerFetcher: controllerFetcher}
controllerFetcher: controllerFetcher,
restMapper: restMapper}
}

func (m *matcher) GetMatchingVPA(pod *core.Pod) *vpa_types.VerticalPodAutoscaler {
Expand All @@ -71,7 +75,7 @@ func (m *matcher) GetMatchingVPA(pod *core.Pod) *vpa_types.VerticalPodAutoscaler
})
}
klog.V(2).Infof("Let's choose from %d configs for pod %s/%s", len(onConfigs), pod.Namespace, pod.Name)
result := vpa_api_util.GetControllingVPAForPod(pod, onConfigs, m.controllerFetcher)
result := vpa_api_util.GetControllingVPAForPod(pod, onConfigs, m.controllerFetcher, m.restMapper)
if result != nil {
return result.Vpa
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
core "k8s.io/api/core/v1"
meta "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/client-go/restmapper"

vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher"
Expand Down Expand Up @@ -60,6 +61,8 @@ func TestGetMatchingVpa(t *testing.T) {
AddContainer(test.Container().WithName("i-am-container").Get())
podBuilder := podBuilderWithoutCreator.WithCreator(&sts.ObjectMeta, &sts.TypeMeta)
vpaBuilder := test.VerticalPodAutoscaler().WithContainer("i-am-container")
mapper := restmapper.NewDiscoveryRESTMapper(testDynamicResources())

testCases := []struct {
name string
pod *core.Pod
Expand Down Expand Up @@ -146,7 +149,7 @@ func TestGetMatchingVpa(t *testing.T) {
// In other words, it cannot go through the hierarchy of controllers like "ReplicaSet => Deployment"
// For this reason we are using "StatefulSet" as the ownerRef kind in the test, since it is a direct link.
// The hierarchy part is being test in the "TestControllerFetcher" test.
matcher := NewMatcher(vpaLister, mockSelectorFetcher, controllerfetcher.FakeControllerFetcher{})
matcher := NewMatcher(vpaLister, mockSelectorFetcher, controllerfetcher.FakeControllerFetcher{}, mapper)

vpa := matcher.GetMatchingVPA(tc.pod)
if tc.expectedFound && assert.NotNil(t, vpa) {
Expand All @@ -156,4 +159,24 @@ func TestGetMatchingVpa(t *testing.T) {
}
})
}

}

func testDynamicResources() []*restmapper.APIGroupResources {
return []*restmapper.APIGroupResources{
{
Group: meta.APIGroup{
Name: "apps",
Versions: []meta.GroupVersionForDiscovery{
{Version: "v1"},
},
PreferredVersion: meta.GroupVersionForDiscovery{Version: "v1"},
},
VersionedResources: map[string][]meta.APIResource{
"v1": {
{Name: "statefulset", Namespaced: true, Kind: "StatefulSet"},
},
},
},
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import (
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/history"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/spec"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher"
controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher"
target_mock "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/mock"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test"
)
Expand Down
7 changes: 6 additions & 1 deletion vertical-pod-autoscaler/pkg/updater/logic/updater.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (
"golang.org/x/time/rate"

apiv1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"

"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
kube_client "k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -66,6 +68,7 @@ type updater struct {
useAdmissionControllerStatus bool
statusValidator status.Validator
controllerFetcher controllerfetcher.ControllerFetcher
restMapper meta.RESTMapper
}

// NewUpdater creates Updater with given configuration
Expand All @@ -82,6 +85,7 @@ func NewUpdater(
evictionAdmission priority.PodEvictionAdmission,
selectorFetcher target.VpaTargetSelectorFetcher,
controllerFetcher controllerfetcher.ControllerFetcher,
restMapper meta.RESTMapper,
priorityProcessor priority.PriorityProcessor,
namespace string,
) (Updater, error) {
Expand All @@ -101,6 +105,7 @@ func NewUpdater(
priorityProcessor: priorityProcessor,
selectorFetcher: selectorFetcher,
controllerFetcher: controllerFetcher,
restMapper: restMapper,
useAdmissionControllerStatus: useAdmissionControllerStatus,
statusValidator: status.NewValidator(
kubeClient,
Expand Down Expand Up @@ -172,7 +177,7 @@ func (u *updater) RunOnce(ctx context.Context) {

controlledPods := make(map[*vpa_types.VerticalPodAutoscaler][]*apiv1.Pod)
for _, pod := range allLivePods {
controllingVPA := vpa_api_util.GetControllingVPAForPod(pod, vpas, u.controllerFetcher)
controllingVPA := vpa_api_util.GetControllingVPAForPod(pod, vpas, u.controllerFetcher, u.restMapper)
if controllingVPA != nil {
controlledPods[controllingVPA.Vpa] = append(controlledPods[controllingVPA.Vpa], pod)
}
Expand Down
26 changes: 25 additions & 1 deletion vertical-pod-autoscaler/pkg/updater/logic/updater_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"golang.org/x/time/rate"
v1 "k8s.io/api/autoscaling/v1"
"k8s.io/client-go/restmapper"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -136,7 +137,7 @@ func testRunOnceBase(
containerName := "container1"
rc := apiv1.ReplicationController{
TypeMeta: metav1.TypeMeta{
Kind: "ReplicationController",
Kind: "Deployment",
APIVersion: "apps/v1",
},
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -183,6 +184,8 @@ func testRunOnceBase(

mockSelectorFetcher := target_mock.NewMockVpaTargetSelectorFetcher(ctrl)

mapper := restmapper.NewDiscoveryRESTMapper(testDynamicResources())

updater := &updater{
vpaLister: vpaLister,
podLister: podLister,
Expand All @@ -195,11 +198,13 @@ func testRunOnceBase(
useAdmissionControllerStatus: true,
statusValidator: statusValidator,
priorityProcessor: priority.NewProcessor(),
restMapper: mapper,
}

if expectFetchCalls {
mockSelectorFetcher.EXPECT().Fetch(gomock.Eq(vpaObj)).Return(selector, nil)
}

updater.RunOnce(context.Background())
eviction.AssertNumberOfCalls(t, "Evict", expectedEvictionCount)
}
Expand Down Expand Up @@ -260,3 +265,22 @@ func newFakeValidator(isValid bool) status.Validator {
func (f *fakeValidator) IsStatusValid(statusTimeout time.Duration) (bool, error) {
return f.isValid, nil
}

func testDynamicResources() []*restmapper.APIGroupResources {
return []*restmapper.APIGroupResources{
{
Group: metav1.APIGroup{
Name: "apps",
Versions: []metav1.GroupVersionForDiscovery{
{Version: "v1"},
},
PreferredVersion: metav1.GroupVersionForDiscovery{Version: "v1"},
},
VersionedResources: map[string][]metav1.APIResource{
"v1": {
{Name: "deployments", Namespaced: true, Kind: "Deployment"},
},
},
},
}
}
7 changes: 7 additions & 0 deletions vertical-pod-autoscaler/pkg/updater/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@ import (
"time"

apiv1 "k8s.io/api/core/v1"
cacheddiscovery "k8s.io/client-go/discovery/cached/memory"
"k8s.io/client-go/informers"
kube_client "k8s.io/client-go/kubernetes"
"k8s.io/client-go/restmapper"
kube_flag "k8s.io/component-base/cli/flag"
"k8s.io/klog/v2"

Expand Down Expand Up @@ -91,6 +93,10 @@ func main() {
factory := informers.NewSharedInformerFactory(kubeClient, defaultResyncPeriod)
targetSelectorFetcher := target.NewVpaTargetSelectorFetcher(config, kubeClient, factory)
controllerFetcher := controllerfetcher.NewControllerFetcher(config, kubeClient, factory, scaleCacheEntryFreshnessTime, scaleCacheEntryLifetime, scaleCacheEntryJitterFactor)

discoveryClient := cacheddiscovery.NewMemCacheClient(kubeClient)
mapper := restmapper.NewDeferredDiscoveryRESTMapper(discoveryClient)

var limitRangeCalculator limitrange.LimitRangeCalculator
limitRangeCalculator, err := limitrange.NewLimitsRangeCalculator(factory)
if err != nil {
Expand All @@ -115,6 +121,7 @@ func main() {
priority.NewScalingDirectionPodEvictionAdmission(),
targetSelectorFetcher,
controllerFetcher,
mapper,
priority.NewProcessor(),
*vpaObjectNamespace,
)
Expand Down
2 changes: 1 addition & 1 deletion vertical-pod-autoscaler/pkg/utils/test/test_pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func (pb *podBuilderImpl) Get() *apiv1.Pod {
{
UID: pb.creatorObjectMeta.UID,
Name: pb.creatorObjectMeta.Name,
APIVersion: pb.creatorObjectMeta.ResourceVersion,
APIVersion: pb.creatorTypeMeta.APIVersion,
Kind: pb.creatorTypeMeta.Kind,
Controller: &isController,
},
Expand Down
40 changes: 38 additions & 2 deletions vertical-pod-autoscaler/pkg/utils/vpa/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@ import (

core "k8s.io/api/core/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
apimachinery_meta "k8s.io/apimachinery/pkg/api/meta"
meta "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/tools/cache"
"k8s.io/klog/v2"
Expand Down Expand Up @@ -127,7 +129,7 @@ func stronger(a, b *vpa_types.VerticalPodAutoscaler) bool {
}

// GetControllingVPAForPod chooses the earliest created VPA from the input list that matches the given Pod.
func GetControllingVPAForPod(pod *core.Pod, vpas []*VpaWithSelector, ctrlFetcher controllerfetcher.ControllerFetcher) *VpaWithSelector {
func GetControllingVPAForPod(pod *core.Pod, vpas []*VpaWithSelector, ctrlFetcher controllerfetcher.ControllerFetcher, mapper apimachinery_meta.RESTMapper) *VpaWithSelector {

var ownerRefrence *meta.OwnerReference
for i := range pod.OwnerReferences {
Expand Down Expand Up @@ -157,16 +159,50 @@ func GetControllingVPAForPod(pod *core.Pod, vpas []*VpaWithSelector, ctrlFetcher
return nil
}

targetGVT, err := schema.ParseGroupVersion(parentController.ApiVersion)
if err != nil {
klog.Errorf("fail to get Group/Version of parent: pod=%s err=%s", pod.Name, err.Error())
return nil
}

targetGKT := schema.GroupKind{
Group: targetGVT.Group,
Kind: parentController.Kind,
}
mappingParent, err := mapper.RESTMapping(targetGKT)
if err != nil {
klog.Errorf("fail to get rest mapping for parent: pod=%s err=%s", pod.Name, err.Error())
return nil
}

var controlling *VpaWithSelector
var controllingVpa *vpa_types.VerticalPodAutoscaler
// Choose the strongest VPA from the ones that match this Pod.
for _, vpaWithSelector := range vpas {
if vpaWithSelector.Vpa.Spec.TargetRef == nil {
continue
}
if vpaWithSelector.Vpa.Spec.TargetRef.Kind != parentController.Kind ||

vpaGV, err := schema.ParseGroupVersion(vpaWithSelector.Vpa.Spec.TargetRef.APIVersion)
if err != nil {
klog.Errorf("fail to get Group/Version of target Ref: vpa=%s err=%s", vpaWithSelector.Vpa.Name, err.Error())
continue
}

vpaGK := schema.GroupKind{
Group: vpaGV.Group,
Kind: vpaWithSelector.Vpa.Spec.TargetRef.Kind,
}
mapping, err := mapper.RESTMapping(vpaGK)
if err != nil {
klog.Errorf("fail to get rest mapping for target ref: vpa=%v err=%s", vpaWithSelector.Vpa.Name, err.Error())
continue
}

if mapping.Resource.Resource != mappingParent.Resource.Resource ||
vpaWithSelector.Vpa.Namespace != parentController.Namespace ||
vpaWithSelector.Vpa.Spec.TargetRef.Name != parentController.Name {

continue // This pod is not associated to the right controller
}
if PodMatchesVPA(pod, vpaWithSelector) && stronger(vpaWithSelector.Vpa, controllingVpa) {
Expand Down
Loading

0 comments on commit 0423980

Please sign in to comment.