feat: v2 fast rebuilding #3401
feat: v2 fast rebuilding #3401
Conversation
WalkthroughThis pull request introduces modifications across multiple files to enhance replica management in the Longhorn system. The primary changes involve updating the Changes
Sequence DiagramsequenceDiagram
participant Controller as Engine/Volume Controller
participant Scheduler as Replica Scheduler
participant Setting as Configuration
participant Engine as Engine API
Setting->>Scheduler: Check V2DataEngineFastReplicaRebuilding
alt Setting Enabled
Scheduler->>Controller: Allow Replica Reuse/Rebuild
Controller->>Engine: ReplicaRemove(engine, url, replicaName)
else Setting Disabled
Scheduler->>Scheduler: Skip Replica Reuse
Controller->>Controller: Standard Replica Handling
end
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
There was a problem hiding this comment.
LGTM.
Only one question and need to update the dependencies.
Found error
vet: engineapi/enginesim_test.go:59:41: not enough arguments in call to sim.ReplicaRemove
have (*"github.com/longhorn/longhorn-manager/k8s/pkg/apis/longhorn/v1beta2".Engine, string)
want (*"github.com/longhorn/longhorn-manager/k8s/pkg/apis/longhorn/v1beta2".Engine, string, string)
time="2024-12-27T16:44:46Z" level=fatal msg="exit status 1"
| V2DataEngineFastReplicaRebuilding, err := c.ds.GetSettingAsBool(types.SettingNameV2DataEngineFastReplicaRebuilding) | ||
| if err != nil { | ||
| log.WithError(err).Warnf("Failed to get the setting %v, will consider it as false", types.SettingDefinitionV2DataEngineFastReplicaRebuilding) | ||
| V2DataEngineFastReplicaRebuilding = false | ||
| } |
| @@ -1443,6 +1446,16 @@ var ( | |||
| Default: "", | |||
| } | |||
|
|
|||
| SettingDefinitionV2DataEngineFastReplicaRebuilding = SettingDefinition{ | |||
| DisplayName: "V2 Data Engine Fast Replica Rebuilding", | |||
There was a problem hiding this comment.
I assume this setting is temporary before v2 becomes GA, as delta replica rebuilding should be eventually enabled always. Right?
There was a problem hiding this comment.
YES! Next, I will check when we can launch a setting similar to v1 Snapshot Data Integrity instead. If we can do that before v1.8 official release, V2 Data Engine Fast Replica Rebuilding won't be delivered to users at all.
Longhorn 9488 Signed-off-by: Shuo Wu <[email protected]>
Longhorn 9488 Signed-off-by: Shuo Wu <[email protected]>
Longhorn 9488 Signed-off-by: Shuo Wu <[email protected]>
![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)
engineapi/proxy_replica.go (1)
15-16: Validate or logreplicaNamefor clarity.
Consider logging or validating thereplicaNameparameter for better traceability or debugging, given its potential diagnostic value.engineapi/engine.go (1)
180-180: Unused parameterreplicaName.
The newly added parameter isn’t used inside this method. If it’s intended to remain optional, consider documenting it. Otherwise, remove it to reduce confusion.types/setting.go (1)
1449-1458: Check prerequisites for fast replica rebuilding.
The description mentions that the snapshot data integrity must be set to enabled or fast-check. Consider validating that requirement at runtime to prevent misconfigurations where fast rebuilding is enabled without snapshot checksums.+ // Example snippet to validate prerequisites + if newSetting.Name == SettingNameV2DataEngineFastReplicaRebuilding && newSetting.Value == "true" { + sdiSetting, err := ds.GetSetting(SettingNameSnapshotDataIntegrity) + if err != nil { + return fmt.Errorf("failed to retrieve snapshot data integrity setting") + } + if sdiSetting.Value == string(longhorn.SnapshotDataIntegrityDisabled) { + return fmt.Errorf("cannot enable v2-engine-fast-replica-rebuilding without enabling snapshot-data-integrity") + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (38)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/fsnotify/fsnotify/.cirrus.ymlis excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/.editorconfigis excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/.gitattributesis excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/.gitignoreis excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/CHANGELOG.mdis excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/CONTRIBUTING.mdis excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/backend_fen.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/backend_inotify.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/backend_kqueue.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/backend_other.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/backend_windows.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/fsnotify.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/darwin.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_darwin.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_dragonfly.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_freebsd.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_kqueue.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_linux.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_netbsd.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_openbsd.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_solaris.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_windows.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/freebsd.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/internal.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/unix.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/unix2.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/internal/windows.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/mkdoc.zshis excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/system_bsd.gois excluded by!vendor/**vendor/github.com/fsnotify/fsnotify/system_darwin.gois excluded by!vendor/**vendor/github.com/longhorn/types/pkg/generated/spdkrpc/spdk.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/google.golang.org/protobuf/internal/errors/is_go112.gois excluded by!vendor/**vendor/google.golang.org/protobuf/internal/errors/is_go113.gois excluded by!vendor/**vendor/google.golang.org/protobuf/internal/impl/message_reflect.gois excluded by!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (9)
controller/engine_controller.go(2 hunks)controller/volume_controller.go(1 hunks)engineapi/engine.go(1 hunks)engineapi/enginesim.go(1 hunks)engineapi/enginesim_test.go(1 hunks)engineapi/proxy_replica.go(1 hunks)engineapi/types.go(1 hunks)scheduler/replica_scheduler.go(2 hunks)types/setting.go(4 hunks)
🔇 Additional comments (11)
controller/volume_controller.go (1)
4991-5001: Consider differentiating ephemeral errors from missing settings.
Currently, the code sets V2DataEngineFastReplicaRebuilding to false upon any retrieval error, potentially triggering undesired replica cleanup if the error is transient. Consider handling “not found” vs. other error scenarios separately, or adding fallback logic to avoid indiscriminate cleanup.
engineapi/enginesim_test.go (1)
59-59: LGTM for new parameter addition.
The extra argument aligns well with the updated ReplicaRemove signature, and the test remains clear and coherent.
engineapi/enginesim.go (1)
148-148: Consider utilizing new parameter if necessary.
Although the signature now includes replicaName, it remains unused in this method. If it can help diagnose or coordinate replica operations, consider logging it.
engineapi/types.go (1)
92-92: Ensure all callers are updated to the new method signature.
Adding the replicaName parameter helps distinguish replicas when removing them. Verify that all references to ReplicaRemove have been updated accordingly to avoid runtime errors.
scheduler/replica_scheduler.go (2)
570-578: Handle setting retrieval errors gracefully.
The code falls back to false if retrieving the V2DataEngineFastReplicaRebuilding setting fails. Consider logging more context or re-checking to ensure we do not silently ignore crucial issues.
670-678: Consistent logic for scheduling new replicas.
Similar to the earlier block, ensure the fallback logic for V2DataEngineFastReplicaRebuilding remains consistent so that scheduling does not unintentionally bypass fast rebuilding.
controller/engine_controller.go (2)
1658-1658: Passing an empty replica name.
When calling ReplicaRemove(e, url, ""), verify that this usage is supported. If the replica’s name is crucial for logs or further validation, consider replacing the empty string with a meaningful identifier.
1861-1861: Remove the rebuilding replica using its actual name.
Now that ReplicaRemove takes replicaName, it ensures clarity. Confirm that both the replica URL and the name match the intended target to avoid removing the wrong replica.
types/setting.go (3)
135-135: Properly name the new setting constant.
The naming convention follows existing patterns for setting definitions.
229-229: Inclusion in SettingNameList is correct.
This ensures the new setting is recognized and managed platform-wide.
349-349: Registering the setting in settingDefinitions.
Ensures that the setting is visible for validation and defaulting mechanisms.
|
@mergify backport v1.8.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
longhorn/longhorn#9488
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context