Copilot commented on code in PR #2890:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2890#discussion_r4082261231
##########
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:
`maps.Values(s.lastChange)` already returns a slice, so wrapping it in
`slices.Collect(...)` is invalid and likely won’t compile. Use
`slices.ContainsFunc(maps.Values(s.lastChange), ...)` directly, or iterate with
a simple `for _, byOwner := range maps.Values(...)` loop.
##########
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:
This does a marshal → unmarshal → marshal cycle for every change-detection
comparison, which can get expensive for large resource sets or frequent
reconciles. If this becomes hot, consider replacing it with a cheaper stable
fingerprint (e.g., hashing `json.Marshal` output once per write and caching
per-owner, or a structural comparison limited to fields that affect the ADC
payload) to reduce allocations and CPU.
##########
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:
`builtRevision := revisions[configName]` silently falls back to `0` if the
key is missing, which would make `ChangedSince(...)` likely treat many
exclusions as stale and skip recording them. Consider checking the map lookup
(`rev, ok := revisions[configName]`) and logging/handling the unexpected
missing-revision case explicitly so failures aren’t dropped silently if future
refactors change how `results`/`revisions` are populated.
--
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]
