AlinsRan commented on code in PR #2856:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2856#discussion_r3947160805
##########
internal/provider/apisix/status.go:
##########
@@ -42,32 +55,101 @@ import (
func (d *apisixProvider) handleStatusUpdate(statusUpdateMap
map[types.NamespacedNameKind][]string) {
// Mark all resources in the current failure set as failed.
for nnk, msgs := range statusUpdateMap {
- d.updateStatus(nnk, cutils.NewConditionTypeAccepted(
- apiv2.ConditionReasonSyncFailed,
- false,
- 0,
- strings.Join(msgs, "; "),
- ))
+ d.updateStatus(nnk, failureCondition(nnk, strings.Join(msgs, ";
")))
}
// Mark resources that exist only in the previous failure history as
successful.
for nnk := range d.statusUpdateMap {
if _, ok := statusUpdateMap[nnk]; !ok {
- d.updateStatus(nnk, cutils.NewConditionTypeAccepted(
- apiv2.ConditionReasonAccepted,
- true,
- 0,
- "",
- ))
+ d.updateStatus(nnk, successCondition(nnk))
Review Comment:
Recovery is inferred from "the key left the failure set", not from "this
sync succeeded".
ADC produces the former without the latter:
| ADC `/sync` response | carries `endpoint_status`? |
| --- | --- |
| 202 success | yes (apisix-standalone) |
| 400 schema/lint, 500 catch-all | **no** |
Checked at `api7/adc` main (`9b0e093`) — same in both the Node and Rust
servers.
So when ADC itself fails, `unavailableEndpointsMessage` returns `""`
(status.go:368), the GatewayProxy never enters `statusUpdateMap`, and this
branch flips it to `True` plus a "every data plane instance took the last sync"
event — while the sync is still failing. The harder ADC fails, the greener the
GatewayProxy gets.
Recovery should be driven by an actually-successful sync, with "ADC Server
itself failed" leaving the condition untouched.
(Possibly related to the failing e2e you linked: on the Node ADC,
`findLatest()` has no per-server error handling, so one unreachable instance
throws out of `dump()` before `sync()` runs — 500, no `endpoint_status`.)
##########
api/v1alpha1/gatewayproxy_types.go:
##########
@@ -178,6 +179,19 @@ type GatewayProxy struct {
// GatewayProxySpec defines configuration of gateway proxy instances,
// including networking settings, global plugins, and plugin metadata.
Spec GatewayProxySpec `json:"spec,omitempty"`
+
+ // Status defines the current state of Gateway Proxy.
+ //
+ // +kubebuilder:default={conditions: {{type: "DataPlaneAvailable",
status: "Unknown", reason:"Pending", message:"Waiting for controller",
lastTransitionTime: "1970-01-01T00:00:00Z"}}}
Review Comment:
This default is never overwritten on the happy path — an always-healthy
GatewayProxy reports `Waiting for controller` forever. That contradicts the
"good" example in the PR description, where `status: "True"` is shown as the
healthy steady state.
Cause: `handleStatusUpdate` writes the success condition only for keys that
were in the *previous* failure set, and nothing else ever writes
`DataPlaneAvailable`. Two effects:
- healthy from the start never turns `True`
- a stale `False` survives a restart — `d.statusUpdateMap` is in-memory, so
the success branch never fires after a restart
Both are apisix-standalone, so scoping the feature to standalone doesn't
cover them. Writing the condition for every known config at the end of each
sync — success included — fixes both.
##########
internal/provider/apisix/status.go:
##########
@@ -42,32 +55,101 @@ import (
func (d *apisixProvider) handleStatusUpdate(statusUpdateMap
map[types.NamespacedNameKind][]string) {
// Mark all resources in the current failure set as failed.
for nnk, msgs := range statusUpdateMap {
- d.updateStatus(nnk, cutils.NewConditionTypeAccepted(
- apiv2.ConditionReasonSyncFailed,
- false,
- 0,
- strings.Join(msgs, "; "),
- ))
+ d.updateStatus(nnk, failureCondition(nnk, strings.Join(msgs, ";
")))
}
// Mark resources that exist only in the previous failure history as
successful.
for nnk := range d.statusUpdateMap {
if _, ok := statusUpdateMap[nnk]; !ok {
- d.updateStatus(nnk, cutils.NewConditionTypeAccepted(
- apiv2.ConditionReasonAccepted,
- true,
- 0,
- "",
- ))
+ d.updateStatus(nnk, successCondition(nnk))
+ if nnk.Kind == types.KindGatewayProxy {
+ d.recordGatewayProxyRecoveredEvent(nnk)
+ }
}
}
// Update the failure history with the current failure set.
d.statusUpdateMap = statusUpdateMap
}
+// failureCondition and successCondition pick which condition a
NamespacedNameKind
+// gets: GatewayProxyConditionDataPlaneAvailable for a GatewayProxy, the
existing
+// Accepted/SyncFailed condition for everything else.
+func failureCondition(nnk types.NamespacedNameKind, msg string)
metav1.Condition {
+ if nnk.Kind == types.KindGatewayProxy {
+ return newGatewayProxyDataPlaneAvailableCondition(false,
GatewayProxyReasonDataPlaneInstanceUnavailable, msg)
+ }
+ return cutils.NewConditionTypeAccepted(apiv2.ConditionReasonSyncFailed,
false, 0, msg)
+}
+
+func successCondition(nnk types.NamespacedNameKind) metav1.Condition {
+ if nnk.Kind == types.KindGatewayProxy {
+ return newGatewayProxyDataPlaneAvailableCondition(true,
GatewayProxyReasonDataPlaneAvailable, "")
+ }
+ return cutils.NewConditionTypeAccepted(apiv2.ConditionReasonAccepted,
true, 0, "")
+}
+
+func newGatewayProxyDataPlaneAvailableCondition(available bool, reason, msg
string) metav1.Condition {
+ conditionStatus := metav1.ConditionFalse
+ if available {
+ conditionStatus = metav1.ConditionTrue
+ }
+ return metav1.Condition{
+ Type: GatewayProxyConditionDataPlaneAvailable,
+ Status: conditionStatus,
+ LastTransitionTime: metav1.Now(),
+ Reason: reason,
+ Message: cutils.TruncateConditionMessage(msg),
+ }
+}
+
+// recordGatewayProxyRecoveredEvent fires once a GatewayProxy leaves the
failure
+// history entirely, i.e. every instance took the last sync.
+func (d *apisixProvider) recordGatewayProxyRecoveredEvent(nnk
types.NamespacedNameKind) {
+ if d.EventRecorder == nil {
+ return
+ }
+ gatewayProxy := &apiv1alpha1.GatewayProxy{
+ ObjectMeta: metav1.ObjectMeta{Name: nnk.Name, Namespace:
nnk.Namespace},
+ }
+ d.EventRecorder.Event(gatewayProxy, corev1.EventTypeNormal,
GatewayProxyReasonDataPlaneAvailable,
+ "every data plane instance took the last sync")
+}
+
+// recordFailedEndpointEvents fires one Warning event per failed
EndpointStatus entry,
+// so each instance's own failure history (when it started, how often) is
visible on
+// its own, not folded into everyone else's.
+func (d *apisixProvider) recordFailedEndpointEvents(nnk
types.NamespacedNameKind, endpoints []adctypes.EndpointStatus) {
+ if d.EventRecorder == nil {
+ return
+ }
+ gatewayProxy := &apiv1alpha1.GatewayProxy{
Review Comment:
This stub has no UID, so the Event's `involvedObject.uid` is empty and
`kubectl describe gatewayproxy` won't show it — which is what
`docs/en/latest/reference/troubleshoot.md` tells users to run.
`client-go` adds `involvedObject.uid=<uid>` to the field selector whenever
the described object has a UID, which a real GatewayProxy always does. Verified
on a live cluster: same object and event, without the uid `describe` prints
`Events: <none>`, with it the event shows. The e2e passes because it selects on
`kind` + `name` only.
Either record against the real object from the informer cache (needs
`mgr.GetClient()` threaded through from `run.go`), or change the doc to
`kubectl get events --field-selector ...`.
--
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]
