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.

Sign up for GitHub

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

Fire SmartHealthStatusFail only for physical devices #378

Closed
wants to merge 3 commits into from
Closed

Fire SmartHealthStatusFail only for physical devices #378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
10f2583
Consider physical blocksize for SmartHealthStatusFail alert
Deezzir Dec 14, 2024
593d164
Merge branch 'main' into SOLENG-946
Deezzir Dec 14, 2024
f292486
Add comments
Deezzir Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/prometheus_alert_rules/smart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ groups:
LABELS = {{ $labels }}

- alert: SmartHealthStatusFail
expr: smartctl_device_smart_status == 0
# We can check if the physical size of the drive is 0, meaning it's a logical device,
# and ignore the status 0, which is always returned for logical devices, like HW RAID,
# avoiding false positives.
expr: (smartctl_device_smart_status == 0) and on(device, juju_unit) (smartctl_device_block_size{blocks_type="physical"} != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking):

It will be more nice if you can provide some information as comment here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree

Copy link

Choose a reason for hiding this comment

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

Unfortunately, I think there is an incorrect assumption here re: "physical" versus "logical" block size.

I tested this on my local machine. I have an NVME, and it does show up in SMART. It doesn't return a physical block size, but only a logical block size, via smartctl.

There is also the concept of logical block sizes on disks.

That is, this likely does not have to do with whether a device is a "real" physical device or a simulated device like a VM disk, but rather with those technical disk parameters.

I installed prometheus on my laptop and did a quick check against its NVME; this is what I see:

paul-goins@pdg-thinkpad:~/code/hardware-observer-operator$ curl --silent http://localhost:9633/metrics | grep block_size
# HELP smartctl_device_block_size Device block size
# TYPE smartctl_device_block_size gauge
smartctl_device_block_size{blocks_type="logical",device="nvme0"} 512
smartctl_device_block_size{blocks_type="physical",device="nvme0"} 0

As written, this check will miss any disks - physical disks - which do not report a physical block size but only a logical one.

Also if you're curious, I poked around the sources of the prometheus plugin and extracted what appears to be the smartctl command it runs to pull the block sizes. This is what I get:

paul-goins@pdg-thinkpad:~/code/hardware-observer-operator$ sudo smartctl --json --info --health --attributes --tolerance=verypermissive --nocheck=standby --format=brief --log=error /dev/nvme0 | grep block_size
  "logical_block_size": 512,

You could try this on your own machine if curious.

TLDR: Unfortunately -1; I think we need a different methodology here.

Copy link
Member

Choose a reason for hiding this comment

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

is it necessary to filter by blocks_type="physical" ? Can't we assume if it has size 0, we should not check 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.

I think that way it's more explicit

Copy link
Member

@gabrielcocenza gabrielcocenza Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know if you understood my question.

I don't know exactly how many block_types that exists on smartctl, let's say that we have:

  • logical
  • physical
  • foo
  • bar
  • ...

With this query we are just joining when physical. Does it make sense to trigger the alert, for example, if the block type is logical with size 0 and status 0 ?

for: 2m
labels:
severity: critical
Expand Down
14 changes: 14 additions & 0 deletions tests/unit/test_alert_rules/test_smart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ tests:
input_series:
- series: 'smartctl_device_smart_status{device="sda", instance="ubuntu-1"}'
values: '0x15'
- series: 'smartctl_device_block_size{device="sda", instance="ubuntu-1", blocks_type="physical"}'
values: '1x15'

alert_rule_test:
- eval_time: 10m
Expand All @@ -84,6 +86,18 @@ tests:
VALUE = 0
LABELS = map[__name__:smartctl_device_smart_status device:sda instance:ubuntu-1]

- interval: 1m
input_series:
- series: 'smartctl_device_smart_status{device="sda", instance="ubuntu-1"}'
values: '0x15'
- series: 'smartctl_device_block_size{device="sda", instance="ubuntu-1", blocks_type="physical"}'
values: '0x15'

alert_rule_test:
- eval_time: 10m
alertname: SmartHealthStatusFail
exp_alerts: # alerts shouldn't fire since block size is 0

- interval: 1m
input_series:
- series: 'smartctl_device_smartctl_exit_status{device="sda", instance="ubuntu-2"}'
Expand Down
Loading