Simplify database interface #8126

rynowak · 2024-12-12T17:41:59Z

Description

This change simplifes our database provider interface by removing the 'name' parameter.

Prior to this change, each database client was used to store and retrieve a single resource type's data. Every piece of code had to be aware of the resource type associated with the client, and make sure to get a different client for each resource type it interacted with.

This was complicated further by the fact that not all of our database providers implemented this restriction.

After removing this restriction, our code can be simplified all over the place. A lot of code that used to need the dataprovider.DataStoreProvider now just needs the store.StorageClient.

This change pushes the simplification as far as possible. I also added validation to places where we initialize controllers, because it was too easy to initialize a controller with missing data.

Type of change

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).

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.

radius-functional-tests · 2024-12-12T17:47:44Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	rynowak/radius
Commit ref	`a72530b`
Unique ID	funcbde3141ecd
Image tag	pr-funcbde3141ecd

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-funcbde3141ecd
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-funcbde3141ecd
dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-funcbde3141ecd
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcbde3141ecd
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcbde3141ecd
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...
❌ Failed to install Radius for ucp-cloud functional test. Please check the logs for more details
❌ ucp-cloud functional test failed. Please check the logs for more details
❌ Failed to install Radius for corerp-cloud functional test. Please check the logs for more details
❌ corerp-cloud functional test failed. Please check the logs for more details

codecov · 2024-12-12T17:53:14Z

Codecov Report

Attention: Patch coverage is 63.38583% with 93 lines in your changes missing coverage. Please review.

Project coverage is 59.46%. Comparing base (186800b) to head (3d1f9f6).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/armrpc/asyncoperation/controller/controller.go	0.00%	9 Missing ⚠️
pkg/armrpc/asyncoperation/worker/service.go	0.00%	8 Missing ⚠️
pkg/armrpc/frontend/server/service.go	0.00%	7 Missing ⚠️
pkg/dynamicrp/options.go	0.00%	7 Missing ⚠️
pkg/server/asyncworker.go	0.00%	7 Missing ⚠️
pkg/ucp/dataprovider/storageprovider.go	85.71%	4 Missing and 3 partials ⚠️
pkg/ucp/frontend/api/server.go	0.00%	7 Missing ⚠️
pkg/server/apiservice.go	0.00%	6 Missing ⚠️
pkg/ucp/backend/service.go	62.50%	6 Missing ⚠️
pkg/armrpc/frontend/server/handler.go	37.50%	5 Missing ⚠️
... and 9 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8126      +/-   ##
==========================================
- Coverage   59.66%   59.46%   -0.20%     
==========================================
  Files         577      577              
  Lines       38573    38644      +71     
==========================================
- Hits        23013    22981      -32     
- Misses      13914    13996      +82     
- Partials     1646     1667      +21

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

pkg/armrpc/asyncoperation/worker/registry.go

pkg/armrpc/frontend/controller/controller.go

pkg/ucp/dataprovider/storageprovider.go

pkg/armrpc/asyncoperation/controller/controller.go

pkg/armrpc/asyncoperation/worker/worker_runoperation_test.go

lakshmimsft · 2024-12-13T00:46:16Z

will there be separate PR for name updates proposed here: https://gist.github.com/rynowak/0e625521404326eb194adc98155a736b

rynowak · 2024-12-13T19:48:36Z

Functional test failure is due to applications rp crashing at startup. I'll dig into it.

This change simplifes our database provider interface by removing the 'name' parameter. Prior to this change, each database client was used to store and retrieve a single resource type's data. Every piece of code had to be aware of the resource type associated with the client, and make sure to get a different client for each resource type it interacted with. This was complicated further by the fact that not all of our database providers implemented this restriction. --- After removing this restriction, our code can be simplified all over the place. A lot of code that used to need the `dataprovider.DataStoreProvider` now just needs the `store.StorageClient`. This change pushes the simplification as far as possible. I also added validation to places where we initialize controllers, because it was too easy to initialize a controller with missing data. Signed-off-by: Ryan Nowak <[email protected]>

