fix(objectstorage): retry enabling the project on 409 conflict - #1645
fix(objectstorage): retry enabling the project on 409 conflict#1645FabianHardt wants to merge 3 commits into
Conversation
bucket, credential and credentials group each enable object storage for the
project before creating their own object. When two of them are created in the
same apply, Terraform runs them in parallel and the API rejects the losing
call:
Error: Enabling object storage project before creation: failed to create
object storage project: 409 Conflict
([{project.create_conflict Two concurrent calls try to create the same
project}]), status code 409
The apply fails, although nothing is wrong - the competing call enables the
project a moment later. The comment in enableProject already assumed the call
to be idempotent ("Creation will also be successful if the project is already
enabled"), which holds for sequential calls but not for concurrent ones.
enableProject now retries on 409 and leaves every other error untouched, so an
apply no longer depends on the order in which Terraform happens to start the
resources. Users can work around it today with depends_on, but that requires
knowing about an implicit call that the resource documentation does not
mention.
The retry is deliberately narrow rather than utils.RetryRequest: that helper
also retries errors that are not API errors, which would slow down the
existing unit tests.
Signed-off-by: Fabian Hardt <fabian.hardt@opitz-consulting.com>
| var err error | ||
| for attempt := 0; attempt < enableProjectAttempts; attempt++ { | ||
| _, err = client.EnableService(ctx, projectId, region).Execute() | ||
| if err == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var oapiErr *oapierror.GenericOpenAPIError | ||
| if !errors.As(err, &oapiErr) || oapiErr.StatusCode != http.StatusConflict { | ||
| break | ||
| } | ||
|
|
||
| timer := time.NewTimer(enableProjectRetryDelay) | ||
| select { | ||
| case <-ctx.Done(): | ||
| timer.Stop() | ||
| return ctx.Err() | ||
| case <-timer.C: | ||
| } |
There was a problem hiding this comment.
Not a full review, but we recently added some retry utils that looks like it might be applicable here. See retry.go
Per review: utils.RetryRequest already covers this, and the loop was duplicated across all three resources. One behavioural difference worth naming: RetryRequest only filters by status code when the error can be type-asserted to *oapierror.GenericOpenAPIError. Anything else - a network failure, a transport error - is now retried as well, where the previous loop bailed out immediately. For an idempotent enable call that seems reasonable, but it is a change, not a refactor. It also shows up in the existing TestEnableProject: its mock returns a plain error, so the failing case now uses every attempt. Those tests shrink the retry delay so they stay fast.
|
Good call, thanks — swapped in One difference I want to flag rather than bury, because it's a change and not a refactor: It surfaced in the existing
|
2 participants
Problem
stackit_objectstorage_bucket,stackit_objectstorage_credentialandstackit_objectstorage_credentials_groupeach enable object storage for theproject before creating their own object. When two of them are created in the
same apply and nothing forces an order, Terraform runs them in parallel and the
API rejects the losing call:
The apply fails although nothing is actually wrong — the competing call enables
the project a moment later.
Minimal reproducer (both resources in one config, no reference between them):
The comment in
enableProjectalready assumes idempotency — "Creation willalso be successful if the project is already enabled" — which holds for
sequential calls but not for concurrent ones.
Change
enableProjectretries on409and leaves every other error untouched. Anapply no longer depends on the order in which Terraform happens to start the
resources.
depends_onworks around it today, but that requires knowing about an implicitAPI call the resource documentation does not mention — the error message points
at object storage projects, not at a missing dependency.
The retry is deliberately narrow rather than
utils.RetryRequest: that helperalso retries errors which are not
GenericOpenAPIError, which would slow theexisting unit tests down.
Tests
Added to
credentialsgroup/resource_test.go:TestEnableProjectRetriesOnConflict— succeeds immediately, one conflict thensuccess, and conflicts until the attempts are used up; asserts the number of
API calls in each case
TestEnableProjectDoesNotRetryOtherErrors— a403must fail on the firstattempt
The retry delay is a package variable so tests can shorten it.
Note
The three copies of
enableProjectare identical; I kept the duplication tokeep the diff reviewable. Happy to extract it into
objectstorage/utilsinstead if you prefer.