issue #6807: Retry failed create when using generateName #6830

sseago · 2023-09-15T19:48:39Z

When creating resources with generateName, apimachinery does not guarantee uniqueness when it appends the random suffix to the generateName stub, so if it fails with already exists error, we need to retry.

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #6807

Please indicate you've done the following:

[x ] Accepted the DCO. Commits without the DCO will delay acceptance.
[x ] 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.

codecov · 2023-09-15T20:03:15Z

Codecov Report

Merging #6830 (09be1f7) into main (541425b) will decrease coverage by 0.06%.
The diff coverage is 39.28%.

@@            Coverage Diff             @@
##             main    #6830      +/-   ##
==========================================
- Coverage   60.67%   60.61%   -0.06%     
==========================================
  Files         249      250       +1     
  Lines       26476    26498      +22     
==========================================
- Hits        16064    16063       -1     
- Misses       9268     9290      +22     
- Partials     1144     1145       +1

Files	Coverage Δ
pkg/podvolume/backupper.go	`83.26% <100.00%> (+0.25%)`	⬆️
pkg/podvolume/restorer.go	`90.15% <100.00%> (+0.15%)`	⬆️
pkg/repository/ensurer.go	`100.00% <100.00%> (ø)`
pkg/restore/dataupload_retrieve_action.go	`75.75% <100.00%> (ø)`
pkg/cmd/cli/backup/delete.go	`70.51% <0.00%> (ø)`
pkg/controller/gc_controller.go	`56.07% <0.00%> (ø)`
pkg/client/retry.go	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

kaovilai · 2023-09-15T20:08:23Z

pkg/cmd/cli/serverstatus/server_status.go

@@ -40,7 +42,9 @@ type DefaultServerStatusGetter struct {
 func (g *DefaultServerStatusGetter) GetServerStatus(kbClient kbclient.Client) (*velerov1api.ServerStatusRequest, error) {
 	created := builder.ForServerStatusRequest(g.Namespace, "", "0").ObjectMeta(builder.WithGenerateName("velero-cli-")).Result()

-	if err := kbClient.Create(context.Background(), created, &kbclient.CreateOptions{}); err != nil {
+	if err := retry.OnError(retry.DefaultRetry, apierrors.IsAlreadyExists, func() error {


I think it would make sense to check for generateName metadata before attempting to retry.

If object does not use generateName, retrying will just be redundant.

I'm only adding this code in places where generateName is always set. If this were in a reusable func, then it would make sense to check that, but since the reusable func would end up just being a thin wrapper around retry.OnError, I'm not sure it's warranted here. Also, most of these calls involve interface objects so we'd have to attempt to cast to ObjectMeta, then look for GenerateName. I'm leaning towards thinking we don't need this since every time we call OnError it's right after explicitly setting generateName.

Upon further reflection, I think this is a good idea, actually -- I'll move the retry logic into a util func which either does retries or a simple one-time call, depending on GenerateName/Name values.

kaovilai · 2023-09-15T20:25:51Z

I think it would be reasonable for velero to by default cleanup completed status CRs since we generate a lot of them.

kaovilai · 2023-09-15T20:26:24Z

like here #5683

sseago · 2023-09-15T20:31:05Z

I think status cleanup would be a separate thing. I'd like to limit this PR to fixing the GenerateName bug.

mateusoliveira43 · 2023-09-17T14:18:43Z

pkg/cmd/cli/backup/delete.go

@@ -22,9 +22,11 @@ import (

 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"


I would use camelCase: apierrors -> apiErrors

@mateusoliveira43 For package names (and package name aliases) the general convention is all lowercase

kaovilai · 2023-09-19T14:25:45Z

Related upstream k8s issue: kubernetes/kubernetes#115489

kaovilai · 2023-09-20T01:50:09Z

changelogs/unreleased/6830-sseago

@@ -0,0 +1 @@
+Retry failed create when using generateName


Suggested change

Retry failed create when using generateName

Retry create calls during restore when using generateName to resolve `the server was not able to generate a unique name for the object` errors.

@kaovilai This doesn't relate to restore. This is when Velero creates CRs with GenerateName. Anything we're restoring already has Name set, so GenerateName isn't involved.

kaovilai · 2023-09-20T02:03:12Z

pkg/client/retry.go~

What is this file for?

@kaovilai auto-created backup file that accidentally got added to the commit. Will remove.

kaovilai · 2023-09-20T02:06:21Z

pkg/client/retry.go

+
+func CreateRetryGenerateName(client kbclient.Client, ctx context.Context, obj kbclient.Object) error {
+	return CreateRetryGenerateNameWithFunc(obj, func() error {
+		return client.Create(ctx, obj, &kbclient.CreateOptions{})


Should CreateOptions be parameterized?

FieldManager could become handy in the future. Also, this maybe what we should be using to restore managedFields during create call.

@kaovilai There weren't any cases in the current code that used it, so it seemed simpler to avoid it. If we need to add CreateOptions in the future, we can add it. I can add it now (and just pass in the empty CreateOptions in all cases), but my thinking was that this was easier to read/maintain without it until we needed it. I don't have a strong opinion there, though.

This is fine as is, just read on the options and saw ability to add managedfields which you might be interested in

@kaovilai Yes, at some point we should probably open a separate issue to investigate better ways to restore managed fields. If we can avoid the extra Patch, that would be great. In any case, none of this code is used for resource restore, since we'll never restore anything that doesn't already have a name.

When creating resources with generateName, apimachinery does not guarantee uniqueness when it appends the random suffix to the generateName stub, so if it fails with already exists error, we need to retry. Signed-off-by: Scott Seago <[email protected]>

sseago marked this pull request as draft September 15, 2023 19:48

github-actions bot requested review from shubham-pampattiwar and ywk253100 September 15, 2023 19:48

github-actions bot assigned sseago Sep 15, 2023

sseago force-pushed the retry-generateName branch from 8106a6e to 9410057 Compare September 15, 2023 19:49

github-actions bot added the has-changelog label Sep 15, 2023

kaovilai requested changes Sep 15, 2023

View reviewed changes

mateusoliveira43 reviewed Sep 17, 2023

View reviewed changes

sseago force-pushed the retry-generateName branch 2 times, most recently from d3b90e9 to 3297d13 Compare September 19, 2023 13:51

sseago force-pushed the retry-generateName branch from 3297d13 to 21d892f Compare September 19, 2023 23:40

sseago marked this pull request as ready for review September 19, 2023 23:41

github-actions bot requested review from Lyndon-Li and reasonerjt September 19, 2023 23:41

kaovilai requested changes Sep 20, 2023

View reviewed changes

sseago force-pushed the retry-generateName branch from 21d892f to 4999e64 Compare September 20, 2023 19:41

kaovilai approved these changes Sep 20, 2023

View reviewed changes

shubham-pampattiwar previously approved these changes Sep 25, 2023

View reviewed changes

sseago dismissed shubham-pampattiwar’s stale review via 09be1f7 October 9, 2023 21:41

sseago force-pushed the retry-generateName branch from 4999e64 to 09be1f7 Compare October 9, 2023 21:41

shubham-pampattiwar approved these changes Oct 9, 2023

View reviewed changes

Lyndon-Li approved these changes Oct 11, 2023

View reviewed changes

shubham-pampattiwar merged commit 74ed994 into vmware-tanzu:main Oct 11, 2023
22 of 24 checks passed

shubham-pampattiwar mentioned this pull request Dec 13, 2023

Velero didn't retry on failed Restore CR status update, causing the CR to remain stuck in "InProgress" #7207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #6807: Retry failed create when using generateName #6830

issue #6807: Retry failed create when using generateName #6830

sseago commented Sep 15, 2023

codecov bot commented Sep 15, 2023 •

edited

Loading

kaovilai Sep 15, 2023

sseago Sep 15, 2023

kaovilai Sep 15, 2023

sseago Sep 15, 2023

kaovilai commented Sep 15, 2023

kaovilai commented Sep 15, 2023

sseago commented Sep 15, 2023

mateusoliveira43 Sep 17, 2023

sseago Sep 19, 2023

kaovilai commented Sep 19, 2023

kaovilai Sep 20, 2023

sseago Sep 20, 2023

kaovilai Sep 20, 2023

sseago Sep 20, 2023

kaovilai Sep 20, 2023

sseago Sep 20, 2023

kaovilai Sep 20, 2023

sseago Sep 20, 2023

kaovilai Sep 20, 2023

	Retry failed create when using generateName
	Retry create calls during restore when using generateName to resolve `the server was not able to generate a unique name for the object` errors.

issue #6807: Retry failed create when using generateName #6830

issue #6807: Retry failed create when using generateName #6830

Conversation

sseago commented Sep 15, 2023

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

codecov bot commented Sep 15, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaovilai commented Sep 15, 2023

kaovilai commented Sep 15, 2023

sseago commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaovilai commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 15, 2023 •

edited

Loading