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

[check-disk] skip checks if there are no inodes #265

Merged
merged 4 commits into from
Dec 17, 2018

Conversation

susisu
Copy link
Contributor

@susisu susisu commented Dec 14, 2018

On Windows, there are no "inodes" and checks on them should be skipped.
However, the current version always raises wranings / errors if -w N% / -c N% is specified.
This PR fixes it by skipping checks if total inodes is zero (gopsutil/disk returns InodesTotal = 0 on Windows https://github.com/shirou/gopsutil/blob/master/disk/disk_windows.go).

The change may affect other (non-Windows) environments, but I think it is OK because talking about the "percentage" of 0/0 is nonsense...

@susisu susisu self-assigned this Dec 14, 2018
current = status
}

if !chkInode && (v > freePct || v > inodesFreePct) {
if !chkInode && (v > freePct || (disk.InodesTotal != 0 && v > inodesFreePct)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think comparing inodesFreePct here is correct behavior, since the help says Exit with WARNING status if less than N units or N% of disk are free, not N% of inodes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Maybe we can omit this inodesFreePct checking from this if.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we should split checkStatus into two (one for usage, one for inode)

Copy link
Contributor Author

@susisu susisu Dec 17, 2018

Choose a reason for hiding this comment

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

I will propose changing this in another PR. Thanks!

current = status
}

if !chkInode && (v > freePct || v > inodesFreePct) {
if !chkInode && (v > freePct || (disk.InodesTotal != 0 && v > inodesFreePct)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Maybe we can omit this inodesFreePct checking from this if.

current = status
}

if !chkInode && (v > freePct || v > inodesFreePct) {
if !chkInode && (v > freePct || (disk.InodesTotal != 0 && v > inodesFreePct)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Or we should split checkStatus into two (one for usage, one for inode)

@astj
Copy link
Contributor

astj commented Dec 17, 2018

This change itself looks good.


if chkInode && v > inodesFreePct {
if chkInode && (disk.InodesTotal != 0 && v > inodesFreePct) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not related to this issues, but I confused what is v...
How about using percentThreshold or pctThreshold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed in bea07d5 👍


if chkInode && v > inodesFreePct {
if chkInode && (disk.InodesTotal != 0 && v > inodesFreePct) {
current = status
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not understand what disk.InodesTotal! = 0 means for just a moment. So How about writing comment, for example If InodesTotal == 0, the system don't use inode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a note 94e20dc 👍

@susisu susisu merged commit ebb0a54 into master Dec 17, 2018
@susisu susisu deleted the check-disk-skip-if-no-inodes branch December 17, 2018 09:05
@astj astj mentioned this pull request Jan 10, 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

3 participants