Make CreateOrUpdate controller async for link resources #5006

vishwahiremat · 2023-01-23T21:44:50Z

Description

Refactor to have single DeploymentObject for linkrp and corerp
refactor the deployment.Delete signature to take only outputresources
add a new async controller for createorupdate for linkrp

Issue reference

Fixes: https://dev.azure.com/azure-octo/Incubations/_workitems/edit/5788

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Adds necessary unit tests for change
Adds necessary E2E tests for change
Unit tests passing
Extended the documentation / Created issue for it

…tputResources instead of ResourceData

pkg/corerp/datamodel/application.go

snehabandla · 2023-01-25T18:34:38Z

pkg/linkrp/frontend/deployment/deploymentprocessor.go

-	SecretValues   map[string]rp.SecretValueReference
-	RecipeData     datamodel.RecipeData
-}
+// type DeploymentOutput struct {


Was this supposed to be commented out?

youngbupark · 2023-01-26T01:01:16Z

pkg/armrpc/frontend/controller/operation.go

+
+// AsyncOperationTimeout returns the timeput for the operation.
+func (b *Operation[P, T]) AsyncOperationTimeout() time.Duration {
+	if b.resourceOptions.AsyncOperationTimeout == 0 {


Can you please add the simple unittest for this ?

Suggested change

if b.resourceOptions.AsyncOperationTimeout == 0 {

if b.resourceOptions.AsyncOperationTimeout == time.Time{} {

youngbupark · 2023-01-26T01:01:59Z

pkg/corerp/frontend/handler/routes.go

@@ -332,6 +333,7 @@ func AddRoutes(ctx context.Context, router *mux.Router, pathBase string, isARM b
 							rp_frontend.PrepareRadiusResource[*datamodel.VolumeResource],
 							vol_ctrl.ValidateRequest,
 						},
+						AsyncOperationTimeout: time.Duration(2) * time.Minute,


this is unnecessary.

youngbupark · 2023-01-26T01:03:04Z

I made huge change in the morning. Please rebase this branch with main.

vishwahiremat · 2023-01-26T03:57:10Z

I made huge change in the morning. Please rebase this branch with main.

Rebased with main.

snehabandla · 2023-01-26T18:08:52Z

pkg/armrpc/frontend/controller/operation.go

+// AsyncOperationTimeout returns the timeput for the operation.
+func (b *Operation[P, T]) AsyncOperationTimeout() time.Duration {
+	if b.resourceOptions.AsyncOperationTimeout == 0 {
+		return time.Duration(2) * time.Minute


Can this be a constant?

mishrapratikshya · 2023-01-27T02:02:49Z

pkg/armrpc/frontend/controller/operation.go

+// AsyncOperationTimeout returns the timeput for the operation.
+func (b *Operation[P, T]) AsyncOperationTimeout() time.Duration {
+	if b.resourceOptions.AsyncOperationTimeout == 0 {
+		return time.Duration(2) * time.Minute


is there a reason for the 2 min AsyncOperationTimeout?

this is the default timeout for all the resources, and we can override it for each resource by sending a timeout for each operation.

mishrapratikshya · 2023-01-27T02:10:13Z

pkg/corerp/frontend/handler/routes.go

@@ -362,6 +362,9 @@ func AddRoutes(ctx context.Context, router *mux.Router, pathBase string, isARM b
 					frontend_ctrl.ResourceOptions[datamodel.VolumeResource]{
 						RequestConverter:  converter.VolumeResourceModelFromVersioned,
 						ResponseConverter: converter.VolumeResourceModelToVersioned,
+						UpdateFilters: []frontend_ctrl.UpdateFilter[datamodel.VolumeResource]{


Should this be a delete filter?

I think it should. Nice catch.

figuered delete/update filters are not required and removed it

mishrapratikshya · 2023-01-27T02:24:40Z

pkg/linkrp/types.go

@@ -2,8 +2,38 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 // ------------------------------------------------------------
+


The file mostly has recipe related data. shouldn't this be in a recipe file instead? we have a task for it. #5035

I think its better to move these to linkrp as these are used by differnet packages inside linkrp to avoid cyclic dependency

mishrapratikshya · 2023-01-27T02:27:52Z

pkg/linkrp/types.go

+	// Parameters are key/value parameters to pass into the recipe at deployment
+	Parameters map[string]any `json:"parameters,omitempty"`
+}
+
 const (
 	DaprInvokeHttpRoutesResourceType  = "Applications.Link/daprInvokeHttpRoutes"


ytimocin

Added a few comments and questions but looks good to me so far. Let me know when this PR takes its final shape and then I can review it again. Thanks for this change.

ytimocin · 2023-01-27T16:54:09Z

pkg/armrpc/frontend/controller/controller.go

@@ -54,6 +55,9 @@ type ResourceOptions[T any] struct {

 	// UpdateFilters is a slice of filters that execute prior to updating a resource.
 	UpdateFilters []UpdateFilter[T]
+
+	// AsyncOperationTimeout is the default timeout duration of async put operation.
+	AsyncOperationTimeout time.Duration


Is this the best place to put this property?

I think so, as this can accessed for all the operations for the resource.

ytimocin · 2023-01-27T16:55:01Z

pkg/armrpc/frontend/controller/operation.go

+// AsyncOperationTimeout returns the timeput for the operation.
+func (b *Operation[P, T]) AsyncOperationTimeout() time.Duration {
+	if b.resourceOptions.AsyncOperationTimeout == 0 {
+		return time.Duration(2) * time.Minute


There should be a Default const for this somewhere in the code.

pkg/corerp/datamodel/application.go

ytimocin · 2023-01-27T16:59:18Z

pkg/corerp/frontend/handler/routes.go

@@ -362,6 +362,9 @@ func AddRoutes(ctx context.Context, router *mux.Router, pathBase string, isARM b
 					frontend_ctrl.ResourceOptions[datamodel.VolumeResource]{
 						RequestConverter:  converter.VolumeResourceModelFromVersioned,
 						ResponseConverter: converter.VolumeResourceModelToVersioned,
+						UpdateFilters: []frontend_ctrl.UpdateFilter[datamodel.VolumeResource]{


I think it should. Nice catch.

pkg/linkrp/backend/controller/createorupdateresource.go

ytimocin · 2023-01-27T17:08:33Z

pkg/linkrp/datamodel/linkmetadata.go

@@ -6,6 +6,7 @@
 package datamodel

 import (
+	"github.com/project-radius/radius/pkg/linkrp"


Could this create a cyclic dependency problem anytime soon?

in that case we can move it all to linkrp instead. I create a types.go inside linkrp to remove the cyclic dependency

ytimocin · 2023-01-27T17:13:17Z

pkg/linkrp/frontend/controller/rediscaches/v20220315privatepreview_test.go

@@ -30,19 +30,3 @@ func getTestModels20220315privatepreview() (input *v20220315privatepreview.Redis

 	return input, dataModel, output
 }
-
-func getTestModelsForGetAndListApis20220315privatepreview() (input *v20220315privatepreview.RedisCacheResource, dataModel *datamodel.RedisCache, output *v20220315privatepreview.RedisCacheResource) {


Why are we deleting this data for Get and List? Was this a wrong naming? Because we are moving the Put and Delete.

We dont use this anymore as we moved list and get as part of the generic controller

ytimocin · 2023-01-27T17:14:20Z

pkg/linkrp/frontend/handler/routes.go

+						UpdateFilters: []frontend_ctrl.UpdateFilter[datamodel.MongoDatabase]{
+							rp_frontend.PrepareRadiusResource[*datamodel.MongoDatabase],
+						},
+						AsyncOperationTimeout: time.Duration(10) * time.Minute,


This shouldn't affect the time of our functional tests, right?

yes thats correct, as out functional tests run in parallel

ytimocin · 2023-01-27T17:15:28Z

pkg/linkrp/frontend/handler/routes.go

+						UpdateFilters: []frontend_ctrl.UpdateFilter[datamodel.MongoDatabase]{
+							rp_frontend.PrepareRadiusResource[*datamodel.MongoDatabase],
+						},
+						AsyncOperationTimeout: time.Duration(15) * time.Minute,


ytimocin · 2023-01-27T17:16:46Z

pkg/linkrp/frontend/handler/routes.go

+					frontend_ctrl.ResourceOptions[datamodel.MongoDatabase]{
+						RequestConverter:  converter.MongoDatabaseDataModelFromVersioned,
+						ResponseConverter: converter.MongoDatabaseDataModelToVersioned,
+						UpdateFilters: []frontend_ctrl.UpdateFilter[datamodel.MongoDatabase]{


Not sure if I remember the details of UpdateFilters and DeleteFilters but should this be DeleteFilters?

figured we dont need delete/update filters and removed it

ytimocin

LGTM but left a few more comments and some may need addressing.
I would also get another thumbs up btw.

ytimocin · 2023-01-27T21:51:47Z

pkg/armrpc/frontend/controller/operation.go

+// AsyncOperationTimeout returns the timeput for the operation.
+func (b *Operation[P, T]) AsyncOperationTimeout() time.Duration {
+	if b.resourceOptions.AsyncOperationTimeout == 0 {
+		return defaultAsyncPutTimeout


but this function is not only used for put calls, right? delete timeout should be different?

currently we use the same default timeout duration for put and delete, i.e 2 mins and we can override it by providing timeout in routes

ytimocin · 2023-01-27T21:52:49Z

pkg/linkrp/backend/controller/createorupdateresource_test.go

+			nil,
+			nil,
+			errors.New("error getting object"),
+		},


Do you also want to test PATCH calls?

ytimocin · 2023-01-27T21:53:41Z

pkg/linkrp/backend/controller/deleteresource.go

+
+	deploymentDataModel, ok := dataModel.(rpv1.DeploymentDataModel)
+	if !ok {
+		return ctrl.NewFailedResult(v1.ErrorDetails{Message: "deployment data model conversion error"}), nil


Should we give more details? Like which data model and something like that?

ytimocin · 2023-01-27T21:56:57Z

pkg/linkrp/types.go

+// ResourceInfo name and id of the resource
+type ResourceInfo struct {
+	Name string `json:"name"`
+	ID   string `json:"id"`
+}
+
+type Runtime struct {
+	Kubernetes Kubernetes `json:"kubernetes,omitempty"`
+}
+
+type Kubernetes struct {
+	// Namespace is set to the applicationNamespace when the Link is application-scoped, and set to the environmentNamespace when the Link is environment scoped
+	Namespace string `json:"namespace"`
+	// EnvironmentNamespace is set to environment namespace.
+	EnvironmentNamespace string `json:"environmentNamespace"`
+}


If these are also being used by corerp, we should move them to the rp package.

these are currently used only in linkrp

pkg/linkrp/backend/controller/createorupdateresource.go

kachawla · 2023-01-28T00:10:51Z

pkg/linkrp/backend/controller/createorupdateresource.go

+	deploymentDataModel, ok := dataModel.(rpv1.DeploymentDataModel)
+	if !ok {
+		return ctrl.NewFailedResult(v1.ErrorDetails{Message: "deployment data model conversion error"}), err
+	}


We can move this check after L61 before rendering and deploying.

rynowak · 2023-01-28T00:32:41Z

Merging to unblock next round of work. The failures are flakiness on the delete path of an Azure resource.

# Description Addressing few pr comments from #5006 ## Issue reference  Fixes: #issue_number ## Checklist Please make sure you've completed the relevant tasks for this PR, out of the following list: * [x] Code compiles correctly * [ ] Adds necessary unit tests for change * [ ] Adds necessary E2E tests for change * [x] Unit tests passing * [ ] Extended the documentation / Created issue for it

# Description - Refactor to have single DeploymentObject for linkrp and corerp - refactor the deployment.Delete signature to take only outputresources - add a new async controller for createorupdate for linkrp ## Issue reference  Fixes: https://dev.azure.com/azure-octo/Incubations/_workitems/edit/5788 ## Checklist Please make sure you've completed the relevant tasks for this PR, out of the following list: * [x] Code compiles correctly * [ ] Adds necessary unit tests for change * [ ] Adds necessary E2E tests for change * [x] Unit tests passing * [ ] Extended the documentation / Created issue for it

# Description Addressing few pr comments from #5006 ## Issue reference  Fixes: #issue_number ## Checklist Please make sure you've completed the relevant tasks for this PR, out of the following list: * [x] Code compiles correctly * [ ] Adds necessary unit tests for change * [ ] Adds necessary E2E tests for change * [x] Unit tests passing * [ ] Extended the documentation / Created issue for it

vishwahiremat added 5 commits January 22, 2023 03:04

refactor to have single deployment output for corerp and linkrp

a0ca774

refactor to change the return type from void->error

14b4fa0

refactor to update deploymentprocessor.Delete signature to take in ou…

e64eab9

…tputResources instead of ResourceData

Making createorupdate apis for mongo and redis async

4a3b531

adding unit tests

779e69f

vishwahiremat requested review from kachawla and mishrapratikshya January 23, 2023 21:44

vishwahiremat requested a review from a team as a code owner January 23, 2023 21:44

mishrapratikshya reviewed Jan 23, 2023

View reviewed changes

pkg/corerp/datamodel/application.go Show resolved Hide resolved

vishwahiremat added 2 commits January 24, 2023 11:45

Merge branch 'main' into vishwahiremat/asyscCreateOrDelete

fd27b90

fix linter issues

7223dba

vishwahiremat requested review from youngbupark and ytimocin January 24, 2023 20:09

snehabandla reviewed Jan 25, 2023

View reviewed changes

vishwahiremat added 3 commits January 25, 2023 15:48

moving to defaultasyncPut

8a680aa

moving to defaultAsync for mongo and redis

f21ba19

remove default asyn delete timeout

c9d636b

youngbupark reviewed Jan 26, 2023

View reviewed changes

vishwahiremat added 2 commits January 25, 2023 18:23

resolving merge conflicts

91405bb

fix for review comments

7ef7d11

snehabandla reviewed Jan 26, 2023

View reviewed changes

mishrapratikshya reviewed Jan 27, 2023

View reviewed changes

ytimocin reviewed Jan 27, 2023

View reviewed changes

vishwahiremat force-pushed the vishwahiremat/asyscCreateOrDelete branch from 6dfac16 to 7ef7d11 Compare January 27, 2023 19:17

vishwahiremat added 3 commits January 27, 2023 11:22

updating reciped data in datamodel

a2f5d1a

resolve merge conflicts

bc8d8c9

addressing pr comments

e119321

ytimocin approved these changes Jan 27, 2023

View reviewed changes

added patch tests

af4dc96

kachawla reviewed Jan 28, 2023

View reviewed changes

rynowak merged commit 430a961 into main Jan 28, 2023

rynowak deleted the vishwahiremat/asyscCreateOrDelete branch January 28, 2023 00:31

vishwahiremat mentioned this pull request Jan 28, 2023

Follow up pr for linkrp backend controller. #5049

Merged

5 tasks

	if b.resourceOptions.AsyncOperationTimeout == 0 {
	if b.resourceOptions.AsyncOperationTimeout == time.Time{} {

Make CreateOrUpdate controller async for link resources #5006

Make CreateOrUpdate controller async for link resources #5006

Conversation

vishwahiremat commented Jan 23, 2023

Description

Issue reference

Checklist

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

youngbupark commented Jan 26, 2023

vishwahiremat commented Jan 26, 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

mishrapratikshya Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytimocin left a 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

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

ytimocin left a 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

rynowak commented Jan 28, 2023

mishrapratikshya Jan 27, 2023 •

edited

Loading