Add retry logic to UCP `GetAWSResourceWithPost` handler #8170

willdavsmith · 2024-12-26T23:11:31Z

Description

We've seen flaky functional test failures with AWS S3: #5963

This PR adds retries to the handler that I think is causing this 404 error.

Add pkg/retry directory for standard retries
Use pkg/retry in UCP GetAWSResourceWithPost handler

Type of change

This pull request fixes a bug in Radius and has an approved issue (issue link required).
This pull request adds or changes features of Radius and has an approved issue (issue link required).
This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional).

Fixes: #7352

Contributor checklist

Please verify that the PR meets the following requirements, where applicable:

An overview of proposed schema changes is included in a linked GitHub issue.
A design document PR is created in the design-notes repository, if new APIs are being introduced.
If applicable, design document has been reviewed and approved by Radius maintainers/approvers.
A PR for the samples repository is created, if existing samples are affected by the changes in this PR.
A PR for the documentation repository is created, if the changes in this PR affect the documentation or any user facing updates are made.
A PR for the recipes repository is created, if existing recipes are affected by the changes in this PR.

Signed-off-by: willdavsmith <willdavsmith@gmail.com>

codecov · 2024-12-27T03:34:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.01%. Comparing base (a1782fc) to head (77fba24).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8170      +/-   ##
==========================================
+ Coverage   59.95%   60.01%   +0.05%     
==========================================
  Files         590      591       +1     
  Lines       39513    39554      +41     
==========================================
+ Hits        23690    23737      +47     
+ Misses      14058    14054       -4     
+ Partials     1765     1763       -2

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

radius-functional-tests · 2024-12-27T15:11:14Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	willdavsmith/radius
Commit ref	`395ecce`
Unique ID	funcd8bba2bb19
Image tag	pr-funcd8bba2bb19

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr:
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcd8bba2bb19
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcd8bba2bb19
dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-funcd8bba2bb19
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcd8bba2bb19
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcd8bba2bb19
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ corerp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded

willdavsmith · 2024-12-27T15:15:13Z

go.mod

