AlinsRan commented on code in PR #2890:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2890#discussion_r4119135842
##########
internal/provider/apisix/status.go:
##########
@@ -137,13 +176,50 @@ func (d *apisixProvider) classifySyncResult(
return gatewayProxyMsgs, failedEndpoints
}
+// dropUnit maps a rejected resource to what has to be dropped for the rest to
apply. A
+// service is dropped by itself. A route, stream route or named upstream is
dropped along
+// with the service it lives in: the service is what one Kubernetes rule
translates to,
+// and a named upstream is referenced by the service's traffic-split by id, so
dropping
+// it alone would leave that reference dangling. Nested resources are found
through their
+// parent, which also tells them apart when several services embed upstreams
of the same
+// id.
+func (d *apisixProvider) dropUnit(configName string, ev adctypes.StatusEvent)
(wireKey, exclusion, bool) {
Review Comment:
`dropUnit` treats every `failed[]` entry as a content rejection, but on the
`apisix` and `api7ee` backends ADC also reports transient failures there. Right
now a single 5xx or transport error can permanently exclude a valid service and
delete it from the data plane.
**Where transient failures come from (ADC master `63a0937`)**
- The ADC server runs sync with `exit_on_failure: false` (Rust
`adc-cli/src/server/sync.rs:95`, Node `apps/cli/src/server/sync.ts:115`). So a
failed event does not abort the sync. It is returned in `failed[]` with its
full `event`, and the response is still a 202.
- **api7ee:** a failed request is not retried at all. See Rust
`adc-backend-api7/src/operator.rs:137`, whose file header says "No retry
wrapping here … a failed request is never retried", and Node
`libs/backend-api7/src/operator.ts:79`. Any one 5xx, 502/504 from an LB, or
reset connection to the control plane during a sync becomes a `failed[]` entry.
- **apisix:** 5xx and transport errors are retried 3 times, 100ms apart
(`adc-backend-core/src/retry.rs:23`). The Admin API returns `503` whenever an
etcd write fails (`apisix/admin/resource.lua:253`, `:332`). An etcd leader
election usually takes longer than ~300ms, so if one overlaps a sync that has
already passed dump, the retries run out.
- **apisix-standalone** is not affected: the Rust server fills `failed[]`
only from `/validate`.
- The Rust `FailedEntry` has no HTTP status (`sync.rs:139`), so AIC cannot
even separate these by status code today.
**Lifecycle once an entry is recorded**
1. `MarkFailing` records the service. Every later push leaves it out
(`provider.go:593`), including periodic `SyncPeriod` rounds, so the ADC diff
DELETEs the previously serving service and its routes.
2. Successful rounds never clear the entry (by design, see
`status_test.go:545`).
3. The entry is cleared only when:
- the owner's translated content changes,
- the owner is deleted or evicted,
- the GatewayProxy config is removed, or
- the controller restarts or leadership changes.
For a transient failure the CR is already correct, so none of these is
expected to happen. The route stays down while the GatewayProxy reports
`DataPlaneAvailable=True`.
When etcd becomes unavailable partway through a large sync, every event
after that point fails. The corresponding services are all excluded, and once
etcd recovers they are all deleted from the data plane with no self-healing.
Before this PR, the whole sync failed and was retried with backoff until it
converged.
**Repro (unit level)**
```go
func TestTransient5xxExcludesStickily(t *testing.T) {
d, route := newDroppingProvider(t)
rev := map[string]uint64{"GatewayProxy/ns/gp": d.store.Revision()}
results := map[string]types.ADCExecutionErrors{"GatewayProxy/ns/gp":
failedEvent(
adctypes.StatusEvent{ResourceType: adctypes.TypeRoute,
ResourceID: "r1", ParentID: "svc1", Type: "update"},
"API error (status 503): etcd request timed out")}
require.True(t, d.updateStatusFromSyncResults(context.Background(),
results, rev))
for i := 0; i < 10; i++ { // data plane recovered, every round succeeds
d.updateStatusFromSyncResults(context.Background(),
map[string]types.ADCExecutionErrors{"GatewayProxy/ns/gp": {}}, rev)
}
require.Contains(t, d.skipped.Excluded("GatewayProxy/ns/gp"),
wireKey{adctypes.TypeService, "svc1"}) // passes
require.NotEmpty(t, d.resourceFailures[route])
// passes
}
```
**Possible directions**
- Have ADC mark each `failed[]` entry as a content rejection (non-retriable
4xx, version gate) or a retriable failure (5xx, transport), and exclude only
content rejections. This needs an ADC change first.
- Or give exclusions a bounded lifetime on the AIC side, for example
re-including them with backoff, so a false positive heals without a content
change or a restart.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
