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

Add mountpoint to mountstats collector. #2676

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

Conversation

dsnt02518
Copy link

I think this will address #2097

This is also useful for Hammerspace where the same 'device' may be mounted via the same protocol to differing locations, but is actually still a unique mount.

As per @SuperQ 's comment in #2097 , only the first unique (Device, Protocol, MountAddress, MountPoint) will be returned, as per the existing logic with (Device, Protocol, MountAddress).

@discordianfish
Copy link
Member

You need to sign-off and fix the tests

@dsnt02518
Copy link
Author

A

You need to sign-off and fix the tests

Apologies - missed the make usage instead of make test - will get that fixed up.

This is useful for Hammerspace where the same 'device' may be mounted
via the same protocol to differing locations, but is actually still a
unique mount.

Signed-off-by: Danny Smith <danny.j.smith@gmail.com>
@dsnt02518
Copy link
Author

Tests are fixed and passing, however they do slightly lose the meaning originally intended regarding the 'duplicates' since the extra mountpoint label means the /mnt/nfs/test-dupe mounts are no longer duplicates.
Let me know if that is considered an issue and I can attempt to re-work them.

@discordianfish
Copy link
Member

Yeah good point re/ dups. I guess with the mountpoint there can't be dups anymore? @SuperQ remember the context?

@SuperQ
Copy link
Member

SuperQ commented May 11, 2023

Yea, I'm not sure we want to do it this way. The mountpoint, while useful, is going to lead to a lot of duplicate cardinality for some users.

For example, NFS mounted homedirs with an automount for each user. It could be the same device for each mount point for /home/foo and /home/bar.

The real question I have is, in the real world, are the metrics per mountpoint different? If they are, it would be a compelling case to add the label.

Otherwise, perhaps a node_nfs_mountpoint_info metric which would limit the M:N cardinality would be better.

I don't really have access to this kind of setup anymore to test.

@discordianfish
Copy link
Member

The real question I have is, in the real world, are the metrics per mountpoint different? If they are, it would be a compelling case to add the label.

Yeah thats the key question. If yes, let's to this here. If not, let's use node_nfs_mountpoint_info

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

@dsnt02518 Would you mind providing some real-world example mounstats output here? Feel free to redact any private data, but the real-world metrics data would be useful to see.

@eugene-eeo
Copy link

@SuperQ one datapoint is mounting the same NFS volume using nosharecache to get per-container NFS client metrics; currently this means that due to using .Device as a key we lose that information -- the alternative is for us to use procfs library ourselves.

the point on cardinality is still valid though, so maybe this is best hidden behind a flag.

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.

4 participants