Set PeerReady condition status to false upon relocation initiation #1020
Conversation
BenamarMk
force-pushed
the
set-peerready-status-true-upon-relocation-initiation
branch
from
edc6685
to
e018d5b
Compare
| @@ -741,7 +741,7 @@ func (d *DRPCInstance) RunRelocate() (bool, error) { | |||
| return !done, fmt.Errorf(errMsg) | |||
| } | |||
|
|
|||
| if !d.validatePeerReady() { | |||
| if d.getLastDRState() != rmn.Relocating && !d.validatePeerReady() { | |||
There was a problem hiding this comment.
Is this required because we set peer ready to false earlier, or additional change? If another change, can you separate it to another commit and explain the reason for this change?
There was a problem hiding this comment.
Reconcile will loop over Relocate multiple times as various states change (e.g current primary transitions to secondary etc.). As the PeerReady is set to false for the current generation of the spec, once relocation has started, a future loop will error out here. This additional check hence is required to pass this gate if we checked everything was fine with the prior generation and started relocation.
| @@ -1048,6 +1048,9 @@ func (d *DRPCInstance) relocate(preferredCluster, preferredClusterNamespace stri | |||
| d.setDRState(drState) | |||
There was a problem hiding this comment.
The comment above:
// Make sure we record the state that we are failing over
Is copy paste from failover code?
controllers/drplacementcontrol.go
Outdated
| @@ -1048,6 +1048,9 @@ func (d *DRPCInstance) relocate(preferredCluster, preferredClusterNamespace stri | |||
| d.setDRState(drState) | |||
| addOrUpdateCondition(&d.instance.Status.Conditions, rmn.ConditionAvailable, d.instance.Generation, | |||
| d.getConditionStatusForTypeAvailable(), string(d.instance.Status.Phase), "Starting relocation") | |||
| addOrUpdateCondition(&d.instance.Status.Conditions, rmn.ConditionPeerReady, d.instance.Generation, | |||
| metav1.ConditionFalse, rmn.ReasonNotStarted, | |||
| fmt.Sprintf("Started relocation to cluster %q", preferredCluster)) | |||
There was a problem hiding this comment.
The reason "ReasonNotStarted" contradicts the test "Started relocation".
There was a problem hiding this comment.
We could potentially reword this to "Relocation in progress to cluster %q"
There was a problem hiding this comment.
LGTM
An orthogonal question/observation:
EnsureCleanup marks PeerReady post its operations are successful. ensureCleanupAndVolSyncReplicationSetup sets up EnsureVolSyncReplicationSetup post cleanup. Should we not mark PeerReady post EnsureVolSyncReplicationSetup is also a success from a VolSync POV?
Not sure I understand the question. EnsureCleanup() is where we set the PeerReady as a success. We don't enter the volsync setup without ensuring that the peer is ready. |
Previously, the PeerReady condition status was set to false after the relocation setup between the source and target clusters was completed and before entering the clean up phase. This led to a misleading indication that the peer was ready for a new action before getting to that point. Consequently, users can initiate another action, assuming that the PeerReady condition being true meant it was ready for the next step. To rectify this issue, this commit ensures that as soon as the relocation action is initiated, the PeerReady condition status is immediately switched to false. This change accurately reflects the actual readiness of the peer, preventing any misconceptions about initiating additional actions prematurely. Signed-off-by: Benamar Mekhissi <bmekhiss@ibm.com>
BenamarMk
force-pushed
the
set-peerready-status-true-upon-relocation-initiation
branch
from
e018d5b
to
fd162a2
Compare
(just reiterating, does not impact this PR) So if volsync is not setup on the secondary, is it ready for a relocate? IOW should we declare DRPC PeerReady with Volsync protected volumes before the volsync setup is complete? (above is based on my understanding that we declare PeerReady true in DRPC post EnsureCleanup and pre EnsureVolSyncReplicationSetup) |
nirs
changed the title
Previously, the
PeerReadycondition status was set tofalseafter the relocation setup between the source and target clusters was completed and before entering the clean-up phase. This led to a misleading indication that the peer was ready for a new action before getting to that point. Consequently, users can initiate another action, assuming that thePeerReadycondition beingtruemeant it was ready for the next step.To rectify this issue, this commit ensures that as soon as the relocation action is initiated, the
PeerReadycondition status is immediately switched tofalse. This change accurately reflects the actual readiness of the peer, preventing any misconceptions about initiating additional actions prematurely.Fixes: bz 2138855