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.

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

Conversation

yati1998
Copy link
Contributor

Avoid extra RPC calls as request will be requested again for updating the LastSyncTime in the status. The image need to be promoted only one time not always during the reconcile.

fixes: #250

Co-authored-by: Madhu Rajanna madhupr007@gmail.com
Signed-off-by: yati1998 ypadia@redhat.com

@yati1998
Copy link
Contributor Author

@Madhu-1 @ShyamsundarR

Comment on lines 277 to 278
// updating the LastSyncTime in the status. The image need to be
// promoted only one time not always during the reconcile.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// updating the LastSyncTime in the status. The image need to be
// promoted only one time not always during the reconcile.
// updating the LastSyncTime in the status. The volume needs to be
// Replication enabled and promoted only once, not always during each reconcilation.

Avoid extra RPC calls as request will be requested again for
updating the LastSyncTime in the status. The image need to be
promoted only one time not always during the reconcile.

fixes: csi-addons#250

Co-authored-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: yati1998 <ypadia@redhat.com>
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM, let's wait for the test results. If everything is good, we can merge this one.

// 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 {

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?

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.

@mergify mergify bot merged commit 97912f9 into csi-addons:main Dec 15, 2022
@yati1998
Copy link
Contributor Author

/cherry-pick release-4.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unwanted RPC calls due to LastSyncTime feature
3 participants