Skip to content

Commit

Permalink
Merge pull request #2275 from uswitch/vpa-find-topmost-wellknown-or-s…
Browse files Browse the repository at this point in the history 
…calable-controller-only

VPA: find topmost well-known or scalable controller
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jan 7, 2020
2 parents 932d62f + 369dfb8 commit 866c27c
Show file tree
Hide file tree
Showing 6 changed files with 286 additions and 42 deletions.
8 changes: 4 additions & 4 deletions vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -458,15 +458,15 @@ func (feeder *clusterStateFeeder) validateTargetRef(vpa *vpa_types.VerticalPodAu
},
ApiVersion: vpa.Spec.TargetRef.APIVersion,
}
top, err := feeder.controllerFetcher.FindTopLevel(&k)
top, err := feeder.controllerFetcher.FindTopMostWellKnownOrScalable(&k)
if err != nil {
return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: fmt.Sprintf("Error checking if target is a top level controller: %s", err)}
return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: fmt.Sprintf("Error checking if target is a topmost well-known or scalable controller: %s", err)}
}
if top == nil {
return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: fmt.Sprintf("Unknown error during checking if target is a top level controller: %s", err)}
return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: fmt.Sprintf("Unknown error during checking if target is a topmost well-known or scalable controller: %s", err)}
}
if *top != k {
return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: "The targetRef controller has a parent but it should point to a top-level controller"}
return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: "The targetRef controller has a parent but it should point to a topmost well-known or scalable controller"}
}
return true, condition{}
}
Expand Down
38 changes: 19 additions & 19 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 @@ -39,7 +39,7 @@ type fakeControllerFetcher struct {
err error
}

