Progression can't go backwards #1880
Conversation
ELENAGER
force-pushed
the
DFBUGS-612
branch
3 times, most recently
from
c6ded1b to
9bc6ff5
Compare
| var deployProgressionList = []rmn.ProgressionStatus{ | ||
| rmn.ProgressionCreatingMW, | ||
| rmn.ProgressionUpdatingPlRule, | ||
| rmn.ProgressionEnsuringVolSyncSetup, | ||
| rmn.ProgressionSettingupVolsyncDest, | ||
| rmn.ProgressionCompleted, | ||
| } | ||
|
|
||
| var failoverProgressionList = []rmn.ProgressionStatus{ | ||
| rmn.ProgressionCheckingFailoverPrerequisites, | ||
| rmn.ProgressionWaitForFencing, | ||
| rmn.ProgressionWaitForStorageMaintenanceActivation, | ||
| rmn.ProgressionFailingOverToCluster, | ||
| rmn.ProgressionWaitingForResourceRestore, | ||
| rmn.ProgressionEnsuringVolSyncSetup, | ||
| rmn.ProgressionSettingupVolsyncDest, | ||
| rmn.ProgressionWaitForReadiness, | ||
| rmn.ProgressionUpdatedPlacement, | ||
| rmn.ProgressionCompleted, | ||
| rmn.ProgressionCleaningUp, | ||
| rmn.ProgressionWaitOnUserToCleanUp, | ||
| } | ||
|
|
||
| var relocateProgressionList = []rmn.ProgressionStatus{ | ||
| rmn.ProgressionPreparingFinalSync, | ||
| rmn.ProgressionClearingPlacement, | ||
| rmn.ProgressionRunningFinalSync, | ||
| rmn.ProgressionFinalSyncComplete, | ||
| rmn.ProgressionEnsuringVolumesAreSecondary, | ||
| rmn.ProgressionWaitOnUserToCleanUp, | ||
| rmn.ProgressionCompleted, | ||
| rmn.ProgressionCleaningUp, | ||
| rmn.ProgressionWaitingForResourceRestore, | ||
| rmn.ProgressionWaitForReadiness, | ||
| rmn.ProgressionUpdatedPlacement, | ||
| rmn.ProgressionEnsuringVolSyncSetup, | ||
| rmn.ProgressionSettingupVolsyncDest, | ||
| } |
There was a problem hiding this comment.
IMHO, using Golang's enumerated type is better. It's more straightforward and spares the need to handle hardcoded lists. Simply refactoring ProgressionStatus in drplacementcontroler_types.go like so (and divide to groups):
type ProgressionStatus int
const (
ProgressionCompleted = iota
ProgressionCreatingMW
ProgressionUpdatingPlRule
ProgressionWaitForReadiness
ProgressionCleaningUp
ProgressionWaitOnUserToCleanUp
ProgressionCheckingFailoverPrerequisites
ProgressionFailingOverToCluster
ProgressionWaitForFencing
ProgressionWaitForStorageMaintenanceActivation
ProgressionPreparingFinalSync
ProgressionClearingPlacement
ProgressionRunningFinalSync
ProgressionFinalSyncComplete
ProgressionEnsuringVolumesAreSecondary
ProgressionWaitingForResourceRestore
ProgressionUpdatedPlacement
ProgressionEnsuringVolSyncSetup
ProgressionSettingupVolsyncDest
ProgressionEnsuringVolSyncDestSetup
ProgressionDeleting
ProgressionDeleted
ProgressionActionPaused
)
func (ps ProgressionStatus) String() string {
return [...]string{
"Completed",
"CreatingMW",
"UpdatingPlRule",
"WaitForReadiness",
"Cleaning Up",
"WaitOnUserToCleanUp",
"CheckingFailoverPrerequisites",
"FailingOverToCluster",
"WaitForFencing",
"WaitForStorageMaintenanceActivation",
"PreparingFinalSync",
"ClearingPlacement",
"RunningFinalSync",
"FinalSyncComplete",
"EnsuringVolumesAreSecondary",
"WaitingForResourceRestore",
"UpdatedPlacement",
"EnsuringVolSyncSetup",
"SettingUpVolSyncDest",
"EnsuringVolSyncDestSetup",
"Deleting",
"Deleted",
"Paused",
}[ps]
}
func (ps ProgressionStatus) EnumIndex() int {
return int(ps)
}There was a problem hiding this comment.
@raaizik it maybe better from golang point of view, but we are losing ordered lists for specific workflows.
Right now, I have explicitly defined sequences of steps (failoverProgressionList, relocateProgressionList etc.).
With the enum approach, I’d need to implement logic to determine valid transitions instead of iterating over a predefined list.
| progressionList = deployProgressionList | ||
| } | ||
|
|
||
| currentProgressionIndex := slices.Index(progressionList, d.instance.Status.Progression) |
There was a problem hiding this comment.
| currentProgressionIndex := slices.Index(progressionList, d.instance.Status.Progression) | |
| currentProgressionIndex := slices.Index(progressionList, d.savedInstanceStatus) |
There was a problem hiding this comment.
In my PR I am playing with in memory progression, not with the saved one. I am comparing the current "in memory" progression with the newer one, I want to set. It is still not the common solution we discussed.
| } | ||
|
|
||
| currentProgressionIndex := slices.Index(progressionList, d.instance.Status.Progression) | ||
| nextProgressionIndex := slices.Index(progressionList, nextProgression) |
There was a problem hiding this comment.
Instead of passing in nextProgression as an argument to this function, you could use:
| nextProgressionIndex := slices.Index(progressionList, nextProgression) | |
| nextProgressionIndex := slices.Index(progressionList, d.getProgression()) |
There was a problem hiding this comment.
@raaizik no, I can't use d.getProgression() in my specific case. I am before the setProgression in the block, where I am calling this function.
Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
Fixes: https://issues.redhat.com/browse/DFBUGS-612
We have encountered several bugs where DRPC progression unexpectedly reverts from advanced stages to earlier ones. In this particular case, it regresses during Failover from Completed to WaitingForReadiness due to conflicts in VRG generations. While these changes typically resolve themselves over time, users may find the regression alarming.
To address this, we will introduce a check before updating the DRPC status to prevent regression, ensuring it remains at the previous progression level. However, this safeguard will apply only when the Phase remains unchanged—when transitioning between Phases, progression can still move in either direction.