search

radius-project / radius

Radius is a cloud-native, portable application platform that makes app development easier for teams building cloud-native apps.
https://radapp.io
Apache License 2.0
1.49k stars 95 forks source link

isNewResource check in the generic backend controller is not necessary #5335

Open ytimocin opened 1 year ago

ytimocin commented 1 year ago

Bug information

We have isNewResource check in the generic async backend controllers for createOrUpdate. That is unnecessary because at that point the object is already saved to the DB.

This one is called first: https://github.com/project-radius/radius/blob/main/pkg/armrpc/frontend/controller/operation.go#L161

And then this is called: https://github.com/project-radius/radius/blob/main/pkg/linkrp/backend/controller/createorupdateresource.go#L38

Steps to reproduce (required)

No specific way

Observed behavior (required)

Unnecessary code branch

Desired behavior (required)

Workaround (optional)

System information

rad Version (required)

Operating system (required)

Additional context

AB#6766

kachawla commented 1 year ago

Good catch @ytimocin. Won't this result into untracked output resources left behind if an existing Radius resource is re-deployed since https://github.com/project-radius/radius/blob/main/pkg/linkrp/backend/controller/createorupdateresource.go#L85 will be skipped?

ytimocin commented 1 year ago

Good catch @ytimocin. Won't this result into untracked output resources left behind if an existing Radius resource is re-deployed since https://github.com/project-radius/radius/blob/main/pkg/linkrp/backend/controller/createorupdateresource.go#L85 will be skipped?

If it is redeployed then it is going to go into this if case. We should just delete the if case because the flow always goes into it.

kachawla commented 1 year ago

Good catch @ytimocin. Won't this result into untracked output resources left behind if an existing Radius resource is re-deployed since https://github.com/project-radius/radius/blob/main/pkg/linkrp/backend/controller/createorupdateresource.go#L85 will be skipped?

If it is redeployed then it is going to go into this if case. We should just delete the if case because the flow always goes into it.

Let me sync up with you offline. I think the original output resources won't be tracked in the data store by this point since the resource stored in the data store is being overwritten by async put controller.

kachawla commented 1 year ago

Good catch @ytimocin. Won't this result into untracked output resources left behind if an existing Radius resource is re-deployed since https://github.com/project-radius/radius/blob/main/pkg/linkrp/backend/controller/createorupdateresource.go#L85 will be skipped?

If it is redeployed then it is going to go into this if case. We should just delete the if case because the flow always goes into it.

Let me sync up with you offline. I think the original output resources won't be tracked in the data store by this point since the resource stored in the data store is being overwritten by async put controller.

Yetkin and I synced up. This indeed is resulting into orphaned output resources, since entire resource including info about previously deployed output resources get overwritten by async put controller https://github.com/project-radius/radius/blob/main/pkg/armrpc/frontend/controller/operation.go#L161. We should prioritize fixing this.