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

Jump to bottom

[prometheus-smartctl-exporter] Don't use hostNetwork #4964

Merged
merged 5 commits into from
Nov 5, 2024
Merged

[prometheus-smartctl-exporter] Don't use hostNetwork #4964

merged 5 commits into from
Nov 5, 2024
+5 −3

Conversation

judahrand
Copy link
Contributor

@judahrand judahrand commented Nov 3, 2024 •
edited
Loading

What this PR does / why we need it

I can't see any reason that this exporter requires hostNetwork: true. It should only be looking at local devices. This also means that issues with port usage can occur when also using node-exporter which does need hostNetwork. I think it'd be best to just just remove the line as this PR does.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Special notes for your reviewer

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

@judahrand judahrand changed the title [smartctl-exporter] Don't use hostNetwork [prometheus-smartctl-exporter] Don't use hostNetwork Nov 3, 2024
@judahrand
Don't use hostNetwork
3656952 
Signed-off-by: GitHub <noreply@github.com>
@judahrand
Bump chart version
3953ba4 
Signed-off-by: GitHub <noreply@github.com>
@kfox1111
Copy link
Contributor

kfox1111 commented Nov 3, 2024

Depends on if prometheus is inside or outside the k8s network....

I think it probably should be made configurable then, rather then just remove it entirely.

@judahrand
Copy link
Contributor Author

Depends on if prometheus is inside or outside the k8s network....

I think it probably should be made configurable then, rather then just remove it entirely.

If Prometheus is inside the cluster then couldn't/shouldn't the exporter be exposed via an ingress (or loadbalancer)?

Either way if you think that this should be configurable then I'll switch to that. Maybe best to keep the current default and then just bump a patch version?

@judahrand
Make hostNetwork configurable
a8cd375 
Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>
kfox1111
kfox1111 approved these changes Nov 3, 2024
View reviewed changes
Copy link
Contributor

@kfox1111 kfox1111 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! :)

@kfox1111
Copy link
Contributor

kfox1111 commented Nov 3, 2024

If Prometheus is inside the cluster then couldn't/shouldn't the exporter be exposed via an ingress (or loadbalancer)?

ingress would expose them load balanced. You couldn't predictably target all of them for scraping.

Either way if you think that this should be configurable then I'll switch to that. Maybe best to keep the current default and then just bump a patch version?

hmmm... Yeah, I guess if its a new default, it may be better to bump the minor version, or leave it as a patch version and keep it with the old default. Either way is ok I think.

zeritti
zeritti previously approved these changes Nov 4, 2024
View reviewed changes
Copy link
Member

@zeritti zeritti left a comment

Choose a reason for hiding this comment

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

Thank you, @judahrand, for your PR. LGTM. I'd consider this change a feature rather than a patch, though.

@judahrand
Bump minor version instead
a556e47 
Signed-off-by: GitHub <noreply@github.com>
@judahrand
Copy link
Contributor Author

Thank you, @judahrand, for your PR. LGTM. I'd consider this change a feature rather than a patch, though.

I've updated the PR to bump a minor version instead so I think this should be good to go?

@zeritti zeritti merged commit d0cbf1b into prometheus-community:main Nov 5, 2024
4 checks passed
kranthikirang pushed a commit to kranthikirang/helm-charts that referenced this pull request Nov 5, 2024
@judahrand @kranthikirang
[prometheus-smartctl-exporter] Don't use hostNetwork (prometheus-comm…
8a8c37d 
…unity#4964)

* Don't use `hostNetwork`

Signed-off-by: GitHub <noreply@github.com>

* Bump chart version

Signed-off-by: GitHub <noreply@github.com>

* Make `hostNetwork` configurable

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

* Bump minor version instead

Signed-off-by: GitHub <noreply@github.com>

---------

Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfox1111 kfox1111 kfox1111 approved these changes

@zeritti zeritti zeritti approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[prometheus-smartctl-exporter] HostNetwork defaults to true
3 participants
@judahrand @kfox1111 @zeritti