bzp2010 commented on code in PR #2890:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2890#discussion_r4084063472
##########
internal/adc/cache/store.go:
##########
@@ -460,3 +566,29 @@ func (s *Store) OwnedEntities(name string, owner
types.NamespacedNameKind) []Ent
}
return entities
}
+
+// Revision returns the store's current revision.
+func (s *Store) Revision() uint64 {
+ s.Lock()
+ defer s.Unlock()
+ return s.revision
+}
+
+// ChangedSince reports whether owner's content in the cacheKey name, or the
cacheKey as a
+// whole, changed after revision: a sync result built at revision then no
longer describes
+// what the store holds for owner.
+func (s *Store) ChangedSince(name string, owner types.NamespacedNameKind,
revision uint64) bool {
+ s.Lock()
+ defer s.Unlock()
+ return s.lastChange[name][owner] > revision || s.resetAt[name] >
revision
+}
+
+// OwnerChangedSince reports whether owner's content changed after revision in
any
+// cacheKey.
+func (s *Store) OwnerChangedSince(owner types.NamespacedNameKind, revision
uint64) bool {
+ s.Lock()
+ defer s.Unlock()
+ return slices.ContainsFunc(slices.Collect(maps.Values(s.lastChange)),
func(byOwner map[types.NamespacedNameKind]uint64) bool {
Review Comment:
This isn't actually a bug: we're using the standard library's `maps` package
(Go 1.26, go.mod), where `maps.Values` returns `iter.Seq[V]`, an iterator, not
a slice. `slices.Collect` is required to turn that into a []V before
slices.ContainsFunc can use it. This builds and is covered by tests; no change
needed.
##########
internal/adc/cache/store.go:
##########
@@ -86,6 +100,61 @@ func gatewayProxyOf(name string) (types.NamespacedNameKind,
bool) {
return gatewayProxy, true
}
+func (s *Store) recordChange(name string, owner types.NamespacedNameKind) {
+ s.revision++
+ if s.lastChange[name] == nil {
+ s.lastChange[name] = make(map[types.NamespacedNameKind]uint64)
+ }
+ s.lastChange[name][owner] = s.revision
+}
+
+// contentOf renders items, ordered by id, the way they reach ADC, so two
writes can be
+// compared by what they would push.
+func contentOf[T any](log logr.Logger, items []T, id func(T) string) string {
+ if len(items) == 0 {
+ return ""
+ }
+ sorted := slices.Clone(items)
+ slices.SortFunc(sorted, func(a, b T) int { return
strings.Compare(id(a), id(b)) })
+ return canonicalJSON(log, sorted)
+}
+
+// canonicalJSON renders v as JSON with every object's keys sorted. A stored
object's
+// plugin configs have been through a JSON round trip while a freshly
translated one may
+// still hold typed structs, whose fields marshal in declaration order rather
than sorted.
+//
+// A marshal error here is logged rather than propagated: v is always a type
this package
+// itself built from already-parsed JSON, so this is defensive rather than
expected, and
+// callers use the result only for change detection, not for anything that
reaches ADC.
+func canonicalJSON(log logr.Logger, v any) string {
+ b, err := json.Marshal(v)
+ if err != nil {
+ log.Error(err, "failed to marshal value for change detection")
+ return ""
+ }
+ var generic any
+ if err := json.Unmarshal(b, &generic); err != nil {
+ return string(b)
+ }
+ if sorted, err := json.Marshal(generic); err != nil {
+ log.Error(err, "failed to re-marshal value for change
detection")
+ } else {
+ b = sorted
+ }
+ return string(b)
+}
Review Comment:
Agreed this has real cost for large resource sets, but fixing it means
changing what change detection compares against (a cached per-owner fingerprint
computed once at write time, rather than a fresh marshal/unmarshal/marshal on
every comparison), which is a bigger design change than fits this PR. Tracking
this alongside the lastChange growth follow-up rather than fixing it here.
##########
internal/provider/apisix/status.go:
##########
@@ -58,23 +61,32 @@ const (
// get a True the first time, and what keeps a restart from leaving a stale
False stuck
// forever: the write only ever depends on this round's actual outcome.
//
-// Resource status can't afford the same full recompute: a config's resource
set can be
-// large, and rewriting every one of them every round even when nothing
changed would be
-// wasteful. So resources keep a small persisted delta in d.resourceFailures
instead:
-// newly (or still) failing resources are written SyncFailed, and any resource
that was
-// failing last round but isn't failing this one gets its error explicitly
cleared with
-// an Accepted write.
-func (d *apisixProvider) updateStatusFromSyncResults(ctx context.Context,
results map[string]types.ADCExecutionErrors) {
+// Resource status follows the whole skip table instead, since an excluded
resource stays
+// dropped until its owner is written again. A config's resource set can be
large, so
+// resources keep a small persisted delta in d.resourceFailures rather than
being rewritten
+// every round: resources that are dropped or failing are written SyncFailed,
and any
+// resource that was last round but isn't now gets its error explicitly
cleared with an
+// Accepted write.
+//
+// It reports whether anything was newly excluded, which is what makes the
next push
+// different from the one that just failed: see sync().
+func (d *apisixProvider) updateStatusFromSyncResults(ctx context.Context,
results map[string]types.ADCExecutionErrors, revisions map[string]uint64) bool {
resourceFailures := map[types.NamespacedNameKind][]string{}
+ newlyExcluded := 0
for configName, execErrs := range results {
+ dropped := map[wireKey]exclusion{}
+ gatewayProxyMsgs, failedEndpoints :=
d.classifySyncResult(configName, execErrs, dropped, resourceFailures)
+ builtRevision := revisions[configName]
+ newlyExcluded += d.skipped.MarkFailing(configName, dropped,
func(owner types.NamespacedNameKind) bool {
+ return d.store.ChangedSince(configName, owner,
builtRevision)
+ })
Review Comment:
Good catch, fixed in 7d7f424d: the lookup is now checked with the ok form,
and skips recording that config's newly rejected resources with a logged error
instead of silently treating a missing revision as 0. As you noted this can't
happen with sync() as written today (revisions and results are always populated
together in the same build closure), but the guard is cheap and protects
against a future refactor breaking that invariant.
--
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]
