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

vpa-recommender: Log object's namespace #6909

Merged
merged 2 commits into from
Jun 28, 2024
Merged

vpa-recommender: Log object's namespace #6909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
628fc39
vpa-recommender: Log object's namespace
ialidzhikov Jun 10, 2024
1739a71
Use klog.KRef wherever possible
ialidzhikov Jun 19, 2024
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/pkg/recommender/checkpoint/checkpoint_writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func getVpasToCheckpoint(clusterVpas map[model.VpaID]*model.Vpa) []*model.Vpa {
vpas := make([]*model.Vpa, 0, len(clusterVpas))
for _, vpa := range clusterVpas {
if isFetchingHistory(vpa) {
klog.V(3).Infof("VPA %s/%s is loading history, skipping checkpoints", vpa.ID.Namespace, vpa.ID.VpaName)
klog.V(3).Infof("VPA %s is loading history, skipping checkpoints", klog.KRef(vpa.ID.Namespace, vpa.ID.VpaName))
continue
}
vpas = append(vpas, vpa)
Expand Down Expand Up @@ -94,7 +94,7 @@ func (writer *checkpointWriter) StoreCheckpoints(ctx context.Context, now time.T
for container, aggregatedContainerState := range aggregateContainerStateMap {
containerCheckpoint, err := aggregatedContainerState.SaveToCheckpoint()
if err != nil {
klog.Errorf("Cannot serialize checkpoint for vpa %v container %v. Reason: %+v", vpa.ID.VpaName, container, err)
klog.Errorf("Cannot serialize checkpoint for vpa %s/%s container %v. Reason: %+v", vpa.ID.Namespace, vpa.ID.VpaName, container, err)
continue
}
checkpointName := fmt.Sprintf("%s-%s", vpa.ID.VpaName, container)
Expand Down
22 changes: 11 additions & 11 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 @@ -42,7 +42,7 @@ import (
"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"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher"
controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher"
metrics_recommender "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/metrics/recommender"
)

Expand Down Expand Up @@ -229,13 +229,13 @@ func (feeder *clusterStateFeeder) setVpaCheckpoint(checkpoint *vpa_types.Vertica
vpaID := model.VpaID{Namespace: checkpoint.Namespace, VpaName: checkpoint.Spec.VPAObjectName}
vpa, exists := feeder.clusterState.Vpas[vpaID]
if !exists {
return fmt.Errorf("cannot load checkpoint to missing VPA object %+v", vpaID)
return fmt.Errorf("cannot load checkpoint to missing VPA object %s/%s", vpa.ID.Namespace, vpa.ID.VpaName)
ialidzhikov marked this conversation as resolved.
Show resolved Hide resolved
}

cs := model.NewAggregateContainerState()
err := cs.LoadFromCheckpoint(&checkpoint.Status)
if err != nil {
return fmt.Errorf("cannot load checkpoint for VPA %+v. Reason: %v", vpa.ID, err)
return fmt.Errorf("cannot load checkpoint for VPA %s/%s. Reason: %v", vpa.ID.Namespace, vpa.ID.VpaName, err)
}
vpa.ContainersInitialAggregateState[checkpoint.Spec.ContainerName] = cs
return nil
Expand All @@ -254,7 +254,7 @@ func (feeder *clusterStateFeeder) InitFromCheckpoints() {
klog.V(3).Infof("Fetching checkpoints from namespace %s", namespace)
checkpointList, err := feeder.vpaCheckpointClient.VerticalPodAutoscalerCheckpoints(namespace).List(context.TODO(), metav1.ListOptions{})
if err != nil {
klog.Errorf("Cannot list VPA checkpoints from namespace %v. Reason: %+v", namespace, err)
klog.Errorf("Cannot list VPA checkpoints from namespace %s. Reason: %+v", namespace, err)
}
for _, checkpoint := range checkpointList.Items {

Expand Down Expand Up @@ -319,16 +319,16 @@ func filterVPAs(feeder *clusterStateFeeder, allVpaCRDs []*vpa_types.VerticalPodA
for _, vpaCRD := range allVpaCRDs {
if feeder.recommenderName == DefaultRecommenderName {
if !implicitDefaultRecommender(vpaCRD.Spec.Recommenders) && !selectsRecommender(vpaCRD.Spec.Recommenders, &feeder.recommenderName) {
klog.V(6).Infof("Ignoring vpaCRD %s in namespace %s as current recommender's name %v doesn't appear among its recommenders", vpaCRD.Name, vpaCRD.Namespace, feeder.recommenderName)
klog.V(6).Infof("Ignoring vpaCRD %s as current recommender's name %v doesn't appear among its recommenders", klog.KObj(vpaCRD), feeder.recommenderName)
continue
}
} else {
if implicitDefaultRecommender(vpaCRD.Spec.Recommenders) {
klog.V(6).Infof("Ignoring vpaCRD %s in namespace %s as %v recommender doesn't process CRDs implicitly destined to %v recommender", vpaCRD.Name, vpaCRD.Namespace, feeder.recommenderName, DefaultRecommenderName)
klog.V(6).Infof("Ignoring vpaCRD %s as %v recommender doesn't process CRDs implicitly destined to %v recommender", klog.KObj(vpaCRD), feeder.recommenderName, DefaultRecommenderName)
kwiesmueller marked this conversation as resolved.
Show resolved Hide resolved
continue
}
if !selectsRecommender(vpaCRD.Spec.Recommenders, &feeder.recommenderName) {
klog.V(6).Infof("Ignoring vpaCRD %s in namespace %s as current recommender's name %v doesn't appear among its recommenders", vpaCRD.Name, vpaCRD.Namespace, feeder.recommenderName)
klog.V(6).Infof("Ignoring vpaCRD %s as current recommender's name %v doesn't appear among its recommenders", klog.KObj(vpaCRD), feeder.recommenderName)
continue
}
}
Expand Down Expand Up @@ -359,7 +359,7 @@ func (feeder *clusterStateFeeder) LoadVPAs() {
}

selector, conditions := feeder.getSelector(vpaCRD)
klog.V(4).Infof("Using selector %s for VPA %s/%s", selector.String(), vpaCRD.Namespace, vpaCRD.Name)
Copy link
Member

Choose a reason for hiding this comment

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

For some lines above you added that they are namespace/name. Is there a specific rule when to print namespacedName vs. the whole resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not printing the whole resource. As explained in #6909 (comment) klog.KObj only prints <namespace>/<name>.

If you are asking why in some places we use klog.KObj and in some places we have to use %s/%s and pass the namespace and name: KObj accepts the interface KMetadata:

autoscaler/vertical-pod-autoscaler/vendor/k8s.io/klog/v2/k8s_references.go

Lines 72 to 75 in f03e98f

type KMetadata interface {
GetName() string
GetNamespace() string
}
. Structs like

autoscaler/vertical-pod-autoscaler/pkg/recommender/model/types.go

Lines 160 to 163 in 8bc327e

type VpaID struct {
Namespace string
VpaName string
}
and

autoscaler/vertical-pod-autoscaler/pkg/recommender/model/types.go

Lines 144 to 150 in 8bc327e

// PodID contains information needed to identify a Pod within a cluster.
type PodID struct {
// Namespaces where the Pod is defined.
Namespace string
// PodName is the name of the pod unique within a namespace.
PodName string
}
don't implement that interface and that's why I am using %s/%s for them.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks. I wonder if it's worth implementing that interface on those types instead for consistency. But I'm good with this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if it's worth implementing that interface on those types instead for consistency. But I'm good with this too.

I discovered klog.KRef and adapted the VpaID/PodID usages to it: see 1739a71

klog.V(4).Infof("Using selector %s for VPA %s", selector.String(), klog.KObj(vpaCRD))

if feeder.clusterState.AddOrUpdateVpa(vpaCRD, selector) == nil {
// Successfully added VPA to the model.
Expand All @@ -377,9 +377,9 @@ func (feeder *clusterStateFeeder) LoadVPAs() {
// Delete non-existent VPAs from the model.
for vpaID := range feeder.clusterState.Vpas {
if _, exists := vpaKeys[vpaID]; !exists {
klog.V(3).Infof("Deleting VPA %v", vpaID)
klog.V(3).Infof("Deleting VPA %s", klog.KRef(vpaID.Namespace, vpaID.VpaName))
if err := feeder.clusterState.DeleteVpa(vpaID); err != nil {
klog.Errorf("Deleting VPA %v failed: %v", vpaID, err)
klog.Errorf("Deleting VPA %s failed: %v", klog.KRef(vpaID.Namespace, vpaID.VpaName), err)
}
}
}
Expand All @@ -398,7 +398,7 @@ func (feeder *clusterStateFeeder) LoadPods() {
}
for key := range feeder.clusterState.Pods {
if _, exists := pods[key]; !exists {
klog.V(3).Infof("Deleting Pod %v", key)
klog.V(3).Infof("Deleting Pod %s", klog.KRef(key.Namespace, key.PodName))
feeder.clusterState.DeletePod(key)
}
}
Expand Down
7 changes: 4 additions & 3 deletions vertical-pod-autoscaler/pkg/recommender/input/metrics/metrics_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package metrics

import (
"context"
"time"

k8sapiv1 "k8s.io/api/core/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
Expand All @@ -28,7 +30,6 @@ import (
"k8s.io/metrics/pkg/apis/metrics/v1beta1"
resourceclient "k8s.io/metrics/pkg/client/clientset/versioned/typed/metrics/v1beta1"
"k8s.io/metrics/pkg/client/external_metrics"
"time"
)

// PodMetricsLister wraps both metrics-client and External Metrics
Expand Down Expand Up @@ -113,10 +114,10 @@ func (s *externalMetricsClient) List(ctx context.Context, namespace string, opts
return nil, err
}
if m == nil || len(m.Items) == 0 {
klog.V(4).Infof("External Metrics Query for VPA %+v: resource %+v, metric %+v, No items,", vpa.ID, resourceName, metricName)
klog.V(4).Infof("External Metrics Query for VPA %s: resource %+v, metric %+v, No items,", klog.KRef(vpa.ID.Namespace, vpa.ID.VpaName), resourceName, metricName)
Copy link
Member

Choose a reason for hiding this comment

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

Just a sanity check as I can't dive deeper on my current device. Can ID be nil?

/hold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In

autoscaler/vertical-pod-autoscaler/pkg/recommender/model/cluster.go

Line 248 in 628fc39

func (cluster *ClusterState) AddOrUpdateVpa(apiObject *vpa_types.VerticalPodAutoscaler, selector labels.Selector) error {
it seems that vpaID is always set to Vpa model.

If vpa.ID is nil, it would panic earlier at

autoscaler/vertical-pod-autoscaler/pkg/recommender/input/metrics/metrics_source.go

Line 105 in 1739a71

ObjectMeta: v1.ObjectMeta{Namespace: vpa.ID.Namespace, Name: pod.PodName},
.

/hold cancel

continue
}
klog.V(4).Infof("External Metrics Query for VPA %+v: resource %+v, metric %+v, %d items, item[0]: %+v", vpa.ID, resourceName, metricName, len(m.Items), m.Items[0])
klog.V(4).Infof("External Metrics Query for VPA %s: resource %+v, metric %+v, %d items, item[0]: %+v", klog.KRef(vpa.ID.Namespace, vpa.ID.VpaName), resourceName, metricName, len(m.Items), m.Items[0])
podMets.Timestamp = m.Items[0].Timestamp
if m.Items[0].WindowSeconds != nil {
podMets.Window = v1.Duration{Duration: time.Duration(*m.Items[0].WindowSeconds) * time.Second}
Expand Down
6 changes: 3 additions & 3 deletions vertical-pod-autoscaler/pkg/recommender/model/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
"k8s.io/klog/v2"

vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher"
controllerfetcher "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/controller_fetcher"
vpa_utils "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa"
)

Expand Down Expand Up @@ -329,7 +329,7 @@ func (cluster *ClusterState) MakeAggregateStateKey(pod *PodState, containerName
func (cluster *ClusterState) aggregateStateKeyForContainerID(containerID ContainerID) AggregateStateKey {
pod, podExists := cluster.Pods[containerID.PodID]
if !podExists {
panic(fmt.Sprintf("Pod not present in the ClusterState: %v", containerID.PodID))
panic(fmt.Sprintf("Pod not present in the ClusterState: %s/%s", containerID.PodID.Namespace, containerID.PodID.PodName))
}
return cluster.MakeAggregateStateKey(pod, containerID.ContainerName)
}
Expand Down Expand Up @@ -433,7 +433,7 @@ func (cluster *ClusterState) RecordRecommendation(vpa *Vpa, now time.Time) error
} else {
if lastLogged.Add(RecommendationMissingMaxDuration).Before(now) {
cluster.EmptyVPAs[vpa.ID] = now
return fmt.Errorf("VPA %v/%v is missing recommendation for more than %v", vpa.ID.Namespace, vpa.ID.VpaName, RecommendationMissingMaxDuration)
return fmt.Errorf("VPA %s/%s is missing recommendation for more than %v", vpa.ID.Namespace, vpa.ID.VpaName, RecommendationMissingMaxDuration)
}
}
return nil
Expand Down
2 changes: 1 addition & 1 deletion vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (c CappingPostProcessor) Process(vpa *vpa_types.VerticalPodAutoscaler, reco
// TODO: maybe rename the vpa_utils.ApplyVPAPolicy to something that mention that it is doing capping only
cappedRecommendation, err := vpa_utils.ApplyVPAPolicy(recommendation, vpa.Spec.ResourcePolicy)
if err != nil {
klog.Errorf("Failed to apply policy for VPA %v/%v: %v", vpa.GetNamespace(), vpa.GetName(), err)
klog.Errorf("Failed to apply policy for VPA %s: %v", klog.KObj(vpa), err)
return recommendation
}
return cappedRecommendation
Expand Down
6 changes: 3 additions & 3 deletions vertical-pod-autoscaler/pkg/recommender/routines/recommender.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/logic"
"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"
metrics_recommender "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/metrics/recommender"
vpa_utils "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa"
)
Expand Down Expand Up @@ -116,7 +116,7 @@ func (r *recommender) UpdateVPAs() {
pods := r.clusterState.GetMatchingPods(vpa)
klog.Infof("MatchingPods: %+v", pods)
if len(pods) != vpa.PodCount {
klog.Errorf("ClusterState pod count and matching pods disagree for vpa %v/%v", vpa.ID.Namespace, vpa.ID.VpaName)
klog.Errorf("ClusterState pod count and matching pods disagree for VPA %s", klog.KRef(vpa.ID.Namespace, vpa.ID.VpaName))
}
}
}
Expand All @@ -126,7 +126,7 @@ func (r *recommender) UpdateVPAs() {
r.vpaClient.VerticalPodAutoscalers(vpa.ID.Namespace), vpa.ID.VpaName, vpa.AsStatus(), &observedVpa.Status)
if err != nil {
klog.Errorf(
"Cannot update VPA %v/%v object. Reason: %+v", vpa.ID.Namespace, vpa.ID.VpaName, err)
"Cannot update VPA %s object. Reason: %+v", klog.KRef(vpa.ID.Namespace, vpa.ID.VpaName), err)
}
}
}
Expand Down
Loading