Copilot commented on code in PR #2869:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2869#discussion_r3985396423
##########
internal/adc/client/executor.go:
##########
@@ -150,40 +154,66 @@ func (e *HTTPADCExecutor) Validate(ctx context.Context,
config adctypes.Config,
return e.runHTTPValidate(ctx, config, resources, labels, resourceTypes)
}
-// runHTTPSync performs HTTP sync to ADC Server for each server address
+// runHTTPSync sends config in one /sync request and returns the parsed
failure, if any.
+// A sync is one request whatever config.ServerAddrs holds: apisix-standalone
joins every
+// entry with commas because ADC addresses them together as one logical
destination
+// (buildHTTPRequest splits them back apart into the request body), every
other backend
+// type takes the first entry only, since a GatewayProxy is expected to
resolve to one
+// address there even though nothing enforces it yet. Deciding how many
addresses a
+// GatewayProxy has belongs to the caller that built config.ServerAddrs.
+//
+// A GatewayProxy with no resolved address is a sync failure for
apisix-standalone (the
+// data plane it configures is unreachable, e.g. scaled to zero), and a no-op
for every
+// other backend type, which pushes per address and so has nothing to push.
+//
+// This package never decides whether to retry the failure; callers interpret
it and ask
+// again if they choose to.
func (e *HTTPADCExecutor) runHTTPSync(ctx context.Context, config
adctypes.Config, resources *adctypes.Resources, labels map[string]string,
resourceTypes []string) error {
- var execErrs = types.ADCExecutionError{
- Name: config.Name,
+ standalone := config.BackendType == BackendAPISIXStandalone
+ if len(config.ServerAddrs) == 0 {
+ if standalone {
+ return types.ADCExecutionServerAddrError{Err: "no data
plane address to sync apisix-standalone config to"}
+ }
+ return nil
}
- serverAddrs := func() []string {
- if config.BackendType == backendAPISIXStandalone {
- return []string{strings.Join(config.ServerAddrs, ",")}
+ target := syncTargetAddr(config)
+ e.log.V(1).Info("running http sync", "server", target)
+
+ ctx, cancel := context.WithTimeout(ctx, e.httpClient.Timeout)
+ defer cancel()
+
+ req, err := e.buildHTTPRequest(ctx, target, config, labels,
resourceTypes, resources, pathSync)
+ if err != nil {
+ return types.ADCExecutionServerAddrError{ServerAddr: target,
Err: fmt.Sprintf("failed to build HTTP request: %s", err)}
+ }
+
+ resp, err := e.httpClient.Do(req)
+ if err != nil {
+ return types.ADCExecutionServerAddrError{ServerAddr: target,
Err: fmt.Sprintf("failed to send HTTP request: %s", err)}
+ }
+ defer func() {
+ if closeErr := resp.Body.Close(); closeErr != nil {
+ e.log.Error(closeErr, "failed to close response body")
}
- return config.ServerAddrs
}()
- e.log.V(1).Info("running http sync", "serverAddrs", serverAddrs)
- for _, addr := range serverAddrs {
- if err := e.runHTTPSyncForSingleServer(ctx, addr, config,
resources, labels, resourceTypes); err != nil {
- e.log.Error(err, "failed to run http sync for server",
"server", addr)
- var execErr types.ADCExecutionServerAddrError
- if errors.As(err, &execErr) {
- execErrs.FailedErrors =
append(execErrs.FailedErrors, execErr)
- } else {
- execErrs.FailedErrors =
append(execErrs.FailedErrors, types.ADCExecutionServerAddrError{
- ServerAddr: addr,
- Err: err.Error(),
- })
- }
- }
- }
- if len(execErrs.FailedErrors) > 0 {
- return execErrs
+ if err := e.handleHTTPResponse(resp, target); err != nil {
+ e.log.Error(err, "failed to run http sync", "server", target)
+ return err
}
return nil
}
+// syncTargetAddr resolves config.ServerAddrs into what one /sync request
targets. Callers
+// must have already handled an empty ServerAddrs (see runHTTPSync).
+func syncTargetAddr(config adctypes.Config) string {
+ if config.BackendType == BackendAPISIXStandalone {
+ return strings.Join(config.ServerAddrs, ",")
+ }
+ return config.ServerAddrs[0]
Review Comment:
`ControlPlaneProvider.Endpoints` permits multiple entries
(`api/v1alpha1/gatewayproxy_types.go:129-132`), and the translator copies that
full list for every mode (`internal/adc/translator/gatewayproxy.go:94-97`).
Selecting only index 0 therefore silently stops syncing all later
non-standalone endpoints, whereas the previous loop sent one request to each.
Preserve the fan-out or reject multi-endpoint non-standalone configurations
before they reach this path.
##########
internal/adc/client/standalone_syncer.go:
##########
@@ -0,0 +1,120 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package client
+
+import (
+ "context"
+ "sync"
+
+ "github.com/go-logr/logr"
+
+ pkgmetrics "github.com/apache/apisix-ingress-controller/pkg/metrics"
+)
+
+// StandaloneSyncer drives apisix-standalone's diff-baseline recovery on top
of the
+// one-shot Client. It is only for apisix-standalone: no other backend type
keeps a
+// conf_version, so no other backend type needs any of this.
+//
+// APISIX standalone keeps a monotonic conf_version per resource type and
refuses a whole
+// push whose version is behind the data plane's. ADC diffs against a cached
baseline to
+// build that push, and the baseline can go stale two ways:
+//
+// - Across a leadership change. The ADC server is a sidecar that outlives
the manager
+// container, so what it holds for a cacheKey can be the snapshot this pod
left behind
+// in an earlier term, while the leader in between moved the data plane's
conf_version
+// past it. InvalidateBaselines on leader acquisition forces the first
push of every
+// cacheKey this term to re-derive its baseline from the data plane.
+// - Within a term, from a desync no leadership change explains, e.g.
another writer on
+// the same data plane. A conf_version the data plane refuses is the only
way that
+// shows itself; Sync answers it with one rebuild-and-retry.
+type StandaloneSyncer struct {
+ client *Client
+ log logr.Logger
+
+ mu sync.Mutex
+ rebuilt map[string]struct{}
+}
+
+func NewStandaloneSyncer(c *Client, log logr.Logger) *StandaloneSyncer {
+ return &StandaloneSyncer{
+ client: c,
+ log: log.WithName("standalone-syncer"),
+ rebuilt: make(map[string]struct{}),
+ }
+}
+
+// InvalidateBaselines forgets every rebuilt-this-term record, so the next
push of each
+// cacheKey re-derives its baseline. Call on leader acquisition.
+func (s *StandaloneSyncer) InvalidateBaselines() {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ clear(s.rebuilt)
+}
+
+func (s *StandaloneSyncer) isCurrent(cacheKey string) bool {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ _, ok := s.rebuilt[cacheKey]
+ return ok
+}
+
+func (s *StandaloneSyncer) markCurrent(cacheKey string) {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ s.rebuilt[cacheKey] = struct{}{}
+}
+
+// Sync pushes in, rebuilding ADC's baseline first if this term has not yet
pushed this
+// cacheKey, and once more if the data plane rejects the push over a stale
conf_version.
+//
+// It returns every error worth reporting, so 0, 1, or 2 of them: the final
failure, plus
+// the conf_version rejection that triggered a rebuild which then failed for a
different
+// reason (on its own that failure points nowhere near its cause, e.g. an ADC
server too
+// old to know bypassCache answers with a schema error). BypassCache is scoped
to the call
+// that recovers from a rejection and never written back into in.
+func (s *StandaloneSyncer) Sync(ctx context.Context, in SyncInput) []error {
+ in.Config.BypassCache = !s.isCurrent(in.Name)
+ err := s.client.Sync(ctx, in)
Review Comment:
Each `Client.Sync` call records sync duration/count and execution-error
metrics before this layer decides whether the failure is recoverable. A
stale-conf_version rejection that the retry fixes now records one failed sync
plus `sync_failed`, then another successful sync, in addition to
`conf_version_conflict`; previously it produced one successful logical-sync
sample plus the conflict counter. This changes alerting/rates despite the PR's
metrics-equivalence claim. Record metrics once around the logical standalone
sync, or use an uninstrumented one-shot primitive and aggregate the final
outcome here.
--
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]
