Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replication: reduce RPC calls when VR state is primary #280

Merged
merged 1 commit into from
Dec 15, 2022
Merged

replication: reduce RPC calls when VR state is primary #280

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions controllers/replication.storage/volumereplication_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,25 +255,30 @@ func (r *VolumeReplicationReconciler) Reconcile(ctx context.Context, req ctrl.Re
return reconcile.Result{}, err
}

// enable replication on every reconcile
if err = r.enableReplication(vr); err != nil {
logger.Error(err, "failed to enable replication")
setFailureCondition(instance)
msg := replication.GetMessageFromError(err)
uErr := r.updateReplicationStatus(instance, logger, getCurrentReplicationState(instance), msg)
if uErr != nil {
logger.Error(uErr, "failed to update volumeReplication status", "VRName", instance.Name)
}

return reconcile.Result{}, err
}

var replicationErr error
var requeueForResync bool

switch instance.Spec.ReplicationState {
case replicationv1alpha1.Primary:
replicationErr = r.markVolumeAsPrimary(vr)
// Avoid extra RPC calls as request will be requested again for
// updating the LastSyncTime in the status. The volume needs to be
// replication enabled and promoted only once, not always during
// each reconciliation.
if instance.Status.State != replicationv1alpha1.PrimaryState {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to also check if instance.Status.ObservedGeneration == instance.Generation, so that we do not process a stale Status. Although in this case I cannot think of a reason for the Status to be stale, unless it was flipped from Primary to Seconday and back to Primary (in which case the generation may not matter), it is safer to check the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can do that if it avoids corner cases, but here is the instance.Status.ObservedGeneration wont be equal to the instance.Generation as the status.ObservedGeneration is not updated yet right? let's take it in follow-up PR as it also requires extensive testing and also this PR is already getting tested for some time. @ShyamsundarR WDYT?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the instance.Generation will equal the instance.Status.ObservedGeneration as there is no change to spec. The reconcile only updates the status for the sync time, which will change the revision.

I am fine taking it into another PR as the generation match is not tested in other conditions as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, correct. Let's take it as a follow-up PR, as we need to test it verywell.

// enable replication only if its not primary
if err = r.enableReplication(vr); err != nil {
logger.Error(err, "failed to enable replication")
setFailureCondition(instance)
msg := replication.GetMessageFromError(err)
uErr := r.updateReplicationStatus(instance, logger, getCurrentReplicationState(instance), msg)
if uErr != nil {
logger.Error(uErr, "failed to update volumeReplication status", "VRName", instance.Name)
}

return reconcile.Result{}, err
}
replicationErr = r.markVolumeAsPrimary(vr)
}

case replicationv1alpha1.Secondary:
// For the first time, mark the volume as secondary and requeue the
Expand Down