radius-functional-tests · 2024-12-16T22:19:43Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	rynowak/radius
Commit ref	`e23467b`
Unique ID	funcac1f4cf5e7
Image tag	pr-funcac1f4cf5e7

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-funcac1f4cf5e7
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-funcac1f4cf5e7
dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-funcac1f4cf5e7
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcac1f4cf5e7
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcac1f4cf5e7
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

radius-functional-tests · 2024-12-16T22:32:39Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	rynowak/radius
Commit ref	`3d1f9f6`
Unique ID	funce196a6cdad
Image tag	pr-funce196a6cdad

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-funce196a6cdad
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-funce196a6cdad
dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-funce196a6cdad
controller test image location: ghcr.io/radius-project/dev/controller:pr-funce196a6cdad
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funce196a6cdad
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

lakshmimsft · 2024-12-16T22:48:45Z

pkg/ucp/dataprovider/storageprovider.go

+	return &DataStorageProvider{result: result{client: client}}
+}
+
+// GetStorageClient returns a storage client for the given resource type.


Comment needs update since resource type is not an input for client generated

rynowak requested review from a team as code owners December 12, 2024 17:42

rynowak temporarily deployed to publish-bicep December 12, 2024 17:42 — with GitHub Actions Inactive

rynowak temporarily deployed to functional-tests December 12, 2024 17:46 — with GitHub Actions Inactive

ytimocin reviewed Dec 12, 2024

View reviewed changes

pkg/armrpc/asyncoperation/worker/registry.go Show resolved Hide resolved

pkg/armrpc/frontend/controller/controller.go Show resolved Hide resolved

pkg/ucp/dataprovider/storageprovider.go Show resolved Hide resolved

lakshmimsft reviewed Dec 12, 2024

View reviewed changes

pkg/armrpc/asyncoperation/controller/controller.go Show resolved Hide resolved

rynowak commented Dec 12, 2024

View reviewed changes

pkg/armrpc/asyncoperation/worker/worker_runoperation_test.go Show resolved Hide resolved

rynowak force-pushed the rynowak/simplify-database branch from a72530b to afc96c6 Compare December 16, 2024 21:16

rynowak temporarily deployed to publish-bicep December 16, 2024 21:16 — with GitHub Actions Inactive

rynowak force-pushed the rynowak/simplify-database branch from afc96c6 to e23467b Compare December 16, 2024 22:04

rynowak temporarily deployed to publish-bicep December 16, 2024 22:04 — with GitHub Actions Inactive

rynowak temporarily deployed to functional-tests December 16, 2024 22:18 — with GitHub Actions Inactive

rynowak force-pushed the rynowak/simplify-database branch from e23467b to 3d1f9f6 Compare December 16, 2024 22:19

rynowak temporarily deployed to publish-bicep December 16, 2024 22:19 — with GitHub Actions Inactive

rynowak temporarily deployed to functional-tests December 16, 2024 22:31 — with GitHub Actions Inactive

lakshmimsft reviewed Dec 16, 2024

View reviewed changes

lakshmimsft approved these changes Dec 16, 2024

View reviewed changes

rynowak merged commit 4f2f16e into radius-project:main Dec 16, 2024
29 checks passed

rynowak deleted the rynowak/simplify-database branch December 17, 2024 00:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify database interface #8126

Simplify database interface #8126

rynowak commented Dec 12, 2024

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

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

lakshmimsft commented Dec 13, 2024

rynowak commented Dec 13, 2024

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

edited

Loading

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

edited

Loading

lakshmimsft Dec 16, 2024

rynowak Dec 16, 2024

Simplify database interface #8126

Simplify database interface #8126

Conversation

rynowak commented Dec 12, 2024

Description

Type of change

Contributor checklist

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

Radius functional test overview

Test Status

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

lakshmimsft commented Dec 13, 2024

rynowak commented Dec 13, 2024

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

Radius functional test overview

Test Status

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

Radius functional test overview

Test Status

lakshmimsft Dec 16, 2024

Choose a reason for hiding this comment

rynowak Dec 16, 2024

Choose a reason for hiding this comment

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

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

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

edited

Loading

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

edited

Loading