Add recipe support to dynamic RP (#8191 Continued) #8267
Conversation
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
c1a1fa7 to
84c52fd
Compare
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
84c52fd to
1e58e76
Compare
lakshmimsft
had a problem deploying
to
publish-bicep
GitHub Actions
Error
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
1e58e76 to
b01b823
Compare
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
b01b823 to
c9480a3
Compare
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8267 +/- ##
==========================================
- Coverage 55.72% 55.65% -0.08%
==========================================
Files 595 599 +4
Lines 40481 40750 +269
==========================================
+ Hits 22557 22678 +121
- Misses 16211 16354 +143
- Partials 1713 1718 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
c9480a3 to
2e7dd87
Compare
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
2e7dd87 to
f6e4c54
Compare
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
f6e4c54 to
802adcb
Compare
lakshmimsft
had a problem deploying
to
publish-bicep
GitHub Actions
Error
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
802adcb to
ded457d
Compare
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
95b4bb4 to
1cf42a6
Compare
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
4f9db83 to
df4b407
Compare
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
| if status == nil { | ||
| status = new(rpv1.RecipeStatus) | ||
| } |
There was a problem hiding this comment.
I think I'm still confused why are we checking if it is nil or not first, something that's not being done for other inputs here. Are we trying to preserve status from a previous run but not rest of the values? And I understand that this was already done in the create/update controller as you pointed out, so this update it just moving it from controller to validator. But I don't understand why status is being handled differently. Let me know if you have more insights on that, else could you please log an issue for us to come back to it to understand it?
There was a problem hiding this comment.
tracking issue: #8922
| if recipeOutput != nil && recipeOutput.Status != nil { | ||
| setRecipeStatus(data, *recipeOutput.Status) | ||
| } |
There was a problem hiding this comment.
Shouldn't this have been during Process call above?
if recipeOutput != nil && recipeOutput.Status != nil {
setRecipeStatus(data, *recipeOutput.Status)
}
There was a problem hiding this comment.
Updated code to remove this and update recipe status in the dynamicrp Processor. The dynamicrp tests and functionality runs as expeced but it took a long time to debug issue with other portable types. They do not update the op resources and recipestatus in their processors in the expected location. Keeping current changes for now till dynamicrp processor is mainly in use.
| // setRecipeStatus sets the recipe status for the given resource model. | ||
| // It retrieves the resource metadata from the provided model, deep copies the current resource status, | ||
| // updates the Recipe field with the supplied recipeStatus, and then applies the updated status back to the resource. | ||
| func setRecipeStatus[P rpv1.RadiusResourceModel](data P, recipeStatus rpv1.RecipeStatus) { | ||
| rm := data.ResourceMetadata() | ||
| status := rm.GetResourceStatus().DeepCopy() | ||
| status.Recipe = &recipeStatus | ||
| rm.SetResourceStatus(status) | ||
| } |
There was a problem hiding this comment.
Based on our other discussion, this DeepCopy is only deep copuing the recipe status which you are replacing here, so I'm confused about this implementation. Can you help me understand why is the DeepCopy() necessary here?
There was a problem hiding this comment.
pending (creating tracking issue/investigate)
There was a problem hiding this comment.
Updated code to remove this block and update recipe status in the dynamicrp Processor. The dynamicrp tests and functionality runs as expeced but it took a long time to debug issue with other portable types. They do not update the op resources and recipestatus in their processors in the expected location. Keeping current changes for now till dynamicrp processor is mainly in use.
| if recipeDataModel.Recipe() != nil { | ||
| recipeDataModel.Recipe().DeploymentStatus = util.Success | ||
|
|
||
| recipeDataModel, supportsRecipes := any(data).(datamodel.RecipeDataModel) |
There was a problem hiding this comment.
This type assertion any(data).(datamodel.RecipeDataModel) is repeated multiple times including in executeRecipesIfNeeded. We can move this above L89 (recipeOutput, err := c.executeRecipeIfNeeded(ctx, data, previousOutputResources, config.Simulated)) and use the output everywhere else. supportsRecipes would be passed as an input to executeRecipeIfNeeded, or we can just skip that call if supportsRecipes evaluates to false.
There was a problem hiding this comment.
updated this to reduce repitition. kept executeRecipeIfNeeded() untouched for now since it needs data, recipeDataModel and supportRecipes
pkg/portableresources/backend/controller/createorupdateresource_test.go
Outdated
Show resolved
Hide resolved
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
This change implements recipe support fo the dynamic RP. With this feature, a user-defined-type can declare the SupportsRecipes capability, and then it will be processed by the recipe engine. The main change here is to the core datamodel interfaces used by our shared controllers. These interfaces have some problematic definitions that rely on returning pointers to go-structs so they can be mutated and saved to the database. This general approach does not work for UDT, or any other design besides the portable resources. To understand this problem in more detail, consider that ".properties.status" is user-defined for a UDT. The existing interfaces assume that all resources have the same go struct at ".properties.status". Because UDT is extensible, we cannot write a single go-struct that captures all possible status values, but that's required by the current interfaces. Clearly we need something more flexible. I took the approach of making minimal changes in this PR, but we should improve the design longer-term. Signed-off-by: Ryan Nowak <nowakra@gmail.com>
Signed-off-by: lakshmimsft <ljavadekar@microsoft.com>
Signed-off-by: lakshmimsft <ljavadekar@microsoft.com>
Signed-off-by: lakshmimsft <ljavadekar@microsoft.com>
Signed-off-by: lakshmimsft <ljavadekar@microsoft.com>
lakshmimsft
force-pushed
the
rynowak/recipe-engine
branch
from
24fbd9f to
ecedcb3
Compare
lakshmimsft
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
lakshmimsft
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
# Description This is a PR to address some pending investigations issues in #8267: #8267 (comment) #8267 (comment) and Test updates in #8924. #8924 (comment) #8924 (comment) #8924 (comment) Still a draft PR to make sure all tests work as expected. ## 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). Fixes: ## 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. - [ ] Yes - [ ] Not applicable - A design document PR is created in the [design-notes repository](https://github.com/radius-project/design-notes/), if new APIs are being introduced. - [ ] Yes - [ ] Not applicable - The design document has been reviewed and approved by Radius maintainers/approvers. - [ ] Yes - [ ] Not applicable - A PR for the [samples repository](https://github.com/radius-project/samples) is created, if existing samples are affected by the changes in this PR. - [ ] Yes - [ ] Not applicable - A PR for the [documentation repository](https://github.com/radius-project/docs) is created, if the changes in this PR affect the documentation or any user facing updates are made. - [ ] Yes - [ ] Not applicable - A PR for the [recipes repository](https://github.com/radius-project/recipes) is created, if existing recipes are affected by the changes in this PR. - [ ] Yes - [ ] Not applicable --------- Signed-off-by: lakshmimsft <ljavadekar@microsoft.com>
This is continuation of the previous PR (#8191)
All comments from previous PR will be addressed here.
Description
Original Description:
This change implements recipe support fo the dynamic RP. With this feature, a user-defined-type can declare the SupportsRecipes capability, and then it will be processed by the recipe engine.
The main change here is to the core datamodel interfaces used by our shared controllers. These interfaces have some problematic definitions that rely on returning pointers to go-structs so they can be mutated and saved to the database. This general approach does not work for UDT, or any other design besides the portable resources.
To understand this problem in more detail, consider that ".properties.status" is user-defined for a UDT. The existing interfaces assume that all resources have the same go struct at ".properties.status". Because UDT is extensible, we cannot write a single go-struct that captures all possible status values, but that's required by the current interfaces. Clearly we need something more flexible.
I took the approach of making minimal changes in this PR, but we should improve the design longer-term.
Type of change
Fixes: Part of #6688
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: