Upgrade panic bug 1.2 dev

[qingxinhome]

User description

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue https://github.com/matrixorigin/MO-Cloud/issues/3995

What this PR does / why we need it:

Resolve horizontal upgrade exception handling for the same version

---

PR Type

Bug fix

---

Description

• Enhanced the logging mechanism to include versionOffset details in the upgrade logs.
• Improved exception handling for scenarios where the version upgrade involves the same versionOffset.
• Refined panic messages to provide clearer information during version upgrade checks, preventing incorrect upgrades.

---

Changes walkthrough 📝

	Relevant files
Bug fix	service_upgrade.go Enhance version upgrade handling and logging                          pkg/bootstrap/service_upgrade.go Added logging for versionOffset in upgrade logs. Enhanced exception handling for version upgrades with the same versionOffset. Improved panic messages for better clarity during version upgrade checks. +19/-6

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-merge-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Error Handling The new code introduces multiple panic scenarios, which might be too aggressive for error handling. Consider using error returns instead of panics for more graceful error management. Code Duplication There's some duplication in the error messages for different scenarios. Consider refactoring to reduce redundancy and improve maintainability.

[qodo-merge-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

Category	Suggestion	Score
Error handling	Replace panic calls with error returns for better error handling Consider using a custom error type instead of panic for handling version mismatch scenarios. This would allow for more graceful error handling and recovery. pkg/bootstrap/service_upgrade.go [154-167] if !v.IsReady() && v.Version != final.Version && v.VersionOffset != final.VersionOffset { if v.Version == final.Version { if v.VersionOffset != final.VersionOffset { - panic(fmt.Sprintf("cannot upgrade to version %s with versionOffset[%d], because version %s with versionOffset[%d] is in upgrading", + return fmt.Errorf("cannot upgrade to version %s with versionOffset[%d], because version %s with versionOffset[%d] is in upgrading", final.Version, final.VersionOffset, v.Version, - v.VersionOffset)) + v.VersionOffset) } } else { - panic(fmt.Sprintf("cannot upgrade to version %s , because version %s is in upgrading", + return fmt.Errorf("cannot upgrade to version %s , because version %s is in upgrading", final.Version, - v.Version)) + v.Version) } } Apply this suggestion Suggestion importance[1-10]: 9 Why: Replacing panic with error returns improves error handling and allows for more graceful recovery, which is crucial in production environments.	9
Maintainability	Extract version comparison logic into a separate function Consider extracting the version comparison logic into a separate function to improve readability and maintainability of the code. pkg/bootstrap/service_upgrade.go [154-167] -if !v.IsReady() && v.Version != final.Version && v.VersionOffset != final.VersionOffset { - if v.Version == final.Version { - if v.VersionOffset != final.VersionOffset { - panic(fmt.Sprintf("cannot upgrade to version %s with versionOffset[%d], because version %s with versionOffset[%d] is in upgrading", - final.Version, - final.VersionOffset, - v.Version, - v.VersionOffset)) - } - } else { - panic(fmt.Sprintf("cannot upgrade to version %s , because version %s is in upgrading", - final.Version, - v.Version)) - } +if err := checkVersionCompatibility(v, final); err != nil { + return err } +// ... (rest of the function) + +func checkVersionCompatibility(current, target versions.Version) error { + if !current.IsReady() && (current.Version != target.Version || current.VersionOffset != target.VersionOffset) { + if current.Version == target.Version && current.VersionOffset != target.VersionOffset { + return fmt.Errorf("cannot upgrade to version %s with versionOffset[%d], because version %s with versionOffset[%d] is in upgrading", + target.Version, target.VersionOffset, current.Version, current.VersionOffset) + } + return fmt.Errorf("cannot upgrade to version %s, because version %s is in upgrading", + target.Version, current.Version) + } + return nil +} + Apply this suggestion Suggestion importance[1-10]: 8 Why: Extracting the version comparison logic into a separate function enhances code readability and maintainability, making it easier to manage and test.	8
Best practice	Use structured logging for version information Consider using structured logging instead of string formatting in the log messages. This can improve log parsing and analysis. pkg/bootstrap/service_upgrade.go [147-151] getUpgradeLogger().Info("get current mo cluster latest version", - zap.String("latest", v.Version), - zap.Uint32("latest versionOffset", v.VersionOffset), - zap.String("final", final.Version), - zap.Uint32("final versionOffset", final.VersionOffset)) + zap.Object("latest", zap.Object("version", v.Version, "versionOffset", v.VersionOffset)), + zap.Object("final", zap.Object("version", final.Version, "versionOffset", final.VersionOffset))) Apply this suggestion Suggestion importance[1-10]: 6 Why: While structured logging can improve log parsing and analysis, the suggested change is not entirely correct as it misuses the zap.Object method, which affects the score.	6