fix: not allow more than 250 snapshots including all kind of snapshots #1404
Conversation
WalkthroughThe pull request simplifies the snapshot counting mechanism in the Replica component. Specifically, the Changes
Sequence Diagram(s)sequenceDiagram
participant U as Caller
participant R as Replica
U->>R: GetSnapshotCountUsage()
loop For each disk in diskData
R->>R: Check if disk is non-nil
alt Disk exists
R->>R: Increment snapshot count
end
end
R-->>U: Return snapshot count
Assessment against linked issues
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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
|
derekbit
requested review from
FrankYang0529,
innobead,
mantissahz,
PhanLe1010 and
james-munson
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/replica/replica.go (3)
414-415: Update the comment to reflect the new counting logic.The comment "exclude volume head, backing disk, and removed disks" is incorrect as the new implementation only excludes nil disks.
- // exclude volume head, backing disk, and removed disks + // exclude nil disks
1401-1406: Rename method to better reflect its purpose.The method name
GetSnapshotCountUsageis misleading since it counts all non-nil disks, not just snapshots. Consider renaming it to better reflect its actual functionality.-func (r *Replica) GetSnapshotCountUsage() int { +func (r *Replica) GetTotalDiskCount() int {
1409-1421: Optimize disk counting implementation.The current implementation can be more efficient by using a counter variable and avoiding unnecessary assignments.
func (r *Replica) getSnapshotCountUsage() int { - var ( - snapshotCount int - ) + count := 0 for _, disk := range r.diskData { - // exclude volume head, backing disk, and removed disks if disk == nil { continue } - snapshotCount++ + count++ } - return snapshotCount + return count }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/replica/replica.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build ARM64 binaries
- GitHub Check: Build AMD64 binaries
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/replica/replica.go (1)
32-32: LGTM! The maximum chain length constant aligns with the 8-bit map limitation.The constant value of 250 provides a safe buffer below the 8-bit maximum of 256, which is crucial for preventing overflow issues.
|
@mergify backport v1.8.x v1.7.x v1.6.x |
🟠 Waiting for conditions to match
|
|
Need to review if the documentation needs to be updated accordingly. |
Not allow creation of more than 250 snapshots including all kind of snapshots (volume-head, backing image, removed snapshot, ...) because we use 8-bit map for snapshot indexing. Longhorn 10308 Signed-off-by: Derek Su <[email protected]>
|
Removed the outdated comment. |
|
Proposing an alternative solution #1405 because this PR has behavior change which might cause confusion/regression in the following case:
|
@PhanLe1010 |
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10308
What this PR does / why we need it:
Not allow creation of more than 250 snapshots including all kind of snapshots (volume-head, backing image, removed snapshot, ...) because we use 8-bit map for snapshot indexing.
Special notes for your reviewer:
Additional documentation or context