fix(upgrade): fix upgrade issues from v1.7.x to v1.8.0 #3383
Conversation
Longhorn 10032 Signed-off-by: Derek Su <derek.su@suse.com>
WalkthroughThe changes in the Changes
Assessment against linked issues
The changes appear to be related to the upgrade process, but the direct link to resolving the specific test failures in issue #10032 is not immediately clear from the provided code modifications. Further investigation and testing would be needed to confirm if these changes address the backup-related test case failures. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
|
@mergify backport v1.8.x |
✅ Backports have been created
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
upgrade/v17xto180/upgrade.go (1)
Line range hint
218-229: Consider extracting the DataEngine setting logicSince both blocks perform similar operations on different maps, consider extracting this logic into a helper function to improve maintainability and reduce code duplication.
Here's a suggested implementation:
+func setDataEngineTypeForDiskFiles[T interface{ GetDataEngine() *longhorn.DataEngineType; SetDataEngine(t longhorn.DataEngineType) }](diskMap map[string]T) { + if diskMap != nil { + for diskUUID := range diskMap { + diskMap[diskUUID].SetDataEngine(longhorn.DataEngineTypeV1) + } + } +} func upgradeBackingImages(namespace string, lhClient *lhclientset.Clientset, resourceMaps map[string]interface{}) (err error) { // ... existing code ... // before v1.8.0, there should not have any v2 data engine disk in the backing image. - if bi.Spec.DiskFileSpecMap != nil { - for diskUUID := range bi.Spec.DiskFileSpecMap { - bi.Spec.DiskFileSpecMap[diskUUID].DataEngine = longhorn.DataEngineTypeV1 - } - } + setDataEngineTypeForDiskFiles(bi.Spec.DiskFileSpecMap) // before v1.8.0, there should not have any v2 data engine disk in the backing image. - if bi.Status.DiskFileStatusMap != nil { - for diskUUID := range bi.Status.DiskFileStatusMap { - bi.Status.DiskFileStatusMap[diskUUID].DataEngine = longhorn.DataEngineTypeV1 - } - } + setDataEngineTypeForDiskFiles(bi.Status.DiskFileStatusMap) // ... rest of the code ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
upgrade/v17xto180/upgrade.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
upgrade/v17xto180/upgrade.go (2)
Learnt from: derekbit
PR: longhorn/longhorn-manager#3295
File: controller/backing_image_data_source_controller.go:404-407
Timestamp: 2024-12-13T07:06:57.033Z
Learning: In the `syncBackingImage` function in `controller/backing_image_data_source_controller.go`, when adding a new entry to `bi.Spec.DiskFileSpecMap`, the `DataEngine` is set to `DataEngineTypeV1` because the backing image data source always prepares the backing image on the v1 disk.
Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/v17xto180/upgrade.go:52-58
Timestamp: 2024-11-10T16:45:04.898Z
Learning: In the `upgrade/v17xto180/upgrade.go` file, prefer to keep individual error handling in each resource upgrade function instead of consolidating them into a shared helper function.
🔇 Additional comments (2)
upgrade/v17xto180/upgrade.go (2)
218-222: LGTM: Setting DataEngine type for DiskFileSpecMap
The implementation correctly sets the DataEngine type to v1 for all disk files in the spec map, which aligns with the requirement that backing image data sources always prepare backing images on v1 disk.
Line range hint 225-229: LGTM: Setting DataEngine type for DiskFileStatusMap
The implementation maintains consistency by setting the DataEngine type to v1 in the status map, matching the spec map changes above.
Longhorn 10032 Signed-off-by: Derek Su <derek.su@suse.com>
|
@mantissahz @mantissahz @ChanYiLin cc @longhorn/qa @innobead |
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10032
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context