Skip to content

Commit

Permalink
Remove legacy selector fetcher
Browse files Browse the repository at this point in the history
  • Loading branch information
@bskiba
bskiba committed Jun 11, 2019
1 parent 6634b97 commit 4e07e1e
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 436 deletions.
5 changes: 1 addition & 4 deletions vertical-pod-autoscaler/pkg/admission-controller/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,7 @@ func main() {
vpaLister := vpa_api_util.NewAllVpasLister(vpaClient, make(chan struct{}))
kubeClient := kube_client.NewForConfigOrDie(config)
factory := informers.NewSharedInformerFactory(kubeClient, defaultResyncPeriod)
targetSelectorFetcher := target.NewCompositeTargetSelectorFetcher(
target.NewVpaTargetSelectorFetcher(config, kubeClient, factory),
target.NewBeta1TargetSelectorFetcher(config),
)
targetSelectorFetcher := target.NewVpaTargetSelectorFetcher(config, kubeClient, factory)
podPreprocessor := logic.NewDefaultPodPreProcessor()
vpaPreprocessor := logic.NewDefaultVpaPreProcessor()
var limitRangeCalculator limitrange.LimitRangeCalculator
Expand Down
109 changes: 42 additions & 67 deletions vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
vpa_clientset "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned"
vpa_api "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned/typed/autoscaling.k8s.io/v1beta2"
vpa_lister "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/listers/autoscaling.k8s.io/v1beta2"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher"
controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/history"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/metrics"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/oom"
Expand Down Expand Up @@ -75,33 +75,31 @@ type ClusterStateFeeder interface {

// ClusterStateFeederFactory makes instances of ClusterStateFeeder.
type ClusterStateFeederFactory struct {
ClusterState *model.ClusterState
KubeClient kube_client.Interface
MetricsClient metrics.MetricsClient
VpaCheckpointClient vpa_api.VerticalPodAutoscalerCheckpointsGetter
VpaLister vpa_lister.VerticalPodAutoscalerLister
PodLister v1lister.PodLister
OOMObserver oom.Observer
LegacySelectorFetcher target.VpaTargetSelectorFetcher
SelectorFetcher target.VpaTargetSelectorFetcher
MemorySaveMode bool
ControllerFetcher controllerfetcher.ControllerFetcher
ClusterState *model.ClusterState
KubeClient kube_client.Interface
MetricsClient metrics.MetricsClient
VpaCheckpointClient vpa_api.VerticalPodAutoscalerCheckpointsGetter
VpaLister vpa_lister.VerticalPodAutoscalerLister
PodLister v1lister.PodLister
OOMObserver oom.Observer
SelectorFetcher target.VpaTargetSelectorFetcher
MemorySaveMode bool
ControllerFetcher controllerfetcher.ControllerFetcher
}

// Make creates new ClusterStateFeeder with internal data providers, based on kube client.
func (m ClusterStateFeederFactory) Make() *clusterStateFeeder {
return &clusterStateFeeder{
coreClient: m.KubeClient.CoreV1(),
metricsClient: m.MetricsClient,
oomChan: m.OOMObserver.GetObservedOomsChannel(),
vpaCheckpointClient: m.VpaCheckpointClient,
vpaLister: m.VpaLister,
clusterState: m.ClusterState,
specClient: spec.NewSpecClient(m.PodLister),
legacySelectorFetcher: m.LegacySelectorFetcher,
selectorFetcher: m.SelectorFetcher,
memorySaveMode: m.MemorySaveMode,
controllerFetcher: m.ControllerFetcher,
coreClient: m.KubeClient.CoreV1(),
metricsClient: m.MetricsClient,
oomChan: m.OOMObserver.GetObservedOomsChannel(),
vpaCheckpointClient: m.VpaCheckpointClient,
vpaLister: m.VpaLister,
clusterState: m.ClusterState,
specClient: spec.NewSpecClient(m.PodLister),
selectorFetcher: m.SelectorFetcher,
memorySaveMode: m.MemorySaveMode,
controllerFetcher: m.ControllerFetcher,
}
}

Expand All @@ -113,17 +111,16 @@ func NewClusterStateFeeder(config *rest.Config, clusterState *model.ClusterState
factory := informers.NewSharedInformerFactory(kubeClient, defaultResyncPeriod)
controllerFetcher := controllerfetcher.NewControllerFetcher(config, kubeClient, factory)
return ClusterStateFeederFactory{
PodLister: podLister,
OOMObserver: oomObserver,
KubeClient: kubeClient,
MetricsClient: newMetricsClient(config),
VpaCheckpointClient: vpa_clientset.NewForConfigOrDie(config).AutoscalingV1beta2(),
VpaLister: vpa_api_util.NewAllVpasLister(vpa_clientset.NewForConfigOrDie(config), make(chan struct{})),
ClusterState: clusterState,
LegacySelectorFetcher: target.NewBeta1TargetSelectorFetcher(config),
SelectorFetcher: target.NewVpaTargetSelectorFetcher(config, kubeClient, factory),
MemorySaveMode: memorySave,
ControllerFetcher: controllerFetcher,
PodLister: podLister,
OOMObserver: oomObserver,
KubeClient: kubeClient,
MetricsClient: newMetricsClient(config),
VpaCheckpointClient: vpa_clientset.NewForConfigOrDie(config).AutoscalingV1beta2(),
VpaLister: vpa_api_util.NewAllVpasLister(vpa_clientset.NewForConfigOrDie(config), make(chan struct{})),
ClusterState: clusterState,
SelectorFetcher: target.NewVpaTargetSelectorFetcher(config, kubeClient, factory),
MemorySaveMode: memorySave,
ControllerFetcher: controllerFetcher,
}.Make()
}

Expand Down Expand Up @@ -193,17 +190,16 @@ func NewPodListerAndOOMObserver(kubeClient kube_client.Interface) (v1lister.PodL
}

type clusterStateFeeder struct {
coreClient corev1.CoreV1Interface
specClient spec.SpecClient
metricsClient metrics.MetricsClient
oomChan <-chan oom.OomInfo
vpaCheckpointClient vpa_api.VerticalPodAutoscalerCheckpointsGetter
vpaLister vpa_lister.VerticalPodAutoscalerLister
clusterState *model.ClusterState
legacySelectorFetcher target.VpaTargetSelectorFetcher
selectorFetcher target.VpaTargetSelectorFetcher
memorySaveMode bool
controllerFetcher controllerfetcher.ControllerFetcher
coreClient corev1.CoreV1Interface
specClient spec.SpecClient
metricsClient metrics.MetricsClient
oomChan <-chan oom.OomInfo
vpaCheckpointClient vpa_api.VerticalPodAutoscalerCheckpointsGetter
vpaLister vpa_lister.VerticalPodAutoscalerLister
clusterState *model.ClusterState
selectorFetcher target.VpaTargetSelectorFetcher
memorySaveMode bool
controllerFetcher controllerfetcher.ControllerFetcher
}

func (feeder *clusterStateFeeder) InitFromHistoryProvider(historyProvider history.HistoryProvider) {
Expand Down Expand Up @@ -330,9 +326,6 @@ func (feeder *clusterStateFeeder) LoadVPAs() {
// Successfully added VPA to the model.
vpaKeys[vpaID] = true

legacySelector, _ := feeder.legacySelectorFetcher.Fetch(vpaCRD)
feeder.clusterState.Vpas[vpaID].IsV1Beta1API = legacySelector != nil

for _, condition := range conditions {
if condition.delete {
delete(feeder.clusterState.Vpas[vpaID].Conditions, condition.conditionType)
Expand Down Expand Up @@ -471,21 +464,8 @@ func (feeder *clusterStateFeeder) validateTargetRef(vpa *vpa_types.VerticalPodAu
}

func (feeder *clusterStateFeeder) getSelector(vpa *vpa_types.VerticalPodAutoscaler) (labels.Selector, []condition) {
legacySelector, fetchLegacyErr := feeder.legacySelectorFetcher.Fetch(vpa)
if fetchLegacyErr != nil {
klog.Errorf("Error while fetching legacy selector. Reason: %+v", fetchLegacyErr)
}
selector, fetchErr := feeder.selectorFetcher.Fetch(vpa)
if fetchErr != nil {
klog.Errorf("Cannot get target selector from VPA's targetRef. Reason: %+v", fetchErr)
}
if selector != nil {
if legacySelector != nil {
return labels.Nothing(), []condition{
{conditionType: vpa_types.ConfigUnsupported, delete: false, message: "Both targetRef and label selector defined. Please remove label selector"},
{conditionType: vpa_types.ConfigDeprecated, delete: true},
}
}
validTargetRef, unsupportedCondition := feeder.validateTargetRef(vpa)
if !validTargetRef {
return labels.Nothing(), []condition{
Expand All @@ -498,14 +478,9 @@ func (feeder *clusterStateFeeder) getSelector(vpa *vpa_types.VerticalPodAutoscal
{conditionType: vpa_types.ConfigDeprecated, delete: true},
}
}
if legacySelector != nil {
return labels.Nothing(), []condition{
{conditionType: vpa_types.ConfigUnsupported, delete: false, message: "Label selector is no longer supported, please migrate to targetRef"},
{conditionType: vpa_types.ConfigDeprecated, delete: true},
}
}
msg := "Cannot read targetRef"
if fetchErr != nil {
klog.Errorf("Cannot get target selector from VPA's targetRef. Reason: %+v", fetchErr)
msg = fmt.Sprintf("Cannot read targetRef. Reason: %s", fetchErr.Error())
}
return labels.Nothing(), []condition{
Expand Down
57 changes: 13 additions & 44 deletions vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,16 @@ package input

import (
"fmt"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher"
"testing"

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

"k8s.io/api/autoscaling/v1"
autoscalingv1 "k8s.io/api/autoscaling/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1beta2"
controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/spec"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model"
target_mock "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/mock"
Expand Down Expand Up @@ -67,14 +67,13 @@ const (
apiVersion = "stardust"
)

func TestLegacySelector(t *testing.T) {
func TestLoadPods(t *testing.T) {

type testCase struct {
name string
legacySelector labels.Selector
selector labels.Selector
fetchSelectorError error
targetRef *v1.CrossVersionObjectReference
targetRef *autoscalingv1.CrossVersionObjectReference
topLevelKey *controllerfetcher.ControllerKeyWithAPIVersion
findTopLevelError error
expectedSelector labels.Selector
Expand All @@ -85,7 +84,6 @@ func TestLegacySelector(t *testing.T) {
testCases := []testCase{
{
name: "no selector",
legacySelector: nil,
selector: nil,
fetchSelectorError: fmt.Errorf("targetRef not defined"),
expectedSelector: labels.Nothing(),
Expand All @@ -94,28 +92,17 @@ func TestLegacySelector(t *testing.T) {
},
{
name: "also no selector but no error",
legacySelector: nil,
selector: nil,
fetchSelectorError: nil,
expectedSelector: labels.Nothing(),
expectedConfigUnsupported: &unsupportedConditionNoExtraText,
expectedConfigDeprecated: nil,
},
{
name: "legacy selector no ref",
legacySelector: parseLabelSelector("app = test"),
selector: nil,
fetchSelectorError: fmt.Errorf("targetRef not defined"),
expectedSelector: labels.Nothing(),
expectedConfigUnsupported: &unsupportedConditionNoLongerSupported,
expectedConfigDeprecated: nil,
}, {
name: "targetRef selector",
// the only valid option since v1beta1 removal
legacySelector: nil,
name: "targetRef selector",
selector: parseLabelSelector("app = test"),
fetchSelectorError: nil,
targetRef: &v1.CrossVersionObjectReference{
targetRef: &autoscalingv1.CrossVersionObjectReference{
Kind: kind,
Name: name1,
APIVersion: apiVersion,
Expand All @@ -131,22 +118,13 @@ func TestLegacySelector(t *testing.T) {
expectedSelector: parseLabelSelector("app = test"),
expectedConfigUnsupported: nil,
expectedConfigDeprecated: nil,
}, {
name: "new and legacy selector",
legacySelector: parseLabelSelector("app = test1"),
selector: parseLabelSelector("app = test2"),
fetchSelectorError: nil,
expectedSelector: labels.Nothing(),
expectedConfigUnsupported: &unsupportedConditionBothDefined,
expectedConfigDeprecated: nil,
},
{
name: "can't decide if top-level-ref",
legacySelector: nil,
selector: nil,
fetchSelectorError: nil,
expectedSelector: labels.Nothing(),
targetRef: &v1.CrossVersionObjectReference{
targetRef: &autoscalingv1.CrossVersionObjectReference{
Kind: kind,
Name: name1,
APIVersion: apiVersion,
Expand All @@ -155,11 +133,10 @@ func TestLegacySelector(t *testing.T) {
},
{
name: "non-top-level targetRef",
legacySelector: nil,
selector: parseLabelSelector("app = test"),
fetchSelectorError: nil,
expectedSelector: labels.Nothing(),
targetRef: &v1.CrossVersionObjectReference{
targetRef: &autoscalingv1.CrossVersionObjectReference{
Kind: kind,
Name: name1,
APIVersion: apiVersion,
Expand All @@ -176,11 +153,10 @@ func TestLegacySelector(t *testing.T) {
},
{
name: "error checking if top-level-ref",
legacySelector: nil,
selector: parseLabelSelector("app = test"),
fetchSelectorError: nil,
expectedSelector: labels.Nothing(),
targetRef: &v1.CrossVersionObjectReference{
targetRef: &autoscalingv1.CrossVersionObjectReference{
Kind: "doestar",
Name: "doseph-doestar",
APIVersion: "taxonomy",
Expand All @@ -190,11 +166,10 @@ func TestLegacySelector(t *testing.T) {
},
{
name: "top-level target ref",
legacySelector: nil,
selector: parseLabelSelector("app = test"),
fetchSelectorError: nil,
expectedSelector: parseLabelSelector("app = test"),
targetRef: &v1.CrossVersionObjectReference{
targetRef: &autoscalingv1.CrossVersionObjectReference{
Kind: kind,
Name: name1,
APIVersion: apiVersion,
Expand All @@ -221,26 +196,20 @@ func TestLegacySelector(t *testing.T) {
vpaLister := &test.VerticalPodAutoscalerListerMock{}
vpaLister.On("List").Return([]*vpa_types.VerticalPodAutoscaler{vpa}, nil)

legacyTargetSelectorFetcher := target_mock.NewMockVpaTargetSelectorFetcher(ctrl)
targetSelectorFetcher := target_mock.NewMockVpaTargetSelectorFetcher(ctrl)

clusterState := model.NewClusterState()

clusterStateFeeder := clusterStateFeeder{
vpaLister: vpaLister,
clusterState: clusterState,
legacySelectorFetcher: legacyTargetSelectorFetcher,
selectorFetcher: targetSelectorFetcher,
vpaLister: vpaLister,
clusterState: clusterState,
selectorFetcher: targetSelectorFetcher,
controllerFetcher: &fakeControllerFetcher{
key: tc.topLevelKey,
err: tc.findTopLevelError,
},
}

// legacyTargetSelectorFetcher is called twice:
// - one time to determine ultimate selector
// - one time to check if object uses deprecated API
legacyTargetSelectorFetcher.EXPECT().Fetch(vpa).Times(2).Return(tc.legacySelector, nil)
targetSelectorFetcher.EXPECT().Fetch(vpa).Return(tc.selector, tc.fetchSelectorError)
clusterStateFeeder.LoadVPAs()

Expand Down
49 changes: 0 additions & 49 deletions vertical-pod-autoscaler/pkg/target/composite_fetcher.go
View file

This file was deleted.

0 comments on commit 4e07e1e

Please sign in to comment.