Allow attaching node metadata #10080

fpetkovski · 2021-12-23T11:06:54Z

This PR implements part of the functionality suggested in #9510.

In order to gather feedback and keep changes small, this commit only implements attaching metadata from nodes when the pod role is used for Kubernetes SD.

fpetkovski · 2021-12-23T11:08:54Z

discovery/kubernetes/pod.go

@@ -87,7 +109,12 @@ func (p *Pod) enqueue(obj interface{}) {
 func (p *Pod) Run(ctx context.Context, ch chan<- []*targetgroup.Group) {
 	defer p.queue.ShutDown()

-	if !cache.WaitForCacheSync(ctx.Done(), p.informer.HasSynced) {
+	cacheSyncs := []cache.InformerSynced{p.podInf.HasSynced}
+	if p.nodeInf != nil {


I don't know if it's a good idea to use the value of the informer as a discriminator of whether node metadata should be attached or not. Any feedback is welcome.

I think that deserves a comment at least, otherwise a config field on the pod role would be ok as well I think

I like the field on the pod role 👍

discovery/kubernetes/pod.go

fpetkovski · 2022-01-07T10:27:02Z

cc @rfratto

brancz

Love it!

discovery/kubernetes/pod.go

roidelapluie · 2022-01-25T12:42:50Z

docs/configuration/configuration.md

+attach_metadata:
+# Attaches node metadata to discovered targets. Only valid for role: pod. 
+# When set to true, Prometheus must have permissions to get Nodes. 
+[ node: <boolean> | default = false ]


Suggested change

[ node: <boolean> | default = false ]

[ node: <boolean> | default = false ]

fpetkovski · 2022-02-01T09:22:31Z

@brancz @roidelapluie would you mind taking another look?

simonpasquier

LGTM. My only remark is that the kube SD documentation needs an update to mention that the pod role supports node selectors when attach_metadata: {node: true}.

prometheus/docs/configuration/configuration.md

Lines 1706 to 1710 in afdd135

    
           # Optional label and field selectors to limit the discovery process to a subset of available resources. 
        
           # See https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/ 
        
           # and https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/ to learn more about the possible 
        
           # filters that can be used. Endpoints role supports pod, service and endpoints selectors, other roles 
        
           # only support selectors matching the role itself (e.g. node role can only contain node selectors).

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>

brancz · 2022-03-10T10:57:31Z

discovery/kubernetes/pod.go

+}
+
+func (p *Pod) enqueuePodsForNode(nodeName string) {
+	for _, pod := range p.store.List() {


Is there no way to index this? This seems like it could potentially be expensive in large clusters.

Thanks, this is a good point, and I think the solution should be straightforward. I added a new commit that converts the pod informer to an informer indexer.

brancz

Nice job! I'm ok with merging this as is even with my comment since this functionality can easily be turned off if it does turn out to be a performance problem.

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>

brancz

Very nice!

…s and annotations to discovered pod targets in the same way as Prometheus 2.35 does See prometheus/prometheus#9510 and prometheus/prometheus#10080

… if the corresponding node objects are missing This reflects the logic used in Prometheus. See prometheus/prometheus#10080

laurovenancio · 2022-05-25T22:32:05Z

@fpetkovski It seems to use node selectors with pods is not working. When I try to use it, I receive the following error:
"pod role supports only pod selectors".
The configuration validation is not allowing it.

fpetkovski · 2022-05-26T06:52:17Z

Can you please post a config example?

laurovenancio · 2022-05-26T13:21:03Z

Can you please post a config example?

kubernetes_sd_configs:
  - api_server: [...]
    role: pod
    kubeconfig_file: ""
    authorization:
      type: Bearer
      credentials_file: [...]
    tls_config:
      ca_file: [...]
      insecure_skip_verify: false
    follow_redirects: true
    enable_http2: true
    selectors:
    - role: node
      label: topology.kubernetes.io/zone=us-east-1f
    attach_metadata:
      node: true

fpetkovski · 2022-05-26T14:15:23Z

Does this configuration work when you remove the following snippet?

attach_metadata:
  node: true

fpetkovski · 2022-05-26T14:23:18Z

I believe there's even a test case to verify that node selectors for the pod role are not supported: https://github.com/prometheus/prometheus/blob/main/config/testdata/kubernetes_selectors_pod.bad.yml

If you would like to filter out pods from certain nodes, can you use relabel configs against the attached node metadata?

laurovenancio · 2022-05-26T15:22:22Z

If you would like to filter out pods from certain nodes, can you use relabel configs against the attached node metadata?

Filtering using relabel works fine but the selector would be more efficient.

As this PR added the following text to the documentation, I wonder if it is just the configuration validation that is failing this feature.

"The pod role supports node selectors when configured with attach_metadata: {node: true}"

fpetkovski · 2022-05-26T17:30:08Z

I see, so the docs say that this should be supported but validation fails.

Let me look into it and see whether we can support that easily, or we have to change the docs and potentially implement it in a follow up.

fpetkovski · 2022-05-27T06:49:40Z

I am looking at the behavior of using pod selectors under the endpoints role. Prometheus doesn't seem to use those pod selectors to filter out pods, but to rather not attach pod specific metadata from pods which do not match the specified selectors. So filtering still has to be done with relabel_configs.

Based on that, I would suggest that we change the validation to match what is specified in the docs and allow setting node selectors when node metadata is attached. In that specific case, the service discovery would only attach node metadata for nodes which match the selector.

Any thoughts on this @simonpasquier @brancz?

8BitSensei · 2022-10-21T10:47:46Z

Has the issue @laurovenancio raised, that is Prometheus reporting "pod role supports only pod selectors" when using a Node selector with a Pod role while attach_metadata is true, been resolved yet? It looks like the test case mentioned earlier is still in place (https://github.com/prometheus/prometheus/blob/main/config/testdata/kubernetes_selectors_pod.bad.yml)

fpetkovski requested a review from brancz as a code owner December 23, 2021 11:06

fpetkovski commented Dec 23, 2021

View reviewed changes

discovery/kubernetes/pod.go Show resolved Hide resolved

fpetkovski force-pushed the attach-target-metadata branch from 312cd19 to 4e3ae8a Compare December 23, 2021 11:09

brancz reviewed Jan 25, 2022

View reviewed changes

discovery/kubernetes/pod.go Show resolved Hide resolved

roidelapluie reviewed Jan 25, 2022

View reviewed changes

fpetkovski force-pushed the attach-target-metadata branch 2 times, most recently from ebcb7e1 to 434e720 Compare January 26, 2022 17:29

fpetkovski requested review from roidelapluie and brancz January 26, 2022 18:57

simonpasquier reviewed Feb 15, 2022

View reviewed changes

rfratto mentioned this pull request Mar 3, 2022

(Feature) PodLogs namespaceSelector add matchExpressions grafana/agent#1457

Open

Allow attaching node metadata

fa798d3

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>

fpetkovski force-pushed the attach-target-metadata branch from 96c305e to 608ce83 Compare March 10, 2022 07:41

Update docs

eb55125

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>

fpetkovski force-pushed the attach-target-metadata branch from 608ce83 to eb55125 Compare March 10, 2022 07:47

brancz reviewed Mar 10, 2022

View reviewed changes

brancz approved these changes Mar 10, 2022

View reviewed changes

Index pods by node name

16bd0d7

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>

fpetkovski force-pushed the attach-target-metadata branch from f8b0e15 to 16bd0d7 Compare March 10, 2022 14:35

brancz approved these changes Mar 17, 2022

View reviewed changes

brancz merged commit 1635dee into prometheus:main Mar 17, 2022

ArthurSens mentioned this pull request Mar 17, 2022

Consider replace relabeling with attach_node_metadata gitpod-io/observability#103

Closed

rongshengfang mentioned this pull request May 3, 2022

Kubernetes SD: Optionally attach namespace and node metadata to targets #9510

Open

fpetkovski mentioned this pull request May 19, 2022

Add support for attach_metadata into ServiceMonitor/PodMonitor prometheus-operator/prometheus-operator#4779

Closed

fpetkovski mentioned this pull request May 27, 2022

kubernetes_sd: Allow attaching node labels for endpoint role #10759

Merged

Allow attaching node metadata #10080

Allow attaching node metadata #10080

fpetkovski commented Dec 23, 2021

fpetkovski Dec 23, 2021

brancz Jan 25, 2022

fpetkovski Jan 26, 2022

fpetkovski commented Jan 7, 2022

brancz left a comment

roidelapluie Jan 25, 2022

fpetkovski commented Feb 1, 2022

simonpasquier left a comment

brancz Mar 10, 2022

fpetkovski Mar 10, 2022

brancz left a comment

brancz left a comment

laurovenancio commented May 25, 2022 •

edited

fpetkovski commented May 26, 2022

laurovenancio commented May 26, 2022

fpetkovski commented May 26, 2022 •

edited

fpetkovski commented May 26, 2022

laurovenancio commented May 26, 2022

fpetkovski commented May 26, 2022

fpetkovski commented May 27, 2022

8BitSensei commented Oct 21, 2022

	[ node: <boolean> \| default = false ]
	[ node: <boolean> \| default = false ]

	# Optional label and field selectors to limit the discovery process to a subset of available resources.
	# See https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/
	# and https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/ to learn more about the possible
	# filters that can be used. Endpoints role supports pod, service and endpoints selectors, other roles
	# only support selectors matching the role itself (e.g. node role can only contain node selectors).

Allow attaching node metadata #10080

Allow attaching node metadata #10080

Conversation

fpetkovski commented Dec 23, 2021

fpetkovski Dec 23, 2021

Choose a reason for hiding this comment

brancz Jan 25, 2022

Choose a reason for hiding this comment

fpetkovski Jan 26, 2022

Choose a reason for hiding this comment

fpetkovski commented Jan 7, 2022

brancz left a comment

Choose a reason for hiding this comment

roidelapluie Jan 25, 2022

Choose a reason for hiding this comment

fpetkovski commented Feb 1, 2022

simonpasquier left a comment

Choose a reason for hiding this comment

brancz Mar 10, 2022

Choose a reason for hiding this comment

fpetkovski Mar 10, 2022

Choose a reason for hiding this comment

brancz left a comment

Choose a reason for hiding this comment

brancz left a comment

Choose a reason for hiding this comment

laurovenancio commented May 25, 2022 • edited

fpetkovski commented May 26, 2022

laurovenancio commented May 26, 2022

fpetkovski commented May 26, 2022 • edited

fpetkovski commented May 26, 2022

laurovenancio commented May 26, 2022

fpetkovski commented May 26, 2022

fpetkovski commented May 27, 2022

8BitSensei commented Oct 21, 2022

laurovenancio commented May 25, 2022 •

edited

fpetkovski commented May 26, 2022 •

edited