bzp2010 commented on code in PR #2865:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2865#discussion_r3967388289
##########
internal/provider/apisix/provider.go:
##########
@@ -299,9 +390,37 @@ func (d *apisixProvider) Start(ctx context.Context) error {
}
func (d *apisixProvider) sync(ctx context.Context) error {
- statusesMap, err := d.client.Sync(ctx)
+ inputs, resourceErr := d.buildSyncInputs()
+ statusesMap, syncErr := d.client.Sync(ctx, inputs)
d.handleADCExecutionErrors(statusesMap)
- return err
+ return errors.Join(resourceErr, syncErr)
+}
+
+// buildSyncInputs organizes this round's full config set -- every
GatewayProxy AIC
+// currently knows about, each with its merged translated resource snapshot --
into the
+// input the adc client package needs. The client package never gathers this
itself.
+func (d *apisixProvider) buildSyncInputs() ([]adcclient.SyncInput, error) {
+ configs := d.configManager.List()
+ if len(configs) == 0 {
+ return nil, nil
+ }
+
+ inputs := make([]adcclient.SyncInput, 0, len(configs))
+ var errs []error
+ for _, config := range configs {
+ resources, err := d.store.GetResources(config.Name)
+ if err != nil {
+ d.log.Error(err, "failed to get resources from store",
"name", config.Name)
+ errs = append(errs, fmt.Errorf("config %s: %w",
config.Name, err))
+ continue
+ }
+ inputs = append(inputs, adcclient.SyncInput{
+ Name: config.Name,
+ Config: config,
+ Resources: resources,
+ })
+ }
+ return inputs, errors.Join(errs...)
}
Review Comment:
Deliberately left as-is: ConfigManager and Store already serialize their own
internal state independently (each method holds its own internal mutex), so
this isn't a memory-safety data race -- the risk is a narrow window where a
reader could see the two structures momentarily out of step with each other
(e.g. a config just registered but its store contribution not yet inserted).
Extending d.Lock()/RLock() to cover these reads would mean holding it for as
long as whatever the read feeds into runs, and on the sync path that includes
the HTTP push to ADC -- risking every other Update/Delete reconcile blocking
behind a slow or stuck push. The practical failure mode here is a transient,
one-round-stale read that the next sync cycle self-corrects, so we're accepting
that over widening the lock's scope.
##########
internal/provider/apisix/status.go:
##########
@@ -109,7 +109,7 @@ func (d *apisixProvider) updateStatus(nnk
types.NamespacedNameKind, condition me
}),
})
case types.KindHTTPRoute:
- parentRefs :=
d.client.ConfigManager.GetConfigRefsByResourceKey(nnk)
+ parentRefs := d.configManager.GetConfigRefsByResourceKey(nnk)
Review Comment:
Deliberately left as-is: ConfigManager and Store already serialize their own
internal state independently (each method holds its own internal mutex), so
this isn't a memory-safety data race -- the risk is a narrow window where a
reader could see the two structures momentarily out of step with each other
(e.g. a config just registered but its store contribution not yet inserted).
Extending d.Lock()/RLock() to cover these reads would mean holding it for as
long as whatever the read feeds into runs, and on the sync path that includes
the HTTP push to ADC -- risking every other Update/Delete reconcile blocking
behind a slow or stuck push. The practical failure mode here is a transient,
one-round-stale read that the next sync cycle self-corrects, so we're accepting
that over widening the lock's scope.
--
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]
