Context aware SDK with timeouts #276

appilon · 2019-12-11T03:33:02Z

This change would allow developers to switch CRUD funcs and CustomizeDiff funcs to ones that receives a context.Context, configured with the appropriate timeouts (in the case of CRUD). This PR makes some breaking changes to a few schema.Resource methods by adding the context.Context as the first argument. This likely won't cause problems in the wild because a schema.Resource's Apply/Refresh/etc methods were not really meant to be public (they are not useful to anyone).

This PR also forces cancellation through the new context API, removing the previous StopContext feature (this is a breaking change).

This is a PR against our working version2 branch. That branch I have taken the liberty of privatizing/internalizing and pruning dead code/code never intended for public use (cutting the internalized version for Core, and removing support for protocol 4). The PR represents the first enhancement w/ deliberate breaking change. PRs like this should go through a full review process and not just push directly to that branch.

appilon · 2019-12-11T03:39:11Z

~~Ignore the first commit of this PR, that is the last commit of the version2 branch I forgot to push... not sure how to get the PR to sync 🤔~~ fixed

UPDATE: common problem trying the 'Edit' trick
https://stackoverflow.com/questions/16306012/github-pull-request-showing-commits-that-are-already-in-target-branch

helper/schema/resource.go

helper/schema/shims_test.go

helper/schema/provider.go

paultyng · 2019-12-11T15:08:57Z

We may need the ability to opt out of timeouts, I'm not sure, we should check in with the Azure team and see how it goes with their implementation

paultyng · 2019-12-11T15:11:48Z

What impact will this have on StopContext? Should it be removed?

appilon · 2019-12-11T20:43:27Z

@paultyng Yes this as its coded doesn't feel just right, as every context has a timeout set even if none was configured (as I believe the timeouts all have some hardcoded 20 min duration if nothing was set?) An opt out would make sense

appilon · 2019-12-11T21:18:27Z

Seems we might have a flaky test:
TestWaitForState_inconsistent_negative Seems to involve randomness and a heuristic or two.. re-running test to ensure it was a one in a hundred thing

helper/schema/resource_test.go

helper/schema/provider.go

helper/schema/resource.go

paultyng · 2020-01-09T18:34:16Z

Is there anyway now that you have been in this code a bit to remove those dead functions with panics?

appilon · 2020-01-09T20:49:44Z

@paultyng As discussed it should be possible, but I'll address it in a follow up PR (maybe I can sneak it in at the end)

helper/schema/resource.go

kmoe

Approved provided everyone's happy on the timeouts issue, and any other review comments are addressed.

refactor codebase around Contextify() this drops the need for conditional evaluation of CRUD w/ context and treats that as the default codepath. One caveat is if a dev sets a noncontext and context func, the noncontext func is essentially ignored. I don't see the need to panic/error if both are set I think it should be obvious.

force cancellation through ctx API merge a server instanced context's cancellation with the GRPC request scoped context's cancellation

make all CustomizeDiff funcs to context aware (breaking change)

remove legacy terraform version as this SDK can never talk with Terraform < 0.12

placeholders. Made configure context aware

rearrange internal crud funcs happy path

add retroactive deprecated comment to MigrateState need to decide if the feature should be cut in v2 if this is the cause we should place the deprecated comment in the next v1 release.

update stop feature tests to have 5 second timeout this ensures any regressions are caught in tests otherwise the tests themselves deadlock when the implementation is broken. Passing tests execute near instantly.

helper/customdiff/testing_test.go

* implement Context aware CRUD(E) functions * set context timeouts according to CRUD func * add InternalValidate check for dually set CRUD * add test cases ensuring context aware funcs are called. * add NoopContext helper * add context to shims tests * breaking change: context aware CustomizeDiff * remove schema.Provider.StopContext feature * remove schema.Provider.MetaReset func * remove legacy terraform version as this SDK can never talk with Terraform < 0.12 * use UnaryInterceptor to configure new stop context * add deprecated comments to now legacy non context identifiers * add grpc cancel and Stop messag tests, as well as reset test * add context support to ImportState * breaking change: context aware StateUpgraders * add context support to ConfigureFunc

ghost · 2020-03-02T01:50:01Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

appilon changed the base branch from version2 to master December 11, 2019 03:42

appilon changed the base branch from master to version2 December 11, 2019 03:42

paultyng reviewed Dec 11, 2019

View reviewed changes

helper/schema/resource.go Outdated Show resolved Hide resolved

helper/schema/resource.go Show resolved Hide resolved

helper/schema/shims_test.go Outdated Show resolved Hide resolved

helper/schema/provider.go Outdated Show resolved Hide resolved

This was referenced Dec 11, 2019

SIGINT ("graceful shutdown") is unacceptably slow / doesn't always work #141

Closed

Implement graceful shutdown for helper/resource waiter #73

Closed

appilon added this to the v2.0.0 milestone Dec 11, 2019

appilon changed the title ~~Context aware CRUD funcs with timeouts~~ Context aware SDK with timeouts Dec 18, 2019

appilon commented Dec 19, 2019