@@ -139,6 +139,7 @@ require (
 	github.com/sagikazarmark/locafero v0.6.0 // indirect
 	github.com/sagikazarmark/slog-shim v0.1.0 // indirect
 	github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 // indirect
+	github.com/sethvargo/go-retry v0.3.0 // indirect


I added this package to simplify our retry logic across the project. Looks like it is well tested with no dependencies so I think it is a good choice. Let's discuss in this PR

Last commit seems to be from 6 months ago. Just wondering if that could be an issue.

I think it should be okay. The code is straightforward and has no dependencies, so hopefully there shouldn't need to be too many new commits.

willdavsmith · 2024-12-27T15:16:33Z

pkg/retry/retry.go

+}
+
+// NewNoOpRetryer creates a new Retryer that does not retry.
+func NewNoOpRetryer() *Retryer {


this is useful for testing. using this retryer should be the same functionality as we have today.

willdavsmith · 2024-12-27T15:17:46Z

pkg/ucp/frontend/controller/awsproxy/createorupdateawsresource.go

@@ -125,7 +125,6 @@ func (p *CreateOrUpdateAWSResource) Run(ctx context.Context, w http.ResponseWrit

 	if existing {
 		// Get resource type schema
-


extra space

willdavsmith · 2024-12-27T15:18:04Z

pkg/ucp/frontend/controller/awsproxy/createorupdateawsresource.go

@@ -87,7 +87,7 @@ func (p *CreateOrUpdateAWSResource) Run(ctx context.Context, w http.ResponseWrit
 	}

 	cloudControlOpts := []func(*cloudcontrol.Options){CloudControlRegionOption(region)}
-	cloudFormationOpts := []func(*cloudformation.Options){CloudFormationWithRegionOption(region)}


I renamed this function to match the cloudcontrol version

willdavsmith · 2024-12-27T15:19:09Z

pkg/ucp/frontend/controller/awsproxy/getawsresourcewithpost_test.go

@@ -74,7 +75,7 @@ func Test_GetAWSResourceWithPost(t *testing.T) {
 		CloudControl:   testOptions.AWSCloudControlClient,
 		CloudFormation: testOptions.AWSCloudFormationClient,
 	}
-	awsController, err := NewGetAWSResourceWithPost(armrpc_controller.Options{DatabaseClient: testOptions.DatabaseClient}, awsClients)
+	awsController, err := NewGetAWSResourceWithPost(armrpc_controller.Options{DatabaseClient: testOptions.DatabaseClient}, awsClients, retry.NewNoOpRetryer())


this should match the functionality we have today, i.e. these tests should pass with no other changes.

willdavsmith · 2024-12-27T15:20:23Z

pkg/ucp/frontend/controller/awsproxy/getawsresourcewithpost.go

 }

 // NewGetAWSResourceWithPost creates a new GetAWSResourceWithPost controller with the given options and AWS clients.
-func NewGetAWSResourceWithPost(opts armrpc_controller.Options, awsClients ucpaws.Clients) (armrpc_controller.Controller, error) {
+func NewGetAWSResourceWithPost(opts armrpc_controller.Options, awsClients ucpaws.Clients, retryer *retry.Retryer) (armrpc_controller.Controller, error) {


we could consider adding the retryer to the other ucp awsproxy routes too, either in the future or this PR. I wanted to get some feedback first

rynowak · 2024-12-28T21:58:25Z

I'm sure this is written up somewhere already, but I'd like to understand the failure pattern that we're addressing with this change.

Is it something like this? (please help me fill in the blanks)

A PUT operation is initiated.
The PUT operation succeeds asynchronously.
A GET operation against the same resource then fails with a 404.
At some point the future, the same GET operation will succeed if retried, without initiating any additional operations.

The background context is that any multi-regional control plane is eventually consistent. Azure/ARM/Bicep has a similar eventually consistent behavior underneath it (see notes above), and it's mostly hidden from users via the deployment engine.

willdavsmith · 2025-01-07T23:12:26Z

I'm sure this is written up somewhere already, but I'd like to understand the failure pattern that we're addressing with this change.

Is it something like this? (please help me fill in the blanks)

A PUT operation is initiated.

The PUT operation succeeds asynchronously.

A GET operation against the same resource then fails with a 404.

At some point the future, the same GET operation will succeed if retried, without initiating any additional operations.

The background context is that any multi-regional control plane is eventually consistent. Azure/ARM/Bicep has a similar eventually consistent behavior underneath it (see notes above), and it's mostly hidden from users via the deployment engine.

This is exactly what I think is happening. I noticed that in the cases that I was investigating, the resource was actually created but returned a 404 on this route during deployment. My understanding of the DE is that it will perform a PUT operation, monitor the operation, and then do a GET at the end, where it calls UCP (getawsresourcewithpost handler) and returns a 404 because AWS says the resource doesn't exist yet. My hope is that adding retries here will make this situation more reliable without too much overhead. We can verify that it works if we see this issue less in the future, but until then, this is pretty much an educated guess as to what's happening and the solution.

ytimocin · 2025-01-18T05:29:38Z

go.mod

@@ -139,6 +139,7 @@ require (
 	github.com/sagikazarmark/locafero v0.6.0 // indirect
 	github.com/sagikazarmark/slog-shim v0.1.0 // indirect
 	github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 // indirect
+	github.com/sethvargo/go-retry v0.3.0 // indirect


Last commit seems to be from 6 months ago. Just wondering if that could be an issue.

ytimocin · 2025-01-18T05:32:51Z

pkg/retry/retry.go

+	}
+
+	return &Retryer{
+		config: retryConfig,


Can retryConfig ever be just empty? Like config is not nil but config.BackOffStrategy is?

Is that an okay case?

good find - I changed the code to not allow this case. now the retryer will use the default configuration in this case. I also added a test

ytimocin · 2025-01-18T05:33:53Z

pkg/retry/retry_test.go

+func TestNewRetryer(t *testing.T) {
+	config := &RetryConfig{
+		BackoffStrategy: goretry.NewConstant(1 * time.Second),
+	}


We can test this with a RetryConfig that has a nil BackOffStrategy.

added a test

Signed-off-by: willdavsmith <willdavsmith@gmail.com>

radius-functional-tests · 2025-01-27T19:09:45Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	willdavsmith/radius
Commit ref	`9931f87`
Unique ID	func1a65c2612f
Image tag	pr-func1a65c2612f

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr:
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func1a65c2612f
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func1a65c2612f
dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-func1a65c2612f
controller test image location: ghcr.io/radius-project/dev/controller:pr-func1a65c2612f
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func1a65c2612f
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

radius-functional-tests · 2025-01-27T20:03:31Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	willdavsmith/radius
Commit ref	`3850ee9`
Unique ID	funcc4ef32c5e8
Image tag	pr-funcc4ef32c5e8

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr:
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcc4ef32c5e8
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcc4ef32c5e8
dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-funcc4ef32c5e8
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcc4ef32c5e8
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcc4ef32c5e8
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

radius-functional-tests · 2025-01-27T22:28:39Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	willdavsmith/radius
Commit ref	`77fba24`
Unique ID	funcc8185c93f0
Image tag	pr-funcc8185c93f0

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr:
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcc8185c93f0
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcc8185c93f0
dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-funcc8185c93f0
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcc8185c93f0
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcc8185c93f0
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

Add AWS retry logic to GetWithPost

d537fcf

Signed-off-by: willdavsmith <willdavsmith@gmail.com>

willdavsmith requested review from a team as code owners December 26, 2024 23:11

willdavsmith temporarily deployed to publish-bicep December 26, 2024 23:11 — with GitHub Actions Inactive

fix test

395ecce

Signed-off-by: willdavsmith <willdavsmith@gmail.com>

willdavsmith temporarily deployed to publish-bicep December 27, 2024 03:20 — with GitHub Actions Inactive

willdavsmith temporarily deployed to functional-tests December 27, 2024 15:10 — with GitHub Actions Inactive

willdavsmith commented Dec 27, 2024

View reviewed changes

kachawla mentioned this pull request Jan 15, 2025

Scheduled functional test failed - Run ID: 12781601247 #8246

Closed

ytimocin reviewed Jan 18, 2025

View reviewed changes

willdavsmith added 2 commits January 27, 2025 10:53

Merge remote-tracking branch 'upstream/main' into aws-retry

a408ba7

PR

21f5cf9

Signed-off-by: willdavsmith <willdavsmith@gmail.com>

willdavsmith temporarily deployed to publish-bicep January 27, 2025 19:06 — with GitHub Actions Inactive

biceptypes

9931f87

Signed-off-by: willdavsmith <willdavsmith@gmail.com>

willdavsmith temporarily deployed to publish-bicep January 27, 2025 19:08 — with GitHub Actions Inactive

willdavsmith temporarily deployed to functional-tests January 27, 2025 19:09 — with GitHub Actions Inactive

willdavsmith requested a review from ytimocin January 27, 2025 19:14

ytimocin approved these changes Jan 27, 2025

View reviewed changes

Merge branch 'main' into aws-retry

3850ee9

willdavsmith temporarily deployed to publish-bicep January 27, 2025 19:44 — with GitHub Actions Inactive

willdavsmith temporarily deployed to functional-tests January 27, 2025 20:01 — with GitHub Actions Inactive

Merge branch 'main' into aws-retry

748893e

willdavsmith temporarily deployed to publish-bicep January 27, 2025 20:53 — with GitHub Actions Inactive

Merge branch 'main' into aws-retry

77fba24

willdavsmith temporarily deployed to publish-bicep January 27, 2025 22:18 — with GitHub Actions Inactive

willdavsmith temporarily deployed to functional-tests January 27, 2025 22:27 — with GitHub Actions Inactive

willdavsmith merged commit e4db991 into radius-project:main Jan 27, 2025
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retry logic to UCP `GetAWSResourceWithPost` handler #8170

Add retry logic to UCP `GetAWSResourceWithPost` handler #8170

willdavsmith commented Dec 26, 2024 •

edited

Loading

codecov bot commented Dec 27, 2024 •

edited

Loading

radius-functional-tests bot commented Dec 27, 2024 •

edited

Loading

willdavsmith Dec 27, 2024

ytimocin Jan 18, 2025

willdavsmith Jan 27, 2025

willdavsmith Dec 27, 2024

willdavsmith Dec 27, 2024

willdavsmith Dec 27, 2024

willdavsmith Dec 27, 2024

willdavsmith Dec 27, 2024

rynowak commented Dec 28, 2024

willdavsmith commented Jan 7, 2025

ytimocin Jan 18, 2025

ytimocin Jan 18, 2025

ytimocin Jan 18, 2025

willdavsmith Jan 27, 2025

ytimocin Jan 18, 2025

willdavsmith Jan 27, 2025

radius-functional-tests bot commented Jan 27, 2025 •

edited

Loading

radius-functional-tests bot commented Jan 27, 2025 •

edited

Loading

radius-functional-tests bot commented Jan 27, 2025 •

edited

Loading

		@@ -125,7 +125,6 @@ func (p *CreateOrUpdateAWSResource) Run(ctx context.Context, w http.ResponseWrit

		if existing {
		// Get resource type schema

Add retry logic to UCP GetAWSResourceWithPost handler #8170

Add retry logic to UCP GetAWSResourceWithPost handler #8170

Conversation

willdavsmith commented Dec 26, 2024 • edited Loading

Description

Type of change

Contributor checklist

codecov bot commented Dec 27, 2024 • edited Loading

Codecov Report

radius-functional-tests bot commented Dec 27, 2024 • edited Loading

Radius functional test overview

Test Status

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

rynowak commented Dec 28, 2024

willdavsmith commented Jan 7, 2025

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

radius-functional-tests bot commented Jan 27, 2025 • edited Loading

Radius functional test overview

Test Status

radius-functional-tests bot commented Jan 27, 2025 • edited Loading

Radius functional test overview

Test Status

radius-functional-tests bot commented Jan 27, 2025 • edited Loading

Radius functional test overview

Test Status

Add retry logic to UCP `GetAWSResourceWithPost` handler #8170

Add retry logic to UCP `GetAWSResourceWithPost` handler #8170

willdavsmith commented Dec 26, 2024 •

edited

Loading

codecov bot commented Dec 27, 2024 •

edited

Loading

radius-functional-tests bot commented Dec 27, 2024 •

edited

Loading

radius-functional-tests bot commented Jan 27, 2025 •

edited

Loading

radius-functional-tests bot commented Jan 27, 2025 •

edited

Loading

radius-functional-tests bot commented Jan 27, 2025 •

edited

Loading