fix: do not allow total all kind of snapshots to be bigger than 250 #1405
Conversation
WalkthroughThis pull request revises how snapshot counts are managed across components. Hardcoded values for the "snapshot-max-count" flag have been replaced with a dynamic constant (types.MaximumTotalSnapshotCount), and an additional integer (countTotal) is now returned in methods that report snapshot metrics. In several places, the method signatures and error returns have been updated accordingly. Furthermore, new fields have been added to the replica information structures and the RPC response to include the total snapshot count, with a new constant defined to standardize the snapshot limit. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Controller
participant B as Backend
participant T as Types
C->>B: Call GetSnapshotCountAndSizeUsage()
B-->>C: Return (countUsage, countTotal, sizeUsage, error)
C->>C: Check if countTotal > T.MaximumTotalSnapshotCount
alt Snapshot count exceeds limit
C-->>C: Return error ("snapshot count too high")
else Within allowed limit
C-->>C: Proceed with snapshot operation
end
sequenceDiagram
participant RC as ReplicaClient
participant RS as ReplicaServer
participant R as Replica
RC->>RS: Request replica information
RS->>R: Call GetSnapshotCount()
R-->>RS: Return (snapCountUsage, snapCountTotal)
RS->>RS: Convert values & populate ReplicaInfo (SnapshotCountUsage, SnapshotCountTotal)
RS-->>RC: Return complete ReplicaInfo
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (8)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/backend/file/file.go (1)
102-103: Add documentation for dummy implementation.The method returns dummy values (1, 1, 0, nil) as file backend doesn't support real snapshots. Consider adding a comment to explain this, similar to the comment on line 88 for GetHeadFileSize.
+// GetSnapshotCountAndSizeUsage uses dummy values for file backend func (f *Wrapper) GetSnapshotCountAndSizeUsage() (int, int, int64, error) { return 1, 1, 0, nilapp/cmd/controller.go (1)
87-89: LGTM! Good replacement of magic number with constant.The change improves maintainability by using
types.MaximumTotalSnapshotCountinstead of a hardcoded value.Consider adding a comment explaining why this specific limit was chosen and what happens when it's reached.
pkg/replica/rpc/server.go (1)
87-89: LGTM! Good implementation of the new snapshot counting logic.The code correctly retrieves and sets both the snapshot usage and total count.
Consider adding error handling for the type conversion to int32 to prevent potential overflow for large snapshot counts:
- snapCountUsage, snapCountTotal := r.GetSnapshotCount() - replica.SnapshotCountUsage = int32(snapCountUsage) - replica.SnapshotCountTotal = int32(snapCountTotal) + snapCountUsage, snapCountTotal := r.GetSnapshotCount() + if snapCountUsage > math.MaxInt32 || snapCountTotal > math.MaxInt32 { + logrus.Warnf("Snapshot count exceeds int32 max value: usage=%d, total=%d", snapCountUsage, snapCountTotal) + } + replica.SnapshotCountUsage = int32(snapCountUsage) + replica.SnapshotCountTotal = int32(snapCountTotal)pkg/replica/replica.go (1)
1399-1408: Consider adding error handling for edge cases.The new
GetSnapshotCountmethod correctly returns both usage and total counts, but consider adding error handling for potential edge cases such as nil maps or corrupted disk data.func (r *Replica) GetSnapshotCount() (int, int) { r.RLock() defer r.RUnlock() - return r.getSnapshotCountUsage(), r.getSnapshotCountTotal() + if r.diskData == nil { + return 0, 0 + } + return r.getSnapshotCountUsage(), r.getSnapshotCountTotal() }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/types/pkg/generated/enginerpc/replica.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (11)
app/cmd/controller.go(1 hunks)app/cmd/replica.go(1 hunks)pkg/backend/file/file.go(1 hunks)pkg/backend/remote/remote.go(1 hunks)pkg/controller/control.go(3 hunks)pkg/controller/replicator.go(1 hunks)pkg/replica/client/client.go(1 hunks)pkg/replica/replica.go(3 hunks)pkg/replica/rpc/server.go(1 hunks)pkg/types/resource.go(1 hunks)pkg/types/types.go(2 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 (12)
pkg/types/types.go (2)
50-50: LGTM! Constant definition aligns with PR objective.The constant
MaximumTotalSnapshotCountis appropriately defined with the value 250, matching the PR's goal of limiting total snapshots.
110-110: LGTM! Method signature updated to support total snapshot count.The
GetSnapshotCountAndSizeUsagemethod now returns(int, int, int64, error)to accommodate the total snapshot count.pkg/types/resource.go (1)
21-21: LGTM! Field addition follows existing patterns.The
SnapshotCountTotalfield is appropriately placed with other snapshot-related fields and follows the established JSON tag naming convention.app/cmd/replica.go (1)
79-79: LGTM! Flag value now uses the centralized constant.The
snapshot-max-countflag appropriately usestypes.MaximumTotalSnapshotCount, ensuring consistency across the codebase.pkg/controller/replicator.go (1)
356-388: LGTM! Good error handling and max value calculation.The method correctly:
- Returns appropriate zero values on error
- Tracks whether a valid result was found
- Calculates maximum values across backends
pkg/backend/remote/remote.go (1)
225-235: LGTM! Clean implementation of the updated method.The changes correctly:
- Update the method signature
- Handle error cases
- Check replica state before returning values
- Return values in the expected order
pkg/replica/client/client.go (1)
171-171: LGTM!The addition of
SnapshotCountTotalfield toReplicaInfostruct is clean and properly handles the type conversion from the gRPC response.pkg/replica/replica.go (2)
152-152: LGTM!Replacing hardcoded value with
types.MaximumTotalSnapshotCountinNewReadOnlyimproves maintainability by centralizing the constant definition.
1051-1053: LGTM!Using
types.MaximumTotalSnapshotCountinopenLiveChainensures consistent snapshot limit enforcement across the codebase.pkg/controller/control.go (3)
289-298: LGTM!The implementation correctly validates both snapshot usage and total count limits. The error messages are clear and informative.
764-765: LGTM!The method correctly adapts to the new return value structure from
GetSnapshotCountAndSizeUsage.
802-803: LGTM!The method correctly adapts to the new return value structure from
GetSnapshotCountAndSizeUsage.
pkg/controller/replicator.go
Outdated
| if countTotal < currentCountTotal { | ||
| countUsage = currentCountTotal | ||
| } |
There was a problem hiding this comment.
Fix critical bug in countTotal assignment.
There's a bug where countUsage is incorrectly used instead of countTotal when updating the maximum total count.
Apply this fix:
- countUsage = currentCountTotal
+ countTotal = currentCountTotal📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if countTotal < currentCountTotal { | |
| countUsage = currentCountTotal | |
| } | |
| if countTotal < currentCountTotal { | |
| countTotal = currentCountTotal | |
| } |
There was a problem hiding this comment.
Cool. Thank you! Fixed
There was a problem hiding this comment.
🧩 Analysis chain
@PhanLe1010 Great! The fix has been applied correctly. 🎉
🏁 Script executed:
#!/bin/bash
# Check if the fix has been applied correctly
rg -A 1 -B 1 "if countTotal < currentCountTotal" pkg/controller/replicator.go
Length of output: 155
@PhanLe1010 The fix looks perfect! The code is now correctly updating the countTotal variable. 👍
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
No. No shout-out for you :)))
There was a problem hiding this comment.
@PhanLe1010 No worries! I'll keep helping you catch those bugs either way! 😄
There was a problem hiding this comment.
Lol. Just kidding. I don't use X so sorry
There was a problem hiding this comment.
@PhanLe1010 No problem at all! Thanks for the friendly banter and for fixing the bug! 😊
PhanLe1010
requested review from
derekbit,
innobead,
james-munson,
shuo-wu,
FrankYang0529 and
ChanYiLin
longhorn-10308 Signed-off-by: Phan Le <[email protected]>
|
@mergify backport v1.8.x v1.7.x v1.6.x |
✅ Backports have been created
|
| @@ -4,6 +4,8 @@ go 1.23.0 | |||
|
|
|||
| toolchain go1.23.5 | |||
|
|
|||
| replace github.com/longhorn/types => ../types | |||
There was a problem hiding this comment.
Missed this. Do we need the replacement? @PhanLe1010
There was a problem hiding this comment.
As mentioned in the PR description, we need to merge this dependent PR first longhorn/types#45 then remove the replace directive
longhorn/longhorn#10308
Special notes for your reviewer:
The dependent PR is at longhorn/types#45
Additional documentation or context
This PR is an alternative proposal to the PR #1404. Why?