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` backend 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`. A failed event does
not abort the sync. It is returned in `failed[]` with its full `event`, and the
response is still a 202.
```rust
// adc: rust/crates/adc-cli/src/server/sync.rs
let sync_opts = BackendSyncOptions {
concurrent: Some(opts.request_concurrent),
exit_on_failure: Some(false),
};
```
```ts
// adc: apps/cli/src/server/sync.ts (the Node server, same behavior)
backend.sync(diff, { exitOnFailure: false, concurrent: 4 })
```
The apisix backend only retries 5xx and transport errors briefly, then gives
up and reports the event as failed:
```rust
// adc: rust/crates/adc-backend-core/src/retry.rs
fn default() -> Self {
Self { retries: 3, delay: Duration::from_millis(100) }
}
```
The Admin API returns `503` for any etcd write error:
```lua
-- apisix: apisix/admin/resource.lua
local res, err = core.etcd.set(key, conf, ttl)
if not res then
core.log.error("failed to put ", self.kind, "[", key, "] to etcd: ", err)
return 503, {error_msg = err}
end
```
An etcd leader election usually lasts longer than ~300ms. If one overlaps a
sync that has already passed dump, the retries run out and the event ends up in
`failed[]`.
The failed entry carries no HTTP status, so AIC cannot even separate these
by status code today:
```rust
// adc: rust/crates/adc-cli/src/server/sync.rs
struct FailedEntry {
server: Option<String>,
event: Option<Value>,
failed_at: String,
reason: String,
}
```
The `apisix-standalone` backend is not affected: the Rust server fills its
`failed[]` only from `/validate`.
**Lifecycle once an entry is recorded**
1. Every later push leaves the service out, including periodic `SyncPeriod`
rounds. The ADC diff then DELETEs the previously serving service and its routes:
```go
// internal/provider/apisix/provider.go, sync()
revisions[config.Name] = revision
resources = exclude(resources, d.skipped.Excluded(config.Name))
```
2. Successful rounds never clear the entry. This PR's own test asserts it:
```go
// internal/provider/apisix/status_test.go
ok := map[string]types.ADCExecutionErrors{"GatewayProxy/ns/gp": {}}
require.False(t, d.updateStatusFromSyncResults(context.Background(), ok,
revisions), "nothing new to exclude")
require.Len(t, d.resourceFailures[route], 1, "still dropped: ADC no
longer mentioning it says nothing about a fix")
```
3. The entry is cleared only when:
- the owner's translated content actually changes,
- the owner is deleted or evicted,
- the GatewayProxy config is removed, or
- the controller restarts or leadership changes.
The content check is the only path a healthy resource could take:
```go
// internal/provider/apisix/provider.go, applyResourceState()
before := d.store.Revision()
defer func() {
if d.store.OwnerChangedSince(rk, before) {
d.skipped.ClearOwner(rk)
}
}()
```
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]
