Skip to content

Comments

Btrfs support#223

Merged
pgier merged 2 commits intoprometheus:masterfrom
silkeh:pr/btrfs-support
Oct 15, 2019
Merged

Btrfs support#223
pgier merged 2 commits intoprometheus:masterfrom
silkeh:pr/btrfs-support

Conversation

@silkeh
Copy link
Contributor

@silkeh silkeh commented Oct 9, 2019

This PR adds rudimentary Btrfs support. It is mainly a strict representation of the content that can be retrieved from /sys/fs/btrfs with the addition of a data ratio calculation (which is useful for calculating required disk space).

Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, looks good! Just had a few in-line questions/comments.

// Package btrfs provides access to statistics exposed by Btrfs filesystems.
package btrfs

// Stats contains statistics for a single Btrfs filesystem.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a link to the docs defining the data structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no proper documentation for this outside of fs/btrfs/sysfs.c, I have added that.

btrfs/get.go Outdated
r := &reader{path: uuidPath}
s := r.readFilesystemStats()

return s, r.err
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would probably be preferable to return a nil Stats here if the error is not nil, to be consistent with the rest of the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack!

btrfs/get.go Outdated

// readLayout reads the Btrfs layout statistics for an allocation layout.
func (r *reader) readLayout(p string) (l *LayoutUsage) {
if !r.exists(p) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering about the reasoning for the existence checks. Is it expected that the directory structure will change often, and that this directory may no longer exist when we try to read it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a leftover from the first quick-and-dirty implementation, where I would loop over the options (single, raid0, etc).

Theoretically it could change while trying to read it (during a balance from one layout to another). But in that case the concern remains the same for the values that are read, so this check doesn't really add anything. I have removed it, and the exists function as it is now unused.

Signed-off-by: Silke Hofstra <silke@slxh.eu>
Signed-off-by: Silke Hofstra <silke@slxh.eu>
Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

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

LGTM

@pgier pgier merged commit 648defe into prometheus:master Oct 15, 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.

2 participants