Skip to content

feat(sfs): make resource pool wait timeouts configurable - #1740

Open
devpie wants to merge 1 commit into
stackitcloud:mainfrom
devpie:fix/sfs-resource-pool-timeouts
Open

feat(sfs): make resource pool wait timeouts configurable#1740
devpie wants to merge 1 commit into
stackitcloud:mainfrom
devpie:fix/sfs-resource-pool-timeouts

Conversation

@devpie

@devpie devpie commented Sep 4, 2026

Copy link
Copy Markdown

Description

relates to #1737

stackit_sfs_resource_pool cannot create a pool that STACKIT needs more than 10 minutes to
provision. CreateResourcePoolWaitHandler and its update/delete counterparts set
SetTimeout(10 * time.Minute), and the resource passed a context without a deadline, so that
default was the only limit and no configuration could reach it.

This adds a timeouts attribute (create/read/update/delete) following the pattern of
dremio/instance.

Why a context deadline and not SetTimeout

core/wait.WaitWithContext applies the handler's own timeout only when the incoming context
carries no deadline
:

if _, ok := ctx.Deadline(); !ok {
	ctx, cancel = context.WithTimeout(ctx, h.timeout)
	defer cancel()
}

(core v0.26.0, wait.go)

Setting a context deadline per CRUD method therefore replaces the hardcoded value, and no
SetTimeout call is needed. TestWaitHandlerTimeoutIsBoundedByContext pins that behaviour, so an
SDK bump that enforces the handler timeout unconditionally fails the build instead of silently
capping the configured value again. Defaults stay at the wait handler value plus
core.DefaultTimeoutMargin, so unconfigured resources keep their current behaviour.

Two things beyond the plain attribute

The configured timeouts are written to state together with the IDs, before the create wait
starts.
SetAndLogStateFields writes three attributes and the framework fills the rest with
null, so timeouts would be null in the entry a failed wait leaves behind. Refresh and destroy of
that entry would then fall back to the defaults — on exactly the recovery path those values are
needed for. TestSfsResourcePoolKeepsConfiguredTimeoutsOnError covers it.

The wait error is more specific. It now says that Terraform marks the resource tainted and
replaces it on the next run, points at terraform untaint and the import ID, and mentions
timeouts.create only when this context's deadline is what ended the wait. The wait handler
reports terminal error states and failing polls through the same error; telling the user to raise
a timeout in those cases would be wrong — the repo's own TestSfsResourcePoolSavesIDsOnError
drives the failing-poll case.

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.SetAndLogStateFields writes the IDs before the wait (CONTRIBUTING.md states
this as project doctrine, and TestSfsResourcePoolSavesIDsOnError covers it), the framework
initialises 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.

No ImportStateVerifyIgnore is needed for that: terraform-plugin-testing deletes timeouts and timeouts.* from
both 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/share has the same hardcoded 10 minutes in all three of its wait handlers. Out of scope for
this issue — happy to follow up if wanted.

Checklist

  • Issue was linked above
  • Code format was applied: make fmt
  • Examples were added / adjusted (see examples/ directory) — deliberately not: no example in examples/ has ever carried a timeouts block (git log -S timeouts -- examples/ is empty), and feat(dns) add timeouts to dns resources and datasources #1345, which added timeouts to the DNS resources, put the demonstrable configuration in testdata/resource-max.tf instead. Hardcoding durations on a registry page would also pin numbers that go stale when the SDK waiter default moves.
  • Docs are up-to-date: make generate-docs (will be checked by CI)
  • Unit tests got implemented or updated
  • Acceptance tests got implemented or updated — testdata/resource-pool-max.tf now sets timeouts, mirroring dns/testdata/resource-max.tf, so TestAccResourcePoolResourceMax covers the attribute across create, import-verify and update
  • Unit tests are passing: make test (will be checked by CI)
  • No linter issues: make lint (will be checked by CI)

🤖 Generated with Claude Code

@devpie
devpie requested a review from a team as a code owner September 4, 2026 12:39
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.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant