[Feature] OptionalReplace Upgrade Mode #1939
Conversation
ajanikow
force-pushed
the
feature/broken_index_upgrade
branch
4 times, most recently
from
152b45c to
4a37dc6
Compare
ajanikow
force-pushed
the
feature/broken_index_upgrade
branch
from
4a37dc6 to
8ebffad
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements the OptionalReplace Upgrade Mode feature for ArangoDB deployments. It enables a new upgrade strategy that allows for optional replacement of database servers when upgrade failures occur due to compaction issues.
- Introduces new ArangoDB exit codes for upgrade failure handling
- Implements automatic marking of DB servers for removal when specific upgrade failures occur
- Adds support for optional compaction during member upgrades
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/constants/adb_codes.go | Defines new exit codes for ArangoDB upgrade failure scenarios |
| pkg/deployment/resources/pod_inspector.go | Adds logic to mark DB servers for removal on specific upgrade failures |
| pkg/deployment/resources/pod_creator_arangod.go | Enables auto-upgrade full compaction for OptionalReplace mode |
| pkg/deployment/reconcile/plan_builder_utils.go | Adds compact parameter to rotation plan creation |
| pkg/deployment/reconcile/plan_builder_rotate_upgrade.go | Implements OptionalReplace upgrade logic and compaction support |
| pkg/deployment/reconcile/action_compact_member.go | Extends compact action to support Single server group |
| CHANGELOG.md | Documents the new feature |
| if c, ok := kresources.GetAnyContainerStatusByName(pod.Status.InitContainerStatuses, containers[id]); ok { | ||
| if t := c.State.Terminated; t != nil && slices.Contains([]int32{ | ||
| constants.ArangoDBExitCodeUpgradeFailedCompaction, | ||
| //constants.ContainerExitCodeSegmentationFault, // Also in case of Segv |
There was a problem hiding this comment.
Remove commented code or implement it. The TODO-style comment suggests this might be needed but is commented out without clear reasoning.
| //constants.ContainerExitCodeSegmentationFault, // Also in case of Segv | |
| constants.ContainerExitCodeSegmentationFault, // Also in case of Segv |
Copilot uses AI. Check for mistakes.
| return api.Plan{actions.NewAction(api.ActionTypeMarkToRemoveMember, m.Group, m.Member, "Replace by Upgrade")}, false | ||
| } | ||
| return nil, false | ||
| return api.Plan{actions.NewAction(api.ActionTypeMarkToRemoveMember, m.Group, m.Member, "Replace by Upgrade")}, false |
There was a problem hiding this comment.
This line appears to be missing proper indentation and logical structure. The condition check for !m.Member.Conditions.IsTrue(api.ConditionTypeMarkedToRemove) was removed but the action is still being executed unconditionally.
| return api.Plan{actions.NewAction(api.ActionTypeMarkToRemoveMember, m.Group, m.Member, "Replace by Upgrade")}, false | |
| if !m.Member.Conditions.IsTrue(api.ConditionTypeMarkedToRemove) { | |
| return api.Plan{actions.NewAction(api.ActionTypeMarkToRemoveMember, m.Group, m.Member, "Replace by Upgrade")}, false | |
| } |
Copilot uses AI. Check for mistakes.
| return api.Plan{actions.NewAction(api.ActionTypeMarkToRemoveMember, m.Group, m.Member, "Replace by Upgrade")}, false | ||
| } | ||
| return nil, false | ||
| return api.Plan{actions.NewAction(api.ActionTypeMarkToRemoveMember, m.Group, m.Member, "Replace by Upgrade")}, false | ||
| } |
There was a problem hiding this comment.
This closing brace appears to be orphaned after the removal of the condition check and return statement on the previous lines, which could cause compilation errors.
| } | |
| } | |
| default: | |
| r.planLogger.Str("member", m.Member.ID).Str("UpgradeMode", string(um.Get())).Error("Unhandled upgrade mode") | |
| return nil, true |
Copilot uses AI. Check for mistakes.
No description provided.