opentelemetry-helm-charts icon indicating copy to clipboard operation
opentelemetry-helm-charts copied to clipboard

[collector] add node ip to kubeletstats

Open alextricity25 opened this issue 1 year ago • 6 comments

In some scenarios, it's desirable to use the node IP instead of the node name to gather kubelet metrics. This commit makes it possible for the user to specify the presets.kubeletMetrics.useNodeIp flag so that the node ip is used as an endpoint instead of the node name.

Somewhat resovles https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/22843

alextricity25 avatar May 30 '24 16:05 alextricity25

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: alextricity25 / name: Alex Cantu (8639fa7154055703f979d8bff1ac6f7d3a47ff25, 8cfed7d7c2e38c01554d10247d6726993d7e2450, 42758ca06d0c9f9748885887fc18e0d014e171ce, d89f00c83918e1ca712590bec0739f5c6bc91b5a, a6fc544a1b513bdb5135b5321d3fb650a380549f, 7972f9ae9c47cb59eff78ce0ad57899ec341b9f2)
  • :white_check_mark: login: TylerHelmuth / name: Tyler Helmuth (79d2127d34ebfdebac330a25a792c36e65ee75ec)

Instead of making this an option I'd be ok with switching to using Node IP in the preset instead of Node Name.

I'm actually surprised we're using node name instead of node ip as I'd expect IP to be more universally usable. We've seen issues like the one linked where IP is the solution, we could just use it all the time. I don't think it would break users.

@dmitryax @pavolloffay @jinja2 what do you think?

TylerHelmuth avatar May 30 '24 17:05 TylerHelmuth

Yeah should be okay to switch to node ip. It might be a breaking change for setups in which kubelet is not serving a certificate with the IP listed in subject alt name. Users will need to add the insecure_skip_verify: true config for such situations.

jinja2 avatar May 30 '24 23:05 jinja2

It might be a breaking change for setups in which kubelet is not serving a certificate with the IP listed in subject alt name

@jinja2 how common would this be?

TylerHelmuth avatar May 31 '24 16:05 TylerHelmuth

@TylerHelmuth Done! Latest commit removes useNodeIp and now exclusively uses K8S_NODE_IP for the kubeMetrics endpoint.

alextricity25 avatar Jun 04 '24 00:06 alextricity25

Error: INSTALLATION FAILED: DaemonSet.apps "opentelemetry-collector-z9u7pzlfno-agent" is invalid: spec.template.spec.containers[0].env[2].valueFrom.fieldRef.fieldPath: Invalid value: "spec.hostIp": error converting fieldPath: field label not supported: spec.hostIp

@alextricity25 looks like something with your use of the downward api is incorrect

TylerHelmuth avatar Jun 21 '24 16:06 TylerHelmuth

@alextricity25 you will need to rebase as well :).

ChrsMark avatar Jul 01 '24 14:07 ChrsMark

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Jul 16 '24 02:07 github-actions[bot]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

github-actions[bot] avatar Jul 31 '24 02:07 github-actions[bot]

Hi all! 👋 Is there a chance this could still happen? I'm happy to help if something is still missing.

legoGoat avatar Dec 11 '24 15:12 legoGoat

@legoGoat feel free to open a new PR

TylerHelmuth avatar Dec 12 '24 16:12 TylerHelmuth