func (f *fakeControllerFetcher) FindTopLevel(controller *controllerfetcher.ControllerKeyWithAPIVersion) (*controllerfetcher.ControllerKeyWithAPIVersion, error) {
func (f *fakeControllerFetcher) FindTopMostWellKnownOrScalable(controller *controllerfetcher.ControllerKeyWithAPIVersion) (*controllerfetcher.ControllerKeyWithAPIVersion, error) {
return f.key, f.err
}

Expand All @@ -55,8 +55,8 @@ var (
unsupportedConditionNoExtraText = "Cannot read targetRef"
unsupportedConditionBothDefined = "Both targetRef and label selector defined. Please remove label selector"
unsupportedConditionNoTargetRef = "Cannot read targetRef"
unsupportedConditionMudaMudaMuda = "Error checking if target is a top level controller: muda muda muda"
unsupportedTargetRefHasParent = "The targetRef controller has a parent but it should point to a top-level controller"
unsupportedConditionMudaMudaMuda = "Error checking if target is a topmost well-known or scalable controller: muda muda muda"
unsupportedTargetRefHasParent = "The targetRef controller has a parent but it should point to a topmost well-known or scalable controller"
)

const (
Expand All @@ -70,15 +70,15 @@ const (
func TestLoadPods(t *testing.T) {

type testCase struct {
name string
selector labels.Selector
fetchSelectorError error
targetRef *autoscalingv1.CrossVersionObjectReference
topLevelKey *controllerfetcher.ControllerKeyWithAPIVersion
findTopLevelError error
expectedSelector labels.Selector
expectedConfigUnsupported *string
expectedConfigDeprecated *string
name string
selector labels.Selector
fetchSelectorError error
targetRef *autoscalingv1.CrossVersionObjectReference
topMostWellKnownOrScalableKey *controllerfetcher.ControllerKeyWithAPIVersion
findTopMostWellKnownOrScalableError error
expectedSelector labels.Selector
expectedConfigUnsupported *string
expectedConfigDeprecated *string
}

testCases := []testCase{
Expand Down Expand Up @@ -107,7 +107,7 @@ func TestLoadPods(t *testing.T) {
Name: name1,
APIVersion: apiVersion,
},
topLevelKey: &controllerfetcher.ControllerKeyWithAPIVersion{
topMostWellKnownOrScalableKey: &controllerfetcher.ControllerKeyWithAPIVersion{
ControllerKey: controllerfetcher.ControllerKey{
Kind: kind,
Name: name1,
Expand Down Expand Up @@ -149,7 +149,7 @@ func TestLoadPods(t *testing.T) {
Name: name1,
APIVersion: apiVersion,
},
topLevelKey: &controllerfetcher.ControllerKeyWithAPIVersion{
topMostWellKnownOrScalableKey: &controllerfetcher.ControllerKeyWithAPIVersion{
ControllerKey: controllerfetcher.ControllerKey{
Kind: kind,
Name: name2,
Expand All @@ -169,8 +169,8 @@ func TestLoadPods(t *testing.T) {
Name: "doseph-doestar",
APIVersion: "taxonomy",
},
expectedConfigUnsupported: &unsupportedConditionMudaMudaMuda,
findTopLevelError: fmt.Errorf("muda muda muda"),
expectedConfigUnsupported: &unsupportedConditionMudaMudaMuda,
findTopMostWellKnownOrScalableError: fmt.Errorf("muda muda muda"),
},
{
name: "top-level target ref",
Expand All @@ -182,7 +182,7 @@ func TestLoadPods(t *testing.T) {
Name: name1,
APIVersion: apiVersion,
},
topLevelKey: &controllerfetcher.ControllerKeyWithAPIVersion{
topMostWellKnownOrScalableKey: &controllerfetcher.ControllerKeyWithAPIVersion{
ControllerKey: controllerfetcher.ControllerKey{
Kind: kind,
Name: name1,
Expand Down Expand Up @@ -213,8 +213,8 @@ func TestLoadPods(t *testing.T) {
clusterState: clusterState,
selectorFetcher: targetSelectorFetcher,
controllerFetcher: &fakeControllerFetcher{
key: tc.topLevelKey,
err: tc.findTopLevelError,
key: tc.topMostWellKnownOrScalableKey,
err: tc.findTopMostWellKnownOrScalableError,
},
}

Expand Down
94 changes: 79 additions & 15 deletions vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher/controller_fetcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
batchv1 "k8s.io/api/batch/v1"
batchv1beta1 "k8s.io/api/batch/v1beta1"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
Expand Down Expand Up @@ -69,10 +70,10 @@ type ControllerKeyWithAPIVersion struct {
ApiVersion string
}

// ControllerFetcher is responsible for finding the top level controller
// ControllerFetcher is responsible for finding the topmost well-known or scalable controller
type ControllerFetcher interface {
// FindTopLevel returns top level controller. Error is returned if top level controller cannot be found.
FindTopLevel(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error)
// FindTopMostWellKnownOrScalable returns topmost well-known or scalable controller. Error is returned if controller cannot be found.
FindTopMostWellKnownOrScalable(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error)
}

type controllerFetcher struct {
Expand Down Expand Up @@ -126,7 +127,7 @@ func NewControllerFetcher(config *rest.Config, kubeClient kube_client.Interface,

func getOwnerController(owners []metav1.OwnerReference, namespace string) *ControllerKeyWithAPIVersion {
for _, owner := range owners {
if owner.Controller != nil && *owner.Controller == true {
if owner.Controller != nil && *owner.Controller {
return &ControllerKeyWithAPIVersion{
ControllerKey: ControllerKey{
Namespace: namespace,
Expand Down Expand Up @@ -207,17 +208,15 @@ func (f *controllerFetcher) getParentOfController(controllerKey ControllerKeyWit
return getParentOfWellKnownController(informer, controllerKey)
}

// TODO: cache response
groupVersion, err := schema.ParseGroupVersion(controllerKey.ApiVersion)
groupKind, err := controllerKey.groupKind()
if err != nil {
return nil, err
}
groupKind := schema.GroupKind{
Group: groupVersion.Group,
Kind: controllerKey.Kind,
}

owner, err := f.getOwnerForScaleResource(groupKind, controllerKey.Namespace, controllerKey.Name)
if apierrors.IsNotFound(err) {
return nil, nil
}
if err != nil {
return nil, fmt.Errorf("Unhandled targetRef %s / %s / %s, last error %v",
controllerKey.ApiVersion, controllerKey.Kind, controllerKey.Name, err)
Expand All @@ -226,6 +225,55 @@ func (f *controllerFetcher) getParentOfController(controllerKey ControllerKeyWit
return owner, nil
}

func (c *ControllerKeyWithAPIVersion) groupKind() (schema.GroupKind, error) {
// TODO: cache response
groupVersion, err := schema.ParseGroupVersion(c.ApiVersion)
if err != nil {
return schema.GroupKind{}, err
}

groupKind := schema.GroupKind{
Group: groupVersion.Group,
Kind: c.ControllerKey.Kind,
}

return groupKind, nil
}

func (f *controllerFetcher) isWellKnown(key *ControllerKeyWithAPIVersion) bool {
kind := wellKnownController(key.ControllerKey.Kind)
_, exists := f.informersMap[kind]
return exists
}

func (f *controllerFetcher) isWellKnownOrScalable(key *ControllerKeyWithAPIVersion) bool {
if f.isWellKnown(key) {
return true
}

//if not well known check if it supports scaling
groupKind, err := key.groupKind()
if err != nil {
klog.Errorf("Could not find groupKind for %s/%s: %v", key.Namespace, key.Name, err)
return false
}

mappings, err := f.mapper.RESTMappings(groupKind)
if err != nil {
klog.Errorf("Could not find mappings for %s: %v", groupKind, err)
return false
}

for _, mapping := range mappings {
groupResource := mapping.Resource.GroupResource()
scale, err := f.scaleNamespacer.Scales(key.Namespace).Get(groupResource, key.Name)
if err == nil && scale != nil {
return true
}
}
return false
}

func (f *controllerFetcher) getOwnerForScaleResource(groupKind schema.GroupKind, namespace, name string) (*ControllerKeyWithAPIVersion, error) {
mappings, err := f.mapper.RESTMappings(groupKind)
if err != nil {
Expand All @@ -246,41 +294,57 @@ func (f *controllerFetcher) getOwnerForScaleResource(groupKind schema.GroupKind,
return nil, lastError
}

func (f *controllerFetcher) FindTopLevel(key *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) {
func (f *controllerFetcher) FindTopMostWellKnownOrScalable(key *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) {
if key == nil {
return nil, nil
}

var topMostWellKnownOrScalable *ControllerKeyWithAPIVersion

wellKnownOrScalable := f.isWellKnownOrScalable(key)
if wellKnownOrScalable {
topMostWellKnownOrScalable = key
}

visited := make(map[ControllerKeyWithAPIVersion]bool)
visited[*key] = true
for {
owner, err := f.getParentOfController(*key)
if err != nil {
return nil, err
}

if owner == nil {
return key, nil
return topMostWellKnownOrScalable, nil
}

wellKnownOrScalable = f.isWellKnownOrScalable(owner)
if wellKnownOrScalable {
topMostWellKnownOrScalable = owner
}

_, alreadyVisited := visited[*owner]
if alreadyVisited {
return nil, fmt.Errorf("Cycle detected in ownership chain")
}
visited[*key] = true

key = owner
}
}

type identityControllerFetcher struct {
}

func (f *identityControllerFetcher) FindTopLevel(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) {
func (f *identityControllerFetcher) FindTopMostWellKnownOrScalable(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) {
return controller, nil
}

type constControllerFetcher struct {
ControllerKeyWithAPIVersion *ControllerKeyWithAPIVersion
}

func (f *constControllerFetcher) FindTopLevel(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) {
func (f *constControllerFetcher) FindTopMostWellKnownOrScalable(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) {
return f.ControllerKeyWithAPIVersion, nil
}

Expand All @@ -289,7 +353,7 @@ type mockControllerFetcher struct {
result *ControllerKeyWithAPIVersion
}

func (f *mockControllerFetcher) FindTopLevel(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) {
func (f *mockControllerFetcher) FindTopMostWellKnownOrScalable(controller *ControllerKeyWithAPIVersion) (*ControllerKeyWithAPIVersion, error) {
if controller == nil && f.expected == nil {
return f.result, nil
}
Expand Down
Loading

0 comments on commit 866c27c

Please sign in to comment.