vpa-recommender: Log object's namespace #6909

ialidzhikov · 2024-06-10T13:21:14Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Similar to #6903 but for the vpa-recommender. The Pod/VPA resources are namespaced and logging only the name does not allow these objections to be unambiguously identified in the cluster.

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

vpa-recommender logging is now improved. In many cases it now logs the objects' namespaces as well.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go

adrianmoisey · 2024-06-11T12:20:42Z

Not a maintainer: but this PR looks good to me. I think the additional namespaces will help in the logs.

adrianmoisey · 2024-06-11T12:21:09Z

Does this PR introduce a user-facing change?

Does it make sense to add a change log entry for this? I think it's useful for users to know that logs have improved if this lands.

ialidzhikov · 2024-06-11T13:33:34Z

Does it make sense to add a change log entry for this? I think it's useful for users to know that logs have improved if this lands.

Done.

adrianmoisey · 2024-06-13T06:58:22Z

Sorry, I can't approve, I'm not an approver for the project. Just someone trying to get involved.

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go

kwiesmueller · 2024-06-13T19:08:55Z

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go

@@ -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)


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

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.

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.

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

raywainman · 2024-06-14T18:51:10Z

/lgtm

Thanks for making this consistent!

kwiesmueller · 2024-06-14T20:19:03Z

/lgtm
/hold
Feel free to remove hold if you don't want to implement the KMetadata interface on relevant types, I'm good either way.

ialidzhikov · 2024-06-19T13:25:24Z

/hold cancel
see #6909 (comment)

ialidzhikov · 2024-06-27T08:53:43Z

@kwiesmueller could you have a look when you have time? Thanks in advance!

kwiesmueller

/lgtm
/approve

Left a last safety question, otherwise great.
If the answer to my question is no, please just unhold.

/hold

kwiesmueller · 2024-06-27T11:58:05Z

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

@@ -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)


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

/hold

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

k8s-ci-robot · 2024-06-27T11:59:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ialidzhikov, kwiesmueller

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~vertical-pod-autoscaler/OWNERS~~ [kwiesmueller]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vpa-recommender: Log object's namespace

628fc39

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 10, 2024

k8s-ci-robot requested a review from kgolab June 10, 2024 13:21

k8s-ci-robot added the area/vertical-pod-autoscaler label Jun 10, 2024

k8s-ci-robot requested a review from kwiesmueller June 10, 2024 13:21

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 10, 2024

adrianmoisey reviewed Jun 11, 2024

View reviewed changes

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go Show resolved Hide resolved

ialidzhikov requested a review from adrianmoisey June 13, 2024 06:55

kwiesmueller reviewed Jun 13, 2024

View reviewed changes

ialidzhikov requested a review from kwiesmueller June 14, 2024 08:13

k8s-ci-robot assigned raywainman Jun 14, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2024

k8s-ci-robot assigned kwiesmueller Jun 14, 2024

Use klog.KRef wherever possible

1739a71

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 19, 2024

kwiesmueller reviewed Jun 27, 2024

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 27, 2024

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 27, 2024

k8s-ci-robot merged commit d0441f7 into kubernetes:master Jun 28, 2024
7 checks passed

ialidzhikov deleted the enh/recommender-logging branch June 29, 2024 17:04

raywainman mentioned this pull request Aug 6, 2024

Release VPA 1.2.0 #7098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vpa-recommender: Log object's namespace #6909

vpa-recommender: Log object's namespace #6909

ialidzhikov commented Jun 10, 2024 •

edited

Loading

adrianmoisey commented Jun 11, 2024 •

edited

Loading

adrianmoisey commented Jun 11, 2024

ialidzhikov commented Jun 11, 2024

adrianmoisey commented Jun 13, 2024

kwiesmueller Jun 13, 2024

ialidzhikov Jun 14, 2024

kwiesmueller Jun 14, 2024

ialidzhikov Jun 19, 2024

raywainman commented Jun 14, 2024

kwiesmueller commented Jun 14, 2024

ialidzhikov commented Jun 19, 2024

ialidzhikov commented Jun 27, 2024

kwiesmueller left a comment

kwiesmueller Jun 27, 2024

ialidzhikov Jun 28, 2024

k8s-ci-robot commented Jun 27, 2024

	type KMetadata interface {
	GetName() string
	GetNamespace() string
	}

	// 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
	}

vpa-recommender: Log object's namespace #6909

vpa-recommender: Log object's namespace #6909

Conversation

ialidzhikov commented Jun 10, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

adrianmoisey commented Jun 11, 2024 • edited Loading

adrianmoisey commented Jun 11, 2024

ialidzhikov commented Jun 11, 2024

adrianmoisey commented Jun 13, 2024

kwiesmueller Jun 13, 2024

Choose a reason for hiding this comment

ialidzhikov Jun 14, 2024

Choose a reason for hiding this comment

kwiesmueller Jun 14, 2024

Choose a reason for hiding this comment

ialidzhikov Jun 19, 2024

Choose a reason for hiding this comment

raywainman commented Jun 14, 2024

kwiesmueller commented Jun 14, 2024

ialidzhikov commented Jun 19, 2024

ialidzhikov commented Jun 27, 2024

kwiesmueller left a comment

Choose a reason for hiding this comment

kwiesmueller Jun 27, 2024

Choose a reason for hiding this comment

ialidzhikov Jun 28, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 27, 2024

ialidzhikov commented Jun 10, 2024 •

edited

Loading

adrianmoisey commented Jun 11, 2024 •

edited

Loading