fix(scheduler): integer divided by zero panic #3608
Conversation
WalkthroughThis pull request refines the disk scheduling logic in two components. In the volume controller, a new condition check ensures that only disks marked as schedulable are processed. In the replica scheduler, additional error handling prevents division by zero by verifying the storage maximum and introduces structured logging. A corresponding unit test has also been added to validate the updated scheduler behavior under various disk pressure scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant VC as VolumeController
participant Disk as DiskInfo
VC->>Disk: Check DiskConditionTypeSchedulable
alt Disk is schedulable
VC->>VC: Process disk further
else Disk is not schedulable
VC->>VC: Skip disk (continue loop)
end
sequenceDiagram
participant RS as ReplicaScheduler
participant Info as DiskSchedulingInfo
RS->>RS: Check StorageMaximum value
alt StorageMaximum <= 0
RS->>RS: Log warning and return false
else Valid StorageMaximum
RS->>RS: Calculate newDiskUsagePercentage
RS->>RS: Evaluate disk pressure against threshold
end
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
c3y1huang
force-pushed
the
10502-integer-divide-by-zero-in-replica-scheduler
branch
from
81dd1db to
d8d648e
Compare
c3y1huang
changed the title
c3y1huang
changed the title
longhorn/longhorn-10502 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-10502 Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
10502-integer-divide-by-zero-in-replica-scheduler
branch
2 times, most recently
from
50041e7 to
4e5f21c
Compare
longhorn/longhorn-10502 Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
10502-integer-divide-by-zero-in-replica-scheduler
branch
from
4e5f21c to
1f0b3b5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/volume_controller.go(1 hunks)scheduler/replica_scheduler.go(1 hunks)scheduler/replica_scheduler_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
scheduler/replica_scheduler.go (1)
825-842: Good fix for preventing division by zero panic.The implementation correctly prevents the division by zero error by adding a null check on
info.StorageMaximumbefore performing the calculation. The structured logging is also a valuable addition that will make debugging easier.func (rcs *ReplicaScheduler) IsSchedulableToDiskConsiderDiskPressure(diskPressurePercentage, size, requiredStorage int64, info *DiskSchedulingInfo) bool { + log := logrus.WithFields(logrus.Fields{ + "diskUUID": info.DiskUUID, + "diskPressurePercentage": diskPressurePercentage, + "requiredStorage": requiredStorage, + "storageScheduled": info.StorageScheduled, + "storageReserved": info.StorageReserved, + "storageMaximum": info.StorageMaximum, + }) + + if info.StorageMaximum <= 0 { + log.Warnf("StorageMaximum is %v, skip evaluating new disk usage", info.StorageMaximum) + return false + } + newDiskUsagePercentage := (requiredStorage + info.StorageScheduled + info.StorageReserved) * 100 / info.StorageMaximum + log.Debugf("Evaluated new disk usage percentage after scheduling replica: %v%%", newDiskUsagePercentage)scheduler/replica_scheduler_test.go (1)
1619-1686: Good test coverage for the fixed issue.The test implementation is comprehensive and includes multiple scenarios to validate the function's behavior:
- A scenario where scheduling is allowed (disk usage is under pressure threshold)
- A scenario where scheduling is denied (disk usage would exceed pressure threshold)
- A specific scenario to test the division by zero fix
This ensures the function works correctly in normal cases while also validating that the new check prevents the panic.
|
@mergify backport v1.8.x 1.7.x |
❌ No backport have been created
GitHub error: |
|
@mergify backport v1.7.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10502
What this PR does / why we need it:
Fix
Panic: runtime error: integer divide by zeroduringIsSchedulableToDiskConsiderDiskPressure().Special notes for your reviewer:
NoneAdditional documentation or context
None