Performance improvement based on pprofile - #327
Conversation
Signed-off-by: Arrobo, Gabriel <gabriel.arrobo@intel.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces caching for UDM UE Authentication (UEAU) resolution and client creation to reduce repeated NRF discovery and repeated OpenAPI client instantiation during authentication flows.
Changes:
- Cache resolved UDM UEAU URL in the AUSF global context to avoid repeated discovery.
- Add a mutex-protected cached
Nudm_UEAU.APIClientto reuse the UDM client instance. - Reset the cached UDM URL in tests to keep subtests independent.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| producer/functions.go | Adds caching for resolved UDM URL and UDM OpenAPI client to reduce per-request overhead. |
| ausf_test.go | Clears the global cached UDM URL between subtests to keep expected call counts stable. |
Suppressed comments (3)
producer/functions.go:317
self.UdmUeauUrlis written without synchronization. This can race with other goroutines callingGetUdmUrlat the same time.
if apiPrefix, ok := ueauService.GetApiPrefixOk(); ok && apiPrefix != nil && *apiPrefix != "" {
self.UdmUeauUrl = *apiPrefix
return *apiPrefix
}
producer/functions.go:324
self.UdmUeauUrlis written without synchronization when building a URL from IP endpoints, which can race with concurrent readers/writers.
url := string(ueauService.GetScheme()) + "://" + ueauEndPoint.GetIpv4Address() + ":" + strconv.Itoa(int(ueauEndPoint.GetPort()))
self.UdmUeauUrl = url
return url
producer/functions.go:339
createClientToUdmUeaureturns the cached client even when called with a differentudmUrl, so callers can silently use a client configured with the wrongapiRoot. Recreate the client when the URL changes (or cache per-URL).
func createClientToUdmUeau(udmUrl string) *Nudm_UEAU.APIClient {
udmCacheMu.Lock()
defer udmCacheMu.Unlock()
if cachedUdmClient != nil {
return cachedUdmClient
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
producer/functions.go:332
- This writes the derived UDM URL into the global context cache even when EnableNrfCaching is disabled, which can cause subsequent calls to silently bypass discovery (once caching is enabled again). Consider only persisting the cache when EnableNrfCaching is enabled.
udmCacheMu.Lock()
self.UdmUeauUrl = url
udmCacheMu.Unlock()
return url
producer/functions.go:322
- This writes the resolved UDM URL into the global context cache even when EnableNrfCaching is disabled, which makes the caching behavior effectively always-on after the first successful resolution. Consider only persisting the cache when EnableNrfCaching is enabled.
udmCacheMu.Lock()
self.UdmUeauUrl = *apiPrefix
udmCacheMu.Unlock()
return *apiPrefix
producer/functions.go:293
- GetUdmUrl now returns a cached UDM URL unconditionally, which means disabling EnableNrfCaching will still reuse a previously resolved URL and skip NRF discovery. If caching is intended to be configurable, the cache read should be gated on EnableNrfCaching so behavior matches the setting consistently.
This issue also appears in the following locations of the same file:
- line 319
- line 329
udmCacheMu.Lock()
cached := self.UdmUeauUrl
udmCacheMu.Unlock()
if cached != "" {
return cached
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
producer/functions.go:348
createClientToUdmUeautakes an exclusive lock on every call, including the cached fast-path. Under load this can serialize all auth flows on a single mutex and become a bottleneck; prefer an RWMutex fast-path withRLock()and only takeLock()when rebuilding the client.
func createClientToUdmUeau(udmUrl string) *Nudm_UEAU.APIClient {
udmCacheMu.Lock()
defer udmCacheMu.Unlock()
if cachedUdmClient != nil && cachedUdmClientURL == udmUrl {
return cachedUdmClient
}
producer/functions.go:292
GetUdmUrlreturnsself.UdmUeauUrlonce set, but that cached value is never invalidated. This bypasses NRF discovery even after NRF cache eviction / NF deregistration events, so AUSF may keep using a stale UDM URL indefinitely after topology changes.
udmCacheMu.RLock()
cached := self.UdmUeauUrl
udmCacheMu.RUnlock()
if cached != "" {
return cached
|
As Copilot points out, the new UDM URL cache is permanent and separate from the NRF cache: it has no expiry or deregistration invalidation. That can retain a stale UDM endpoint after a topology change. Is it acceptable to have a stable UDM endpoint for the lifetime of AUSF? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (5)
producer/functions.go:333
- Same as above: this stores the computed endpoint URL into AUSFContext.UdmUeauUrl regardless of EnableNrfCaching. Guarding this write keeps “caching disabled” semantics consistent.
udmUrlMu.Lock()
self.UdmUeauUrl = url
udmUrlMu.Unlock()
ausf_test.go:138
- The tests cover the first GetUdmUrl call for each EnableNrfCaching setting, but not the important follow-up behavior (i.e., caching enabled should not re-query NRF on a second call, while caching disabled should). Adding a second call assertion here would prevent regressions in the new URL caching logic.
ausfContext.GetSelf().UdmUeauUrl = ""
ausfContext.GetSelf().EnableNrfCaching = parameters[i].inputEnableNrfCaching
udm_uri := producer.GetUdmUrl(ausfContext.GetSelf().NrfUri)
if callCountSearchNFInstances != parameters[i].expectedCallCountSearchNFInstances {
t.Errorf("NF instance search count mismatch. got = %d, want = %d (NF instance is searched in the cache)",
producer/functions.go:296
- GetUdmUrl now returns the cached AUSFContext.UdmUeauUrl unconditionally. This changes behavior when EnableNrfCaching is false: after the first discovery, subsequent calls will still return the cached URL and skip the intended “no caching” behavior.
udmUrlMu.RLock()
cached := self.UdmUeauUrl
udmUrlMu.RUnlock()
if cached != "" {
return cached
producer/functions.go:323
- This writes the discovered UDM URL into AUSFContext.UdmUeauUrl even when EnableNrfCaching is disabled. If caching is meant to be off, avoid populating the cache so later calls can’t accidentally reuse a stale value.
This issue also appears on line 331 of the same file.
udmUrlMu.Lock()
self.UdmUeauUrl = *apiPrefix
udmUrlMu.Unlock()
producer/functions.go:297
- When the cached UDM URL is empty, multiple concurrent callers can all pass this empty-cache check and perform NRF discovery at the same time (thundering herd). After startup or after invalidateUdmCache(), this can negate the intended performance improvement.
if cached != "" {
return cached
}
udmUrl := "https://localhost:29503" // default
Signed-off-by: Arrobo, Gabriel <gabriel.arrobo@intel.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
producer/functions.go:324
- GetUdmUrl writes the discovered URL into AUSFContext.UdmUeauUrl unconditionally. If you intend EnableNrfCaching=false to mean “do fresh discovery each time”, the write should also be gated; otherwise a later call (or runtime toggle) can start returning a cached URL even when caching was disabled during discovery.
if apiPrefix, ok := ueauService.GetApiPrefixOk(); ok && apiPrefix != nil && *apiPrefix != "" {
udmUrlMu.Lock()
self.UdmUeauUrl = *apiPrefix
udmUrlMu.Unlock()
return *apiPrefix
producer/functions.go:334
- Same as above for the endpoint-derived URL: the cache write should be conditional on EnableNrfCaching to avoid implicitly enabling caching when it’s configured off.
url := string(ueauService.GetScheme()) + "://" + ueauEndPoint.GetIpv4Address() + ":" + strconv.Itoa(int(ueauEndPoint.GetPort()))
udmUrlMu.Lock()
self.UdmUeauUrl = url
udmUrlMu.Unlock()
return url
producer/functions.go:296
- GetUdmUrl now short-circuits on AUSFContext.UdmUeauUrl even when EnableNrfCaching is false. That effectively introduces URL caching regardless of the config flag (consumer.SendSearchNFInstances uses EnableNrfCaching to decide whether to bypass the NRF cache), and can change expected behavior when caching is intentionally disabled.
This issue also appears in the following locations of the same file:
- line 320
- line 330
func GetUdmUrl(nrfUri string) string {
self := ausf_context.GetSelf()
udmUrlMu.RLock()
cached := self.UdmUeauUrl
udmUrlMu.RUnlock()
if cached != "" {
return cached
}
Fixed/Corrected |
No description provided.