bzp2010 commented on code in PR #2890:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2890#discussion_r4080941140
##########
internal/provider/apisix/status.go:
##########
@@ -137,13 +173,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) {
+ serviceID := ev.ResourceID
+ switch ev.ResourceType {
+ case adctypes.TypeService:
+ case adctypes.TypeRoute, adctypes.TypeStreamRoute,
adctypes.TypeUpstream:
+ serviceID = ev.ParentID
+ default:
+ return wireKey{}, exclusion{}, false
+ }
+ service, ok := d.store.Lookup(configName, adctypes.TypeService,
serviceID)
+ if !ok {
+ return wireKey{}, exclusion{}, false
+ }
+ owner, name := service.Owner, service.Name
+ // A route's own owner can differ from its service's: a traffic-split
service can
+ // combine rules several ApisixRoutes each contributed. Attribute to
the route
+ // itself when the store can tell them apart, so fixing the actual bad
route clears
+ // the exclusion instead of leaving it stuck on whichever owner the
shared service
+ // happens to carry.
+ if ev.ResourceType == adctypes.TypeRoute {
+ if route, ok := d.store.Lookup(configName, adctypes.TypeRoute,
ev.ResourceID); ok && route.Owner != (types.NamespacedNameKind{}) {
+ owner = route.Owner
+ }
+ }
+ return wireKey{adctypes.TypeService, service.ID}, exclusion{owner:
owner, name: name}, true
+}
+
// applyResourceFailures writes this round's newly (or still) failing
resources, and
// clears the recorded error from any resource that was failing last round but
isn't in
// newFailures now. See updateStatusFromSyncResults for why resources use this
delta
// instead of GatewayProxy's full recompute.
-func (d *apisixProvider) applyResourceFailures(newFailures
map[types.NamespacedNameKind][]string) {
+func (d *apisixProvider) applyResourceFailures(ctx context.Context,
newFailures map[types.NamespacedNameKind][]string) {
for resourceKey, msgs := range newFailures {
d.updateStatus(resourceKey, failureCondition(resourceKey,
strings.Join(msgs, "; ")))
+ if resourceKey.Kind == types.KindIngress {
+ d.recordIngressFailureEvent(ctx, resourceKey, msgs)
+ }
}
Review Comment:
This is intentional, not an oversight: client-go's EventRecorder correlates
repeats of the same reason and message against the same object into one Event,
PATCHing its count and lastTimestamp instead of creating a new object each
time. Firing every round is what keeps a long-lived failure's lastTimestamp
refreshed so it doesn't age out of `kubectl get events` once the default TTL
passes; deduping on our own end would undo that. The Get is a single object
read per failing Ingress per round, not a list, so its cost scales with how
many Ingresses are actually failing rather than with sync frequency alone.
##########
internal/provider/apisix/status.go:
##########
@@ -153,6 +226,21 @@ func (d *apisixProvider) applyResourceFailures(newFailures
map[types.NamespacedN
d.resourceFailures = newFailures
}
+// recordIngressFailureEvent fires a Warning event for an Ingress whose status
has no
+// conditions to carry a failure. It is fired every round the Ingress stays
failing, since
+// events expire.
+func (d *apisixProvider) recordIngressFailureEvent(ctx context.Context, nnk
types.NamespacedNameKind, msgs []string) {
Review Comment:
Same as above, this is the same call site's correlation behavior; see the
reply above.
##########
internal/provider/apisix/status.go:
##########
@@ -153,6 +226,21 @@ func (d *apisixProvider) applyResourceFailures(newFailures
map[types.NamespacedN
d.resourceFailures = newFailures
}
+// recordIngressFailureEvent fires a Warning event for an Ingress whose status
has no
+// conditions to carry a failure. It is fired every round the Ingress stays
failing, since
+// events expire.
+func (d *apisixProvider) recordIngressFailureEvent(ctx context.Context, nnk
types.NamespacedNameKind, msgs []string) {
+ if d.EventRecorder == nil || d.K8sClient == nil {
+ return
+ }
+ ingress := &networkingv1.Ingress{}
+ if err := d.K8sClient.Get(ctx, nnk.NamespacedName(), ingress); err !=
nil {
+ d.log.Error(err, "failed to get Ingress to record a failure
event", "name", nnk.Name, "namespace", nnk.Namespace)
+ return
+ }
+ d.EventRecorder.Event(ingress, corev1.EventTypeWarning,
string(apiv2.ConditionReasonSyncFailed), strings.Join(msgs, "; "))
+}
Review Comment:
Same as above, this is the same call site's correlation behavior; see the
reply above.
##########
internal/provider/apisix/provider.go:
##########
@@ -287,6 +292,11 @@ func (d *apisixProvider) Delete(ctx context.Context, obj
client.Object) error {
// applyResourceState upserts a resource's config associations and its
contribution to each
// target config's cached resource snapshot, the AIC-side bookkeeping the adc
client
// package no longer holds itself.
+// applyResourceState upserts a resource's config associations and its
contribution to each
+// target config's cached resource snapshot. Whatever the skip table excluded
for a
+// resource whose content this changed gets another try; rewriting identical
content, which
+// reconciles triggered by unrelated events do all the time, must not retry a
known-bad
+// resource.
Review Comment:
Fixed in 7a278941: collapsed into a single doc comment.
--
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]
