feat(sfs): make resource pool wait timeouts configurable - #1740
Conversation
1c1872e to
09296ec
Compare
82d9ac7 to
6d2a216
Compare
CreateResourcePoolWaitHandler and its update/delete counterparts default to 10 minutes. The resource passed a context without a deadline, so that default was the only limit and no configuration could reach it. A pool that STACKIT needs longer than 10 minutes to provision could not be created at all. The SDK wait handler applies its own timeout only when the incoming context carries no deadline (core/wait.WaitWithContext). Setting a context deadline in each CRUD method therefore replaces the hardcoded value, which is what the new `timeouts` attribute does. Defaults stay at the wait handler value plus core.DefaultTimeoutMargin, so unconfigured resources keep their behavior. The configured timeouts are written to state together with the IDs before the create wait starts. Without that, a failed wait leaves an entry whose refresh and destroy fall back to the default timeouts - on exactly the recovery path those values are needed for. The error raised when the create wait handler gives up now says that Terraform marks the resource tainted and replaces it on the next run, names `untaint` and the import ID, and mentions `timeouts.create` only when this context's deadline is what ended the wait. The handler reports terminal error states and failing polls through the same error, which are not timeouts. TestWaitHandlerTimeoutIsBoundedByContext pins the SDK behavior the attribute depends on, so an SDK bump that enforces the handler timeout unconditionally fails the build instead of silently capping the configured value again.
6d2a216 to
6644872
Compare
marceljk
left a comment
There was a problem hiding this comment.
Thanks for your contribution! I left some minor comments
- shorten the create wait error to the timeout hint, and emit the same hint in update and delete so the three read consistently - drop the write of the timeouts attribute into the partial state - add the timeouts attribute to the resource pool data source as well - replace the create timeout test with the shorter form used for dns, on the existing MockServer - keep a blank line before the timeout blocks in Read and Update
…e review The update wait branch kept the summary "Error creating resource pool" while the appended hint named `timeouts.update`, so the diagnostic contradicted itself. Summary and detail now say "updating", matching what stackitcloud#1741 changes the same line to. The hint starts on its own line again; concatenating it directly onto the wrapped error ran the two sentences together. Two follow-ups in the spirit of the review rather than its letter: the data source read timeout gets the same blank line that was asked for in the resource, and the acceptance-test data source declares a timeouts block, as the dns testdata does for its data source.
|
Follow-up in 7f8825e, after re-reading the result of the previous commit:
I also removed a paragraph from the PR description that still described the state write you asked me to drop. |
| // timeoutHint names the configured timeout when this context's deadline is what ended a wait. The wait handler | ||
| // reports a timeout, a terminal error state and a failing poll through the same error, so on the other two the | ||
| // hint would point at the wrong cause. | ||
| func timeoutHint(ctx context.Context, operation string, timeout time.Duration) string { | ||
| if !errors.Is(ctx.Err(), context.DeadlineExceeded) { | ||
| return "" | ||
| } | ||
| return fmt.Sprintf("\nThe wait gave up after the configured `timeouts.%s` of %s; raise it if the operation regularly needs longer.", operation, timeout) | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you move this to our utils package, so it can be also reused by other resources and datasources in the future?
There was a problem hiding this comment.
Moved in 06224ce. It now lives in stackit/internal/utils/timeouts.go as utils.TimeoutHint, since nothing in it is SFS-specific. TestTimeoutHint next to it pins the exact text for an exceeded deadline and checks that a cancellation, a wait that failed before the deadline and a context without a deadline produce no hint.
The hint that names the configured timeout after a wait ran out is not specific to SFS, so it now lives in stackit/internal/utils as TimeoutHint where other resources and data sources can use it. The move adds a unit test that pins the exact text, including the leading newline, and checks that a cancellation or a wait that failed before the deadline produce no hint.
Description
relates to #1737
stackit_sfs_resource_poolcannot create a pool that STACKIT needs more than 10 minutes toprovision.
CreateResourcePoolWaitHandlerand its update/delete counterparts setSetTimeout(10 * time.Minute), and the resource passed a context without a deadline, so thatdefault was the only limit and no configuration could reach it.
This adds a
timeoutsattribute (create/read/update/delete) following the pattern ofdremio/instance.Why a context deadline and not
SetTimeoutcore/wait.WaitWithContextapplies the handler's own timeout only when the incoming contextcarries no deadline:
(core v0.26.0, wait.go)
Setting a context deadline per CRUD method therefore replaces the hardcoded value, and no
SetTimeoutcall is needed. Defaults stay at the wait handler value pluscore.DefaultTimeoutMargin, so unconfigured resources keep their current behaviour.TestSfsResourcePoolCreateTimeoutchecks that a configuredtimeouts.createbounds the create.The wait error
When a wait ends because this context's deadline expired, the error names the configured timeout and
suggests raising it. The wait handler reports terminal error states and failing polls through the
same error, so the hint is added only on the deadline path, and identically in create, update and
delete. The hint is
utils.TimeoutHintinstackit/internal/utils, so other resources can reuse it;TestTimeoutHintpins its text.On point 2 of the issue
The issue also asks to keep the resource in state when the wait times out. That is already
implemented:
utils.SetAndLogStateFieldswrites the IDs before the wait (CONTRIBUTING.md statesthis as project doctrine, and
TestSfsResourcePoolSavesIDsOnErrorcovers it), the frameworkinitialises the create response state to a null object so no unknowns leak, and Terraform keeps the
object and marks it tainted rather than discarding it. What follows is a replace, not a lost entry.
No provider change was needed for that, only the corrected wording of the error.
testdata/resource-pool-max.tfsetstimeouts, and noImportStateVerifyIgnoreis needed for it: terraform-plugin-testing deletestimeoutsandtimeouts.*fromboth sides of the comparison unconditionally, after the ignore loop
(testing_new_import_state.go:398-411, v1.16.0).
The DNS acceptance tests rely on the same behaviour.
Not changed
sfs/sharehas the same hardcoded 10 minutes in all three of its wait handlers. Out of scope forthis issue — happy to follow up if wanted.
Checklist
make fmtexamples/directory) — deliberately not: no example inexamples/has ever carried atimeoutsblock (git log -S timeouts -- examples/is empty), and feat(dns) add timeouts to dns resources and datasources #1345, which addedtimeoutsto the DNS resources, put the demonstrable configuration intestdata/resource-max.tfinstead. Hardcoding durations on a registry page would also pin numbers that go stale when the SDK waiter default moves.make generate-docs(will be checked by CI)testdata/resource-pool-max.tfnow setstimeouts, mirroringdns/testdata/resource-max.tf, soTestAccResourcePoolResourceMaxcovers the attribute across create, import-verify and updatemake test(will be checked by CI)make lint(will be checked by CI)