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

vrg: remove the use of captureInProgressStatusUpdate #1530

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

raghavendra-talur
Copy link
Member

@raghavendra-talur raghavendra-talur commented Aug 27, 2024

captureInProgressStatusUpdate was a function pointer that was an empty
function when the VRG was being reconciled as Primary and a call to
VRGConditionReasonUploading with status false when reconciling as
Secondary in the relocate case.

Simplify the code by removing the function pointer.

  • Remove the captureInProgressStatusUpdate parameter from
    kubeObjectsProtect
    kubeObjectsCaptureStartOrResumeOrDelay
    kubeObjectsCaptureStartOrResume
    kubeObjectsGroupCapture.

captureInProgressStatusUpdate was a function pointer that was an empty
function when the VRG was being reconciled as Primary and a call to
VRGConditionReasonUploading with status false when reconciling as
Secondary in the relocate case.

Simplify the code by removing the function pointer.

* Remove the captureInProgressStatusUpdate parameter from
  kubeObjectsProtect
  kubeObjectsCaptureStartOrResumeOrDelay
  kubeObjectsCaptureStartOrResume
  kubeObjectsGroupCapture.

Signed-off-by: Raghavendra Talur <raghavendra.talur@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

Regarding the commit message:

captureInProgressStatusUpdate was a function pointer that was an empty
function when the VRG was being reconciled as Primary and a call to
VRGConditionReasonUploading with status false when reconciling as
Secondary in the relocate case.

This is does not match the code. Previously this code did not consider the action. calling kubeObjectsProtectSecondary() sent a function caling kubeObjectsCaptureStatusFalse() regardless of the action.

This change call kubeObjectsCaptureStatusFalse() only during relocate. It may be correct due to the way we call kubeObjectsProtectSecondary() but it is not a refactoring - you changed the behavior. If you want to change the behavior do this in another commit explaining the change.

@@ -328,7 +312,12 @@ func (v *VRGInstance) kubeObjectsGroupCapture(
return
}

captureInProgressStatusUpdate()
if v.instance.Spec.ReplicationState == ramen.Secondary &&
v.instance.Spec.Action == ramen.VRGActionRelocate {
Copy link
Member

Choose a reason for hiding this comment

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

To keep the behviour of the previous code, remove the check for action == relocate.

To keep the code simpler and easier to follow, it will help to more the condition into v.kubeObjectsCaptureStatusFalse - we can always call it and it will do the right thing based on the vrg replication state.

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.

2 participants