Skip to content

pool server - don't issue MANY_STORAGE_ISSUES after scaling up (dfbgus 4152) #9235

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

Merged
merged 1 commit into from
Oct 24, 2025
Merged

pool server - don't issue MANY_STORAGE_ISSUES after scaling up (dfbgus 4152) #9235

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/server/system_services/pool_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1190,8 +1190,11 @@ function calc_hosts_pool_mode(pool_info, storage_by_mode, s3_by_mode) {
const storage_count = hosts.by_service.STORAGE;
const storage_offline = storage_by_mode.OFFLINE || 0;
const storage_optimal = storage_by_mode.OPTIMAL || 0;
const storage_low_capacity = storage_by_mode.LOW_CAPACITY || 0;
Copy link
Member

Choose a reason for hiding this comment

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

What about NO_CAPACITY mode ?
Worth going over all other modes (enum here) and see if they can also be excluded from MANY_STORAGE_ISSUES.
Maybe anything that can be a result of a user operation (e.g.DELETING) or a temp thing (INITIALIZING) can also be ignored. @alphaprinz @nimrod-becker WDYT?

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 tend to go with a no.
-Temp states will go away and no one will care about them.
-User actions affecting the state makes sense.
-NO_CAPACITY is tricky to ignore because if some other issues incapacitates other hosts, you're left with nothing (as opposed to LOW_CAPACITY which would still allow you some operational uptime).

const storage_offline_ratio = (storage_offline / host_count) * 100;
const storage_issues_ratio = ((storage_count - storage_optimal) / storage_count) * 100;
//don't count individual storage with low capacity as having issues.
//low capacity is handled for the entire BS by free_ratio check below
const storage_issues_ratio = ((storage_count - storage_optimal - storage_low_capacity) / storage_count) * 100;
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Verify handling of zero storage_count.

If storage_count is zero, the calculation (storage_count - storage_optimal - storage_low_capacity) / storage_count results in NaN. This would cause the storage issue ratio checks (lines 1218, 1220-1221) to fail silently since NaN >= 90 evaluates to false.

Confirm whether storage_count can be zero when host_count > 0 (e.g., when only S3 gateway nodes are present). If so, consider adding a guard condition or defaulting storage_issues_ratio to 0 when storage_count === 0.

Note: Line 1202 has similar logic for s3_issues_ratio, suggesting this may be a pre-existing pattern.

Run the following script to check if there are any guards or validations for zero counts elsewhere in the codebase:

🏁 Script executed:

#!/bin/bash
# Description: Search for similar ratio calculations and guards for zero denominators

# Search for ratio calculations with potential division by zero
rg -n -C3 'ratio.*=.*\/.*count' src/server/system_services/pool_server.js

# Search for guards checking if count is zero before division
rg -n -C3 'count.*===.*0|count.*>.*0.*ratio' src/server/system_services/pool_server.js

Length of output: 1635

🏁 Script executed:

#!/bin/bash
# Search for storage_issues_ratio definition and nearby guards
rg -n -C3 'storage_issues_ratio' src/server/system_services/pool_server.js

# Search for any if-statements checking storage_count before use
rg -n -C3 'if.*storage_count' src/server/system_services/pool_server.js

# Search for s3_issues_ratio definition and nearby guards
rg -n -C3 's3_issues_ratio' src/server/system_services/pool_server.js

# Search for any if-statements checking s3_issues_ratio or its denominator
rg -n -C3 'if.*s3_issues_ratio' src/server/system_services/pool_server.js

Length of output: 2783

Guard against zero denominators when computing storage_issues_ratio and s3_issues_ratio. Default each ratio to 0 if its count is 0 (e.g. replace lines 1197 and 1202 with count > 0 ? (…) : 0) to avoid NaN.

🤖 Prompt for AI Agents 
In src/server/system_services/pool_server.js around lines 1197 and 1202, the
calculations for storage_issues_ratio and s3_issues_ratio can divide by zero
producing NaN; update each assignment to check the denominator count
(storage_count and s3_count) and return 0 when the count is 0, otherwise compute
the percentage as before (i.e., use a ternary like count > 0 ? (numerator/
count) * 100 : 0) so the ratios default to 0 when their counts are zero.

const hosts_initializing = hosts.by_mode.INITIALIZING || 0;
const hosts_migrating = (hosts.by_mode.INITIALIZING || 0) + (hosts.by_mode.MIGRATING || 0);
const s3_count = hosts.by_service.GATEWAY;
Expand Down