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

Added MemoryCurrent and MemoryLimit metrics to systemd #1036

Closed
wants to merge 1 commit into from

Conversation

jameshartig
Copy link
Contributor

@jameshartig jameshartig commented Aug 6, 2018

Since systemd reports memory usage and memory limits, I added them to the systemd collector. This would be very useful for us since we recently had node_exporter use up all of the memory on our servers and without per-service metrics we couldn't quickly tell what service was using all of the memory. This would additionally help us in fine-tuning our per-service memory limits.

Unfortunately when I was running this on our servers systemd reported some services as having maxuint64 memory usage so I added a check to ignore those. It's unclear why it's doing that and whether it's a bug or not. Also, some of our servers running CentOS don't have a new enough systemd to report NRestarts so I didn't make it ignore services that couldn't report that.

@SuperQ @discordianfish

@SuperQ
Copy link
Member

SuperQ commented Aug 6, 2018

I wonder if max uint64 is when the container is unlimited.

@jameshartig
Copy link
Contributor Author

@SuperQ I'm not sure what the condition is but here's an example:

busctl introspect :1.1 /org/freedesktop/systemd1/unit/rescue_2eservice | grep Memory
.MemoryAccounting                   property  b              false                                    -
.MemoryCurrent                      property  t              18446744073709551615                     -
.MemoryLimit                        property  t              18446744073709551615                     -

It only seems to happen for system services (rescue, rsyslog, firewalld, etc). Maybe it's something with permissions? Looking at the service files for some of those doesn't seem to indicate anything. firewalld is Type=dbus which I thought was maybe it, but rsyslog is Type=notify so I'm not sure.

@SuperQ
Copy link
Member

SuperQ commented Aug 6, 2018

Ahh, I'm guessing it's because accounting is disabled for those services. If MemoryAccounting is false, we should probably not return any metrics for those units.

@jameshartig
Copy link
Contributor Author

@SuperQ I thought that as well... but it seems to be always false:

$ busctl introspect :1.1 /org/freedesktop/systemd1/unit/hoothoot_2eservice | grep Memory
.MemoryAccounting                   property  b              false                                    -
.MemoryCurrent                      property  t              15122432                                 -
.MemoryLimit                        property  t              2909798400                               -

That service has memory limits defined and systemd is tracking the memory usage correctly but MemoryAccounting is still false...

@brian-brazil
Copy link
Contributor

Might this already be covered by cAdvisor?

@jameshartig
Copy link
Contributor Author

@brian-brazil Doesn't cAdvisor only work for containers?

@brian-brazil
Copy link
Contributor

Systemd runs services in containers, and this is presumably how this is implemented.

@jameshartig
Copy link
Contributor Author

@brian-brazil I didn't realize that cAdvisor supported non-docker containers, is what I meant. I'll try out cAdvisor and report back. I presume that if it offers the ability to get these memory metrics then I should close this?

@juliusv
Copy link
Member

juliusv commented Aug 6, 2018

@fastest963 cAdvisor just reads cgroup information from /sys/fs/cgroup. Both Docker and systemd use cgroups for containerization. cAdvisor will definitely have the current/max memory metrics.

No opinion on whether this should also be added to the systemd collector.

Signed-off-by: James Hartig <james@getadmiral.com>
@discordianfish
Copy link
Member

@fastest963 I think it depends a bit on how well this works. I'm not completely opposed to adding this PR, but if it's straight forward to use cadvisor instead, we should prefer that way. Did you get to trying this out?

@jameshartig
Copy link
Contributor Author

@discordianfish I had success just installing cadvisor and getting it hooked up. I'll close this. Thanks for the tip about cadvisor!

@jameshartig jameshartig deleted the memory branch August 19, 2018 02:54
@pgier pgier mentioned this pull request Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants