Repository navigation
chore: remove unused sweeper code - #1047
tng1-equinix wants to merge 2 commits into
Conversation
0d1711a to
4af959a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
f57d2fb to
6ece9a9
Compare
|
|
||
| const ( | ||
| // duplicated from equinix_sweeoer_test.go | ||
| testResourcePrefix = "tfacc" |
There was a problem hiding this comment.
Following Metal's removal and pending #622, sweep.go is really unused, with the Fabric parts moved to https://github.com/equinix/terraform-provider-equinix/blob/main/internal/fabric/sweep/sweep.go#L7-L18
What I am noticing in this file and the moved file, is a dangerous pattern of deleting resources based on suffix. The prefix check (tfacc*) has saved the day when a developer (any user/contributor performing provider testing against their own environment) intentionally or accidentally runs the sweepers against an environment with non-test resources.
Test resources are generally expected to match tfacc*. If you want to add a suffix as a secondary check, that is fine. I do wonder if that suffix check is passing for all of the Fabric test resources being created. I would imagine some test resources will not fall into one of these three categories ("_PFCR", "_PNFV", "_PPDS").
Assuming (and perhaps this is not the case today), that Fabric TF test resources are tfacc prefixed, I would prefer the standard prefix check over this custom suffix check. This standard prefix check can continue to be used and shared when NE is moved to internal (#622 (comment)), and when additional resources are added.
There was a problem hiding this comment.
While this is all dead-code today, the pattern and intention of this sweeper file is to be used with additional resources added to the provider. Only the Fabric definitions, redundant to internal/fabric/sweep and internal/fabric/config, should be removed from this file.
| @@ -1,57 +0,0 @@ | |||
| package sweep | |||
There was a problem hiding this comment.
Sweeper client config and standards for identifying test resources is really a thing where we should be consistent across products rather than defining separate, product-specific baselines.
| return false | ||
| } | ||
|
|
||
| func GetConfigForFabric() (*config.Config, error) { |
There was a problem hiding this comment.
I see nothing Fabric-specific in here. Client ID and client secret are the same things used for Network Edge, for example. I suspect this name is what it is because GetConfigForMetal truly used a different configuration for Metal's non-standard auth, and Fabric just happened to be the other product that was implementing sweepers.
I would prefer for this to become GetConfig() and to be used for any sweeper that is making Equinix API calls.
| ) | ||
|
|
||
| func IsSweepableTestResource(namePrefix string) bool { | ||
| return strings.HasPrefix(namePrefix, testResourcePrefix) |
There was a problem hiding this comment.
Echoing Marques's comment above, using a consistent, unlikely-to-be-accidental prefix for test resource names is more reliable and less risky than using suffixes that might reasonably be used for non-test resources.
From what I recall, part of the reason for introducing suffixes was to prevent cross-talk between sweeper steps for the different acceptance test jobs. It would be preferable to have a single sweeper job that runs after the parallel test jobs complete, and sweeps all Terraform test resources by prefix.
There was a problem hiding this comment.
To avoid a sweeper knocking out the resources of another test in progress, https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-workflow-concurrency may be of interest.
There was a problem hiding this comment.
My understanding is that concurrency would be functionally equivalent to running the test suites in series in a single workflow. If the goal is to run tests in parallel but avoid cross-talk between sweepers, that would likely involve a matrix job to run the test suites and a sweeper job that depends on (needs) the matrix job.
Follow-up to the sweeper failures discussed in #1088 (comment). Supersedes #1050. ## Problem From the PFCR sweep on #1088 ([job](https://github.com/equinix/terraform-provider-equinix/actions/runs/37343717922/job/111877377285)): ``` 17:07:16 DELETE .../connections/{fb95b459,554185f3,de30e83c} -> EQ-3142509 Connection already deleted 17:07:17 Sweeper (equinix_fabric_cloud_router) has dependency (equinix_fabric_connection) … already ran 17:07:18 DELETE .../routers/... -> EQ-3040050 Fabric Cloud Routers cannot be deleted when there are active Connections ``` The `cloud_router -> connection` dependency was already declared and ran in the right order. The problem is in the connection sweeper: 1. **It didn't wait.** Connection delete is async, and the router/network sweepers ran about 1s later while connections were still `DEPROVISIONING`. 2. **Its filter was too narrow.** Only `equinixStatus = PROVISIONED` was swept. Connections stuck mid-provisioning still block router/network deletes but were never selected. (#1050 added `PENDING_INTERFACE_CONFIGURATION`; this covers it and the other non-terminal states.) 3. **It treated "already deleted" as an error.** `EQ-3142509` is now treated as success, the same way `resourceFabricConnectionDelete` already does. ## Changes - Search `/state IN [ACTIVE, DEPROVISIONING, DRAFT, FAILED, PENDING, PROVISIONING]` (name suffix filter unchanged). The search rejects `PROVISIONED` and `REPROVISIONING` even though `GET` can return them, so those two only appear in the deprovision waiter's pending list. - Don't re-delete `DEPROVISIONING` connections; just wait on them. - Sweep connections **concurrently** under one 15m deadline (10s poll), waiting for each to reach `DEPROVISIONED` (or 404). An earlier version of this PR waited on them one after another. On PFCR, 14 stuck connections each used the full 15m, the sweep hit `go test`'s 180m timeout, and it was killed before the router and network sweepers ran. - Retry deletes rejected with `EQ-3142510 Connection is in transient state` until the deadline. - Sweep errors name the connection and UUID, and timeouts report the last observed state. ### Shared connection waiter The five `StateChangeConf` waiters in `connection/resource. 8BD0 go` differed only in which status field they read, their pending/target lists, and whether they returned the connection. Rather than add a sixth for the sweeper, they now all go through: ```go waitForConnection(ctx, client, uuid, pending, target, timeout, opts...) ``` with `withConnectionStatus` (read `operation.providerStatus` / `change.status` instead of `state`), `withNotFoundAs` (treat 404 as a given status), and `withPollInterval`. The existing waiters are thin wrappers with unchanged signatures, so `CheckConnectionDelete` in the acceptance tests keeps working. Each wait now builds the client once instead of on every poll. The `EQ-3142509` check is shared between `resourceFabricConnectionDelete` and the sweeper as `isConnectionAlreadyDeleted`. ## Not in scope - Connections that don't match the test-name check but are attached to a test router are still not swept. Sweeping by router UUID would delete resources without test names, which goes against the prefix-safety direction from #1047. - Manually created `_PFCR`-suffixed fixtures (e.g. `ClaudeDemo_PFCR`) will keep failing until they're renamed or cleaned up out-of-band. - The `equinix_fabric_service_profile` sweeper failure (`data failed to match schemas in oneOf(ServiceProfileUpdateOperation)`) is an SDK spec issue: `AddOperation`/`ReplaceOperation.value` is typed `object`, so a scalar or array value fails the decode. It needs an `anyType` patch in equinix-sdk-go like the existing `20250409_anyType_for_PortChangeOperation.patch`. ## Testing New `wait_test.go` unit tests (`net/http/httptest`) cover `waitForConnection` (target reached, unexpected state, empty pending, timeout, 404 with and without `withNotFoundAs`, 5xx, custom status) and `isConnectionAlreadyDeleted`, all at 100%. `go build ./...`, `go vet`, and `golangci-lint` pass. I haven't run the sweeper against the API. The first CI sweep showed `/state` with `IN` passes validation as long as the values are in the accepted set. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Uh oh!
There was an error while loading. Please reload this page.