View reviewed changes

helper/schema/resource_test.go Outdated Show resolved Hide resolved

paultyng reviewed Dec 19, 2019

View reviewed changes

helper/schema/provider.go Outdated Show resolved Hide resolved

helper/schema/resource.go Outdated Show resolved Hide resolved

appilon force-pushed the version2 branch from 5789756 to 4ab9a56 Compare January 6, 2020 17:10

appilon force-pushed the context-crud branch from 82fcbc7 to 42ded8a Compare January 6, 2020 17:10

appilon force-pushed the version2 branch from 4ab9a56 to 906276d Compare January 9, 2020 04:58

appilon force-pushed the context-crud branch from 4846761 to 6725355 Compare January 9, 2020 04:58

paultyng reviewed Jan 10, 2020

View reviewed changes

helper/schema/resource.go Show resolved Hide resolved

helper/schema/resource.go Outdated Show resolved Hide resolved

appilon force-pushed the version2 branch from 906276d to c3e242e Compare January 17, 2020 01:37

appilon force-pushed the context-crud branch from 786861c to 351da45 Compare January 17, 2020 01:38

bflad reviewed Jan 17, 2020

View reviewed changes

helper/schema/resource.go Show resolved Hide resolved

appilon force-pushed the version2 branch from c3e242e to 2e760c5 Compare January 21, 2020 22:58

appilon force-pushed the context-crud branch from 9fc775e to d53f197 Compare January 21, 2020 22:59

appilon marked this pull request as ready for review January 21, 2020 22:59

kmoe approved these changes Jan 27, 2020

View reviewed changes

paultyng assigned appilon Jan 27, 2020

appilon force-pushed the version2 branch from 2e760c5 to 49ff95b Compare January 29, 2020 20:04

appilon added 17 commits January 29, 2020 15:04

remove StopContext feature

b468450

force cancellation through ctx API merge a server instanced context's cancellation with the GRPC request scoped context's cancellation

privatize contextify

c84e09b

switch back to runtime check of CRUD funcs

a3105d2

make all CustomizeDiff funcs to context aware (breaking change)

fix code comments

6699cde

remove legacy terraform version as this SDK can never talk with Terraform < 0.12

Prune remaining legacy provider calls with panic

f1225b6

placeholders. Made configure context aware

InternalValidate context funcs

550d1e6

rearrange internal crud funcs happy path

use UnaryInterceptor to configure stop context

271770b

add deprecated comment to CRUD funcs

5fdef2f

add retroactive deprecated comment to MigrateState need to decide if the feature should be cut in v2 if this is the cause we should place the deprecated comment in the next v1 release.

Stop tests

8189aab

deprecate CRUD func types

e246c58

contextify ImportState

8ff7a06

Add context support to Importers and StateUpgraders

5186454

fix comment

b9f4b35

switch server stop signal to be channel based

a2c9ed6

update stop feature tests to have 5 second timeout this ensures any regressions are caught in tests otherwise the tests themselves deadlock when the implementation is broken. Passing tests execute near instantly.

restore resource/datasource error check

f18f470

add StopContext reset test

1cc6cb1

appilon force-pushed the context-crud branch from 54eedc1 to 1cc6cb1 Compare January 29, 2020 20:04

paultyng reviewed Jan 29, 2020

View reviewed changes

helper/customdiff/testing_test.go Show resolved Hide resolved

appilon added 3 commits January 30, 2020 14:00

add newline

db1bdbe

context support to ConfigureFunc

3a9a6bb

fix deprecated comments

6c4bf73

appilon merged commit 389dc87 into version2 Jan 31, 2020

appilon deleted the context-crud branch January 31, 2020 21:25

appilon added the breaking-change Implementation which breaks compatibility or other promises label Feb 6, 2020

ghost locked and limited conversation to collaborators Mar 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Context aware SDK with timeouts #276

Context aware SDK with timeouts #276

appilon commented Dec 11, 2019 •

edited

Loading

appilon commented Dec 11, 2019 •

edited

Loading

paultyng commented Dec 11, 2019

paultyng commented Dec 11, 2019

appilon commented Dec 11, 2019 •

edited

Loading

appilon commented Dec 11, 2019

paultyng commented Jan 9, 2020

appilon commented Jan 9, 2020

kmoe left a comment

ghost commented Mar 2, 2020

Context aware SDK with timeouts #276

Context aware SDK with timeouts #276

Conversation

appilon commented Dec 11, 2019 • edited Loading

appilon commented Dec 11, 2019 • edited Loading

paultyng commented Dec 11, 2019

paultyng commented Dec 11, 2019

appilon commented Dec 11, 2019 • edited Loading

appilon commented Dec 11, 2019

paultyng commented Jan 9, 2020

appilon commented Jan 9, 2020

kmoe left a comment

Choose a reason for hiding this comment

ghost commented Mar 2, 2020

appilon commented Dec 11, 2019 •

edited

Loading

appilon commented Dec 11, 2019 •

edited

Loading

appilon commented Dec 11, 2019 •

edited

Loading