Skip to content

Commit

Permalink
refactor: use the most accurate pod spec to compute differences
Browse files Browse the repository at this point in the history
The PodSpec template defined by a controller might not represent the final
spec of the pods. For example, a LimitRanger controller could change the spec
of new pods after their creation to set default resource requests and limits.
To ensure that we have a reliable comparison source, we have no choice but to
list the pods and find those who are dependents of the controller to get the
most up-to-date spec.
  • Loading branch information
wI2L committed Jan 18, 2022
1 parent ee2ee7a commit 8f5bbe5
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 19 deletions.
44 changes: 44 additions & 0 deletions client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ package client
import (
"context"
"fmt"
"strings"
"sync"

autoscalingv1 "k8s.io/api/autoscaling/v1"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -16,6 +18,8 @@ import (
"k8s.io/cli-runtime/pkg/resource"
"k8s.io/client-go/discovery"
"k8s.io/client-go/dynamic"
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/client-go/tools/pager"
)

const vpaKind = "VerticalPodAutoscaler"
Expand All @@ -39,6 +43,7 @@ type Interface interface {
HasGroupVersion(version schema.GroupVersion) (bool, error)
ListVPAResources(context.Context, ListOptions) ([]*vpav1.VerticalPodAutoscaler, error)
GetVPATarget(context.Context, *autoscalingv1.CrossVersionObjectReference, string) (*unstructuredv1.Unstructured, error)
ListDependentPods(ctx context.Context, targetMeta metav1.ObjectMeta) ([]*corev1.Pod, error)
}

var _ Interface = (*client)(nil)
Expand All @@ -49,6 +54,7 @@ type client struct {
flags *Flags
dynamicClient dynamic.Interface
discoveryClient discovery.DiscoveryInterface
coreClient *corev1client.CoreV1Client
mapper meta.RESTMapper

// lock during lazy init of the client
Expand Down Expand Up @@ -183,6 +189,44 @@ func (c *client) GetVPATarget(ctx context.Context, ref *autoscalingv1.CrossVersi
return obj, nil
}

// ListDependentPods returns the list of pods that depends
// on the controller represented by its metadata.
func (c *client) ListDependentPods(ctx context.Context, targetMeta metav1.ObjectMeta) ([]*corev1.Pod, error) {
i := c.coreClient.Pods(targetMeta.Namespace)

p := pager.New(func(ctx context.Context, o metav1.ListOptions) (runtime.Object, error) {
return i.List(ctx, o)
})
obj, _, err := p.List(ctx, metav1.ListOptions{
Limit: 250,
})
if err != nil {
return nil, err
}
list := obj.(*corev1.PodList)
pods := make([]*corev1.Pod, 0)

for i, pod := range list.Items {
for _, ref := range pod.GetOwnerReferences() {
// TODO(will): This is rather weak.
// Instead, we should go through the chain of owner references
// to find the top-most controller that match the target ref
// of the VerticalPodAutoscaler resource.
// i.e. Pod -> ReplicaSet -> Deployment
if referenceMatchController(ref, targetMeta) {
pods = append(pods, &list.Items[i])
}
}
}
return pods, nil
}

// referenceMatchController returns whether the given
// OwnerReference matches a target controller.
func referenceMatchController(ref metav1.OwnerReference, targetMeta metav1.ObjectMeta) bool {
return strings.HasPrefix(ref.Name, targetMeta.Name)
}

// hasMatchingGroupVersions returns whether the group versions lists match.
func hasMatchingGroupVersions(groupVersions []metav1.GroupVersionForDiscovery, wantVersions ...string) bool {
b := false
Expand Down
6 changes: 6 additions & 0 deletions client/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/spf13/pflag"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/client-go/dynamic"
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/kubectl/pkg/cmd/get"
cmdutil "k8s.io/kubectl/pkg/cmd/util"
"k8s.io/kubectl/pkg/util"
Expand Down Expand Up @@ -102,10 +103,15 @@ func (f *Flags) NewClient() (Interface, error) {
if err != nil {
return nil, err
}
pc, err := corev1client.NewForConfig(config)
if err != nil {
return nil, err
}
c := &client{
flags: f,
dynamicClient: dyn,
discoveryClient: dis,
coreClient: pc,
mapper: m,
}
return c, nil
Expand Down
63 changes: 44 additions & 19 deletions vpa/target.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ package vpa
import (
"context"
"fmt"
"reflect"
"strings"

autoscalingv1 "k8s.io/api/autoscaling/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
unstructuredv1 "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
Expand Down Expand Up @@ -41,10 +43,10 @@ type TargetController struct {
}

// NewTargetController resolves the target of a VPA resource.
func NewTargetController(client client.Interface, ref *autoscalingv1.CrossVersionObjectReference, namespace string) (*TargetController, error) {
func NewTargetController(c client.Interface, ref *autoscalingv1.CrossVersionObjectReference, namespace string) (*TargetController, error) {
ctx := context.Background()

obj, err := client.GetVPATarget(ctx, ref, namespace)
obj, err := c.GetVPATarget(ctx, ref, namespace)
if err != nil {
return nil, fmt.Errorf("cannot fetch VPa target: %w", err)
}
Expand All @@ -71,9 +73,48 @@ func NewTargetController(client client.Interface, ref *autoscalingv1.CrossVersio
if err != nil {
return nil, err
}
// The PodSpec template defined by a controller might not represent
// the final spec of the pods. For example, a LimitRanger controller
// could change the spec of pods to set default resource requests and
// limits. To ensure that we have a reliable comparison source, we have
// no choice but to list the pods and find those who are dependents of
// the controller to get the most up-to-date spec.
m, _, err := unstructuredv1.NestedMap(obj.Object, "metadata")
if err != nil {
return nil, err
}
meta := metav1.ObjectMeta{}
conv := runtime.DefaultUnstructuredConverter

if err := conv.FromUnstructured(m, &meta); err != nil {
return nil, err
}
pods, err := c.ListDependentPods(context.Background(), meta)
if err != nil {
return nil, err
}
if len(pods) != 0 {
p := pods[0]
if !reflect.DeepEqual(p.Spec.Containers, tc.podSpec.Containers) {
tc.podSpec = &p.Spec
}
}
return tc, nil
}

// GetContainerRequests returns the resource requests of a container.
func (tc *TargetController) GetContainerRequests(name string) *ResourceQuantities {
for _, c := range tc.podSpec.Containers {
if c.Name == name {
return &ResourceQuantities{
CPU: c.Resources.Requests.Cpu(),
Memory: c.Resources.Requests.Memory(),
}
}
}
return nil
}

// GetRequests returns the resource requests defined by the
// pod spec of the controller, which is the sum of all resource
// quantities for each container declared by the spec.
Expand All @@ -89,23 +130,7 @@ func (tc *TargetController) GetRequests() *ResourceQuantities {
mem.Add(*m)
}
}
return &ResourceQuantities{
CPU: &cpu,
Memory: &mem,
}
}

// GetContainerRequests returns the resource requests of a container.
func (tc *TargetController) GetContainerRequests(name string) *ResourceQuantities {
for _, c := range tc.podSpec.Containers {
if c.Name == name {
return &ResourceQuantities{
CPU: c.Resources.Requests.Cpu(),
Memory: c.Resources.Requests.Memory(),
}
}
}
return nil
return &ResourceQuantities{CPU: &cpu, Memory: &mem}
}

// resolvePodSpec returns the corev1.PodSpec field of a
Expand Down

0 comments on commit 8f5bbe5

Please sign in to comment.