-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement provider cancellation #3224
Conversation
Attempt to gracefully shut down the provider if the engine indicates that we need to halt operations. Fixes #1055
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3224 +/- ##
==========================================
+ Coverage 55.28% 55.32% +0.03%
==========================================
Files 66 66
Lines 9917 9929 +12
==========================================
+ Hits 5483 5493 +10
- Misses 4002 4004 +2
Partials 432 432 ☔ View full report in Codecov by Sentry. |
@@ -1007,6 +1016,8 @@ func (k *azureNativeProvider) findUnsetPropertiesToMaintain(res *resources.Azure | |||
|
|||
// Read the current live state associated with a resource. | |||
func (k *azureNativeProvider) Read(ctx context.Context, req *rpc.ReadRequest) (*rpc.ReadResponse, error) { | |||
// Use the global context to handle provider shutdown. | |||
ctx = k.context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really correct to disregard the passed-in context? Can you explain why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly what aws-native and kubernetes do. I think by the time the request gets cancelled, we're way past being able to actually do anything to shut down cleanly. This is why the cancellation exists externally.
My first pass of this was to do something fancy with combining the contexts to make it an OR between the two cancellations, but I think it's significantly harder to reason about in practice and this appears sufficient from other providers.
Attempt to gracefully shut down the provider if the engine indicates that we need to halt operations.
Fixes #1055