Add provisioning state to output resources property #40
Conversation
|
|
||
| 1. Output resources do not have a ProvisioningStatus property. Instead, if any of the output resource deployment fails, we set the Provisioning state of the corresponding radius resource accordingly and do not store the output resources. With this approach, we lose the information on which output resources failed deployment since we aggregate failures into the owning Radius resource's provisioningStatus property. | ||
|
|
||
| 2. We do not record any of the output resources as part of the radius resource whose deployment failed, although we deployed them with an errored status. This leads to orphaned resources which should be manually cleaned up. |
There was a problem hiding this comment.
This can be fixed without an API change right?
There was a problem hiding this comment.
yes, no API changes needed.
| 2. We do not record any of the output resources as part of the radius resource whose deployment failed, although we deployed them with an errored status. This leads to orphaned resources which should be manually cleaned up. | ||
|
|
||
| This design proposes addition of ProvisioningStatus to outputResources so that, | ||
| once we add changes to track these resources in case of both successful and failed deployment to avoid orphaning resources, there will be clearer indication of what lead to failed deployment to the user. |
There was a problem hiding this comment.
I want to push back on one aspect of this. The operationStatus and operationResult are the things that describe the result of an operation. The resource itself describes the state of the resource. They have different purposes.
I'm saying this because we need to keep the purpose in mind when we're designing. For the example the error messages or other debugging information are not part of the resource, they are part of the operationStatus, and it should stay that way.
There was a problem hiding this comment.
I am ok with adding the provisioningState for output resources, but we should be careful how we're applying this reasoning.
|
|
||
| ### Goals | ||
|
|
||
| The goal is to record the output resources whether or not deployment succeeds, so that radius can still manage the cleanup of resources it failed to deploy successfully. |
There was a problem hiding this comment.
This is independent of the API change right?
There was a problem hiding this comment.
yes.. I was just trying to capture what lead to this.
|
|
||
| @doc("The resource provisioning failed") | ||
| Failed, | ||
| } |
There was a problem hiding this comment.
We shouldn't need a separate definition of this, it should be the same as .properties.provisioningState.
|
|
||
| @doc("Determines whether Radius manages the lifecycle of the underlying resource.") | ||
| radiusManaged?: boolean; | ||
|
|
There was a problem hiding this comment.
What would provisioningState be for a non-radius-managed resource?
| radiusManaged?: boolean; | ||
|
|
||
| @doc("Determines whether the resource was provisioned successfully") | ||
| provisioningStatus: ProvisioningStatus |
There was a problem hiding this comment.
This should be the same as properties.provisioningState. The goal is to reuse the concept, so it should be spelled the same.
| provisioningStatus: ProvisioningStatus | ||
| } | ||
|
|
||
| @doc("Provisioning Status of Output Resource") |
There was a problem hiding this comment.
Are these the only possible values? What will happen in the case where 2 output resources are rendered and the deployment of the 1st output resource fails? What will be the provisioningStatus of resource 2?
There was a problem hiding this comment.
Based on @rynowak 's feedback, I will just use the existing provisioning state enum with all its values.
If only 1st outputResource failed, only its provisioning status will be failed. And the parent radius resource will have a failed provisioningStatus too. resource 2 will have its status based on its own deployment being successful.
There was a problem hiding this comment.
Here is the spec:
The provisioningState property has only three terminal states: Succeeded , Failed and Canceled. If the resource returns no provisioningState, it is assumed to be Succeeded.
Any state other than above three terminal states is a non-terminal state (e.g. "PreparingVMDisk", "MountingDrives", "SelectingHosts", "Deleted", "Accepted" etc.). Each individual RP is able to define their own non-terminal / transitioning / ephemeral states that are set before the resource reaches terminal states.
|
|
||
| ## Development plan | ||
|
|
||
| There is an in-progress PR which brings in the proposed changes as part of fix for rad app delete. |
There was a problem hiding this comment.
Would be good to also specify the steps and link to your PR.
|
|
||
| ## Development plan | ||
|
|
||
| There is an in-progress PR which brings in the proposed changes as part of fix for rad app delete. |
There was a problem hiding this comment.
There would be some changes needed in the delete logic as well, right?
There was a problem hiding this comment.
Yes. radius-project/radius#7148
The PR needs to be updated based on the feedback I receive here, but it tests the idea out and worked in case of container deployment failures followed by a rad app delete. I also have to test it for some other scenarios such as deployment involving recipes.
|
This pull request is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
closing due to low priority on the issue. |
No description provided.