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

Feat: improve VPA filtering and checkpoint garbage collection #7716

Merged
merged 6 commits into from
Feb 5, 2025
+46 −15
Merged

Feat: improve VPA filtering and checkpoint garbage collection #7716

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8117b14
Feat: improve VPA filtering and checkpoint garbage collection
omerap12 Jan 17, 2025
9d450f8
Merge branch 'master' into recommender-namespaces
omerap12 Jan 21, 2025
3d94088
Fixed typo
omerap12 Jan 21, 2025
f4f67f5
Simplify function
omerap12 Jan 31, 2025
86eb551
using wrapf errors
omerap12 Jan 31, 2025
694cce8
Change function name
omerap12 Feb 4, 2025
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
4 changes: 2 additions & 2 deletions vertical-pod-autoscaler/common/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ func InitCommonFlags() *CommonFlags {
flag.Float64Var(&cf.KubeApiQps, "kube-api-qps", 5.0, "QPS limit when making requests to Kubernetes apiserver")
flag.Float64Var(&cf.KubeApiBurst, "kube-api-burst", 10.0, "QPS burst limit when making requests to Kubernetes apiserver")
flag.BoolVar(&cf.EnableProfiling, "profiling", false, "Is debug/pprof endpoint enabled")
flag.StringVar(&cf.VpaObjectNamespace, "vpa-object-namespace", apiv1.NamespaceAll, "Namespace to search for VPA objects. Empty means all namespaces will be used.")
flag.StringVar(&cf.IgnoredVpaObjectNamespaces, "ignored-vpa-object-namespaces", "", "Comma separated list of namespaces to ignore when searching for VPA objects. Empty means no namespaces will be ignored.")
flag.StringVar(&cf.VpaObjectNamespace, "vpa-object-namespace", apiv1.NamespaceAll, "Specifies the namespace to search for VPA objects. Leave empty to include all namespaces. If provided, the garbage collector will only clean this namespace.")
flag.StringVar(&cf.IgnoredVpaObjectNamespaces, "ignored-vpa-object-namespaces", "", "A comma-separated list of namespaces to ignore when searching for VPA objects. Leave empty to avoid ignoring any namespaces. These namespaces will not be cleaned by the garbage collector.")
return cf
}

Expand Down
56 changes: 42 additions & 14 deletions vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ type ClusterStateFeederFactory struct {
ControllerFetcher controllerfetcher.ControllerFetcher
RecommenderName string
IgnoredNamespaces []string
VpaObjectNamespace string
}

// Make creates new ClusterStateFeeder with internal data providers, based on kube client.
Expand All @@ -106,6 +107,7 @@ func (m ClusterStateFeederFactory) Make() *clusterStateFeeder {
controllerFetcher: m.ControllerFetcher,
recommenderName: m.RecommenderName,
ignoredNamespaces: m.IgnoredNamespaces,
vpaObjectNamespace: m.VpaObjectNamespace,
}
}

Expand Down Expand Up @@ -198,6 +200,7 @@ type clusterStateFeeder struct {
controllerFetcher controllerfetcher.ControllerFetcher
recommenderName string
ignoredNamespaces []string
vpaObjectNamespace string
}

func (feeder *clusterStateFeeder) InitFromHistoryProvider(historyProvider history.HistoryProvider) {
Expand Down Expand Up @@ -300,21 +303,46 @@ func (feeder *clusterStateFeeder) GarbageCollectCheckpoints() {

for _, namespaceItem := range namespaceList.Items {
namespace := namespaceItem.Name
checkpointList, err := feeder.vpaCheckpointClient.VerticalPodAutoscalerCheckpoints(namespace).List(context.TODO(), metav1.ListOptions{})
if err != nil {
klog.ErrorS(err, "Cannot list VPA checkpoints", "namespace", namespace)
// Clean the namespace if any of the following conditions are true:
// 1. `vpaObjectNamespace` is set and matches the current namespace.
// 2. `ignoredNamespaces` is set, but the current namespace is not in the list.
// 3. Neither `vpaObjectNamespace` nor `ignoredNamespaces` is set, so all namespaces are included.
if feeder.shouldCleanupNamespace(namespace) {
omerap12 marked this conversation as resolved.
Show resolved Hide resolved
feeder.cleanupCheckpointsForNamespace(namespace, allVPAKeys)
} else {
klog.V(3).InfoS("Skipping namespace; it does not meet cleanup criteria", "namespace", namespace, "vpaObjectNamespace", feeder.vpaObjectNamespace, "ignoredNamespaces", feeder.ignoredNamespaces)
}
for _, checkpoint := range checkpointList.Items {
vpaID := model.VpaID{Namespace: checkpoint.Namespace, VpaName: checkpoint.Spec.VPAObjectName}
exists := allVPAKeys[vpaID]
}
}

if !exists {
err = feeder.vpaCheckpointClient.VerticalPodAutoscalerCheckpoints(namespace).Delete(context.TODO(), checkpoint.Name, metav1.DeleteOptions{})
if err == nil {
klog.V(3).InfoS("Orphaned VPA checkpoint cleanup - deleting", "checkpoint", klog.KRef(namespace, checkpoint.Name))
} else {
klog.ErrorS(err, "Orphaned VPA checkpoint cleanup - error deleting", "checkpoint", klog.KRef(namespace, checkpoint.Name))
}
func (feeder *clusterStateFeeder) shouldCleanupNamespace(namespace string) bool {
omerap12 marked this conversation as resolved.
Show resolved Hide resolved
// Case 1: Specific namespace explicitly set.
if feeder.vpaObjectNamespace == namespace {
return true
}
// Case 2: Ignored namespaces are defined, and this namespace is not ignored.
if len(feeder.ignoredNamespaces) > 0 {
return !slices.Contains(feeder.ignoredNamespaces, namespace)
}
// Case 3: Default to including all namespaces if no filters are defined.
return feeder.vpaObjectNamespace == "" && len(feeder.ignoredNamespaces) == 0
}

func (feeder *clusterStateFeeder) cleanupCheckpointsForNamespace(namespace string, allVPAKeys map[model.VpaID]bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make this function return err and have the caller handle it. This way, you can also simplify the if-else and the error messages, as they don't need to contain the context that this happened inside the garbage collection function. Adding context for the checkpoint listing with e.g. errors.Wrapf could still be useful, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 86eb551

checkpointList, err := feeder.vpaCheckpointClient.VerticalPodAutoscalerCheckpoints(namespace).List(context.TODO(), metav1.ListOptions{})
if err != nil {
klog.ErrorS(err, "Cannot list VPA checkpoints", "namespace", namespace)
return
}
for _, checkpoint := range checkpointList.Items {
vpaID := model.VpaID{Namespace: checkpoint.Namespace, VpaName: checkpoint.Spec.VPAObjectName}
exists := allVPAKeys[vpaID]
if !exists {
err = feeder.vpaCheckpointClient.VerticalPodAutoscalerCheckpoints(namespace).Delete(context.TODO(), checkpoint.Name, metav1.DeleteOptions{})
if err == nil {
klog.V(3).InfoS("Orphaned VPA checkpoint cleanup - deleting", "checkpoint", klog.KRef(namespace, checkpoint.Name))
} else {
klog.ErrorS(err, "Orphaned VPA checkpoint cleanup - error deleting", "checkpoint", klog.KRef(namespace, checkpoint.Name))
}
}
}
Expand Down Expand Up @@ -354,7 +382,7 @@ func filterVPAs(feeder *clusterStateFeeder, allVpaCRDs []*vpa_types.VerticalPodA
}
}

if slices.Contains(feeder.ignoredNamespaces, vpaCRD.ObjectMeta.Namespace) {
if !feeder.shouldCleanupNamespace(vpaCRD.ObjectMeta.Namespace) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're re-using shouldCleanupNamespace here, I think we should rename it. Currently, its name is tied pretty much to the garbage collection use-case. Maybe it is rather something like shouldIgnoreNamespace?

On the other hand, we already initialize the vpaLister with a single namespace if vpaObjectNamespace is set:

autoscaler/vertical-pod-autoscaler/pkg/recommender/main.go

Line 251 in 29b611d

VpaLister: vpa_api_util.NewVpasLister(vpa_clientset.NewForConfigOrDie(config), make(chan struct{}), commonFlag.VpaObjectNamespace),
so we probably don't need to do this here and filtering for the ignored namespaces is enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can keep using slices.Contains as it was, but personally, I’m not a fan of duplicated code (since shouldCleanupNamespace does the same thing).

Do you have a preference? Should we rename the function, or are we okay with some duplication?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with renaming the method and reduce duplication. We could also re-use the same method then for filtering the VPACheckpoints returned by the lister introduced in #6419

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 694cce8

klog.V(6).InfoS("Ignoring vpaCRD as this namespace is ignored", "vpaCRD", klog.KObj(vpaCRD))
continue
}
Expand Down
1 change: 1 addition & 0 deletions vertical-pod-autoscaler/pkg/recommender/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ func run(healthCheck *metrics.HealthCheck, commonFlag *common.CommonFlags) {
ControllerFetcher: controllerFetcher,
RecommenderName: *recommenderName,
IgnoredNamespaces: ignoredNamespaces,
VpaObjectNamespace: commonFlag.VpaObjectNamespace,
}.Make()
controllerFetcher.Start(context.Background(), scaleCacheLoopPeriod)

Expand Down
Loading