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

Add status patching retry configuration design. #8063

Merged

Conversation

kaovilai
Copy link
Contributor

@kaovilai kaovilai commented Jul 30, 2024

Signed-off-by: Tiger Kaovilai [email protected]

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Design for #7207

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@kaovilai
Copy link
Contributor Author

/kind changelog-not-required

@github-actions github-actions bot added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Jul 30, 2024
@github-actions github-actions bot requested a review from sseago July 30, 2024 23:28
@github-actions github-actions bot requested a review from ywk253100 July 30, 2024 23:28
@kaovilai kaovilai force-pushed the retry-patching-inprogress-design branch from e42441e to 15dc7af Compare July 30, 2024 23:28
@github-actions github-actions bot added the Area/Design Design Documents label Jul 30, 2024
@kaovilai kaovilai force-pushed the retry-patching-inprogress-design branch from 15dc7af to 653a2c8 Compare July 30, 2024 23:35
Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.07%. Comparing base (d9ca147) to head (eebc4af).
Report is 95 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8063      +/-   ##
==========================================
+ Coverage   58.88%   59.07%   +0.18%     
==========================================
  Files         351      364      +13     
  Lines       29367    30307     +940     
==========================================
+ Hits        17294    17904     +610     
- Misses      10639    10960     +321     
- Partials     1434     1443       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kaovilai kaovilai force-pushed the retry-patching-inprogress-design branch 2 times, most recently from 92fa2a6 to 4dfbacd Compare July 31, 2024 00:09
@kaovilai
Copy link
Contributor Author

Maybe this should also cover #7799?

@kaovilai kaovilai force-pushed the retry-patching-inprogress-design branch 2 times, most recently from 113939e to ca91cff Compare August 1, 2024 02:13
@sseago
Copy link
Collaborator

sseago commented Aug 1, 2024

@kaovilai I think 7799 should be handled separately. That addresses non-APIServer errors, where retry isn't required, and where status updates are possible -- i.e. that issue is where we're not trying to update status but we could. Here we have the opposite problem -- we're trying to update status but can't.

design/retry-patching-configuration_design.md Outdated Show resolved Hide resolved
design/retry-patching-configuration_design.md Outdated Show resolved Hide resolved
We will add retries with timeout to existing patch calls that moves a backup/restore from InProgress to a final status such as
- Completed
- PartiallyFailed
- Failed
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure there are cases here where the transition is to Failed or FailedValidation -- FailedValidation -- in those cases, I think we're mostly moving from New, so a future reconcile can just handle it normally. Although maybe retry is still appropriate there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Future reconcile isn't generally handled. So we should still retry or it can be stuck in a phase other than new. New or "" is the only phase being retried via future reconcile.

	switch restore.Status.Phase {
	case "", api.RestorePhaseNew:
		// only process new restores
	default:
		r.logger.WithFields(logrus.Fields{
			"restore": kubeutil.NamespaceAndName(restore),
			"phase":   restore.Status.Phase,
		}).Debug("Restore is not handled")
		return ctrl.Result{}, nil
	}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kaovilai well the various WaitingForPluginOperations will also be retried on future reconcile, since the backup/restore operations controller re-reconcile to check progress on a periodic basis rather than on a cr-updated basis.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But those aren't handled by the backup/restore controller, which only handles new CRs.

@kaovilai kaovilai requested a review from sseago August 2, 2024 02:21
@kaovilai kaovilai changed the title Add retry patching configuration design. Add status patching retry configuration design. Aug 2, 2024
@kaovilai kaovilai force-pushed the retry-patching-inprogress-design branch from 71b9ac3 to 0e967e4 Compare August 2, 2024 02:28

and from above non final phases to
- Completed
- PartiallyFailed
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we could retry for WaitingForPluginOperations->final -- but since the backup/restore operations controllers re-reconcile all WaitingForPluginOperations CRs every 10 seconds by default anyway, it's probably unnecessary. Finalizing CRs definitely need retry though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easily removable if deemed unnecessary and already covered by opreation controllers

sseago
sseago previously approved these changes Aug 9, 2024
Copy link
Contributor Author

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

feedbacks from Shubham

design/retry-patching-configuration_design.md Outdated Show resolved Hide resolved
design/retry-patching-configuration_design.md Outdated Show resolved Hide resolved
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
@kaovilai kaovilai force-pushed the retry-patching-inprogress-design branch from 0f3c911 to eebc4af Compare August 28, 2024 15:19
@kaovilai
Copy link
Contributor Author

fixed DCO

@reasonerjt reasonerjt merged commit 8ae667e into vmware-tanzu:main Sep 3, 2024
8 checks passed
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 4, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 9, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 9, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 10, 2024
Signed-off-by: Tiger Kaovilai <[email protected]>

update to design vmware-tanzu#8063

Signed-off-by: Tiger Kaovilai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/Design Design Documents kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants