bzp2010 commented on code in PR #2890:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2890#discussion_r4080939837
##########
internal/adc/cache/store.go:
##########
@@ -86,6 +100,52 @@ 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
Review Comment:
Agreed this is a real gap: lastChange only ever grows, and DeleteAll is the
only thing that prunes a cacheKey's whole map, never an individual owner.
Pruning safely means knowing no in-flight sync result can still reference an
owner's old revision, which touches the sync loop's own lifecycle rather than
being a local change to this file, so leaving it for this PR would widen scope
past what it's meant to review. Deferring this to a small follow-up PR rather
than fixing it here.
##########
internal/adc/cache/store.go:
##########
@@ -118,11 +178,18 @@ func routeOwner(targetCache Cache, id string)
(types.NamespacedNameKind, bool) {
// setGlobalRules is Insert and SetGlobalRules' shared implementation. Callers
must
// already hold s.Lock.
-func (s *Store) setGlobalRules(targetCache Cache, owner
types.NamespacedNameKind, plugins adctypes.GlobalRule) error {
+func (s *Store) setGlobalRules(name string, targetCache Cache, owner
types.NamespacedNameKind, plugins adctypes.GlobalRule) error {
Review Comment:
Fixed in 7a278941, together with the setPluginMetadata instance below:
recordChange is no longer deferred, it's called explicitly right before each
function's own successful return, so an error partway through (a failed
Delete/InsertGlobalRule) no longer leaves a phantom revision bump behind.
##########
internal/adc/cache/store.go:
##########
@@ -118,11 +178,18 @@ func routeOwner(targetCache Cache, id string)
(types.NamespacedNameKind, bool) {
// setGlobalRules is Insert and SetGlobalRules' shared implementation. Callers
must
// already hold s.Lock.
-func (s *Store) setGlobalRules(targetCache Cache, owner
types.NamespacedNameKind, plugins adctypes.GlobalRule) error {
+func (s *Store) setGlobalRules(name string, targetCache Cache, owner
types.NamespacedNameKind, plugins adctypes.GlobalRule) error {
rows, err := targetCache.ListGlobalRules(&OwnerSelector{Owner: owner})
if err != nil {
return err
}
+ existing := make(adctypes.Plugins, len(rows))
+ for _, row := range rows {
+ existing[row.ID] = row.Config
+ }
+ if pluginsContent(existing) !=
pluginsContent(adctypes.Plugins(plugins)) {
+ defer s.recordChange(name, owner)
+ }
Review Comment:
Same fix as setGlobalRules above, in 7a278941.
##########
internal/adc/cache/store.go:
##########
@@ -86,6 +100,52 @@ 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](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(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.
+func canonicalJSON(v any) string {
+ b, err := json.Marshal(v)
+ if err != nil {
+ return ""
+ }
+ var generic any
+ if err := json.Unmarshal(b, &generic); err != nil {
+ return string(b)
+ }
+ b, _ = json.Marshal(generic)
+ return string(b)
+}
Review Comment:
canonicalJSON now logs both marshal failures instead of swallowing them
(7a278941), so a real occurrence is visible. Not changing the two call sites to
force changed semantics on error for now: v here is always a type this package
built from already-parsed JSON, so a Marshal failure is defensive rather than
expected, and forcing changed would mean plumbing an ok/error signal back
through contentOf and pluginsContent into every one of their callers for a path
that shouldn't be reachable in practice.
--
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]
