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.

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

enhancement(node_exporter): use systemd to create node_exporter_textfile_dir if it doesn't exist #323

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ym
Copy link

@ym ym commented Mar 23, 2024

We're currently using a directory inside tmpfs for textfile collectors to reduce possible unnecessary IO usage, and the directory will be removed after a reboot. This PR is to ensure that node_exporter_textfile_dir exists when node-exporter is started.

@github-actions github-actions bot added enhancement New feature or request roles/node_exporter labels Mar 23, 2024
@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Mar 23, 2024
…ile_dir if it

doesn't exist

Signed-off-by: Siyuan Miao <[email protected]>
@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Mar 24, 2024
{% if node_exporter_textfile_dir | length > 0 %}
{% if (ansible_facts.packages.systemd | first).version is version('235', '>=') %}
ExecStartPre=+/bin/mkdir -p {{ node_exporter_textfile_dir }}
ExecStartPre=+/bin/chown -R {{ node_exporter_system_user }}:{{ node_exporter_system_group }} {{ node_exporter_textfile_dir }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I wonder if this should be -R. I'm not sure we want all of the prom files changed, just the dir.

Copy link
Author

Choose a reason for hiding this comment

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

In our environment, the text file exporters are started after node_exporter.service and the directory is used exclusively for text file exporters, so it doesn't really matter, but I think we can remove the -R, or should we go further and only change the owner if the directory is empty?

@SuperQ SuperQ requested a review from gardar March 24, 2024 15:30
Copy link
Member

@gardar gardar left a comment

Choose a reason for hiding this comment

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

I wonder if using systemd tmpfiles in this scenario would be a better solution? https://www.freedesktop.org/software/systemd/man/latest/tmpfiles.d.html

@gardar
Copy link
Member

gardar commented Oct 17, 2024

I wonder if using systemd tmpfiles in this scenario would be a better solution? https://www.freedesktop.org/software/systemd/man/latest/tmpfiles.d.html

ping @ym

@ym
Copy link
Author

ym commented Oct 17, 2024

I wonder if using systemd tmpfiles in this scenario would be a better solution? https://www.freedesktop.org/software/systemd/man/latest/tmpfiles.d.html

ping @ym

External scripts will also need to write to node_exporter_textfile_dir, so I think a predictable and static path might be a better idea. But if you have any good suggestions, I'm all ears.

@gardar
Copy link
Member

gardar commented Oct 17, 2024

The path is not dynamic like what you get with mktemp, you define the path you wish to be created in a config file under tmpfiles.d and then systemd will take care of creating it.

@@ -8,6 +8,16 @@ After=network-online.target
Type=simple
User={{ node_exporter_system_user }}
Group={{ node_exporter_system_group }}
{% if node_exporter_textfile_dir | length > 0 %}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would affect the common user who does not use tmpfs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants