[DLX] Integrate cache and watcher into dlx struct #76

weilerN · 2025-07-13T10:04:17Z

Description

This PR integrates the cache and watcher components into the DLX to enable scale-from-zero functionality based on ingress resources rather than relying on request headers.

Changes Made

Refactored initialization logic in both the ingress cache and ingress watcher operator to support external usage from the DLX.
Integrated ingresscache.IngressHostCache and kube.IngressWatcher into the DLX.
Optimize the watcher's UpdateHandler by adding a fast exit when the old and new objects share the same ResourceVersion

References

Jira ticket link - https://iguazio.atlassian.net/browse/NUC-510

Additional Notes

This task will be ready for review once manual testing is complete, including the Nuclio side.
A separate ticket will handle integrating the cache into the requestHandler as part of completing the full solution

rokatyy

Nice! we are getting closer and closer to the final :)

Some questions and suggestions

rokatyy · 2025-07-15T15:47:07Z

cmd/dlx/app/dlx.go

+func createDLX(
+	resourceScaler scalertypes.ResourceScaler,
+	options scalertypes.DLXOptions,
+	kubeClientSet kubernetes.Interface,


why not adding kubeClientSet into options?

I initially assumed that options was meant only for static variables or configuration values.
From your comment, I understand that including runtime dependencies like the Kubernetes client in options is acceptable here—and likely the better practice in this case.

Fixed here - CR comments - move kubeClientSet to dlx.options, avoid tests addition…

rokatyy · 2025-07-15T15:52:27Z

pkg/kube/ingress.go

 type ingressValue struct {
 	name    string
 	host    string
 	path    string
+	version string


It's better to avoid changing the actual code for testing purposes, please remove this

During development, I initially planned to use this in the actual code (e.g., for logging), but it turned out to be redundant—removing it.

Fixed here - CR comments - move kubeClientSet to dlx.options, avoid tests addition…

rokatyy · 2025-07-15T15:54:40Z

pkg/kube/ingress.go

 	}

+	ctx, cancel := context.WithCancel(dlxCtx)


Suggested change

ctx, cancel := context.WithCancel(dlxCtx)

contextWithCancel, cancel := context.WithCancel(dlxCtx)

Fixed here - CR comments - move kubeClientSet to dlx.options, avoid tests addition…

rokatyy · 2025-07-15T15:56:14Z

pkg/kube/ingress_test.go

+			testOldObj := suite.createDummyIngress(testCase.testOldObj.host, testCase.testOldObj.path, testCase.testOldObj.version, testCase.testOldObj.targets)
+			testNewObj := suite.createDummyIngress(testCase.testNewObj.host, testCase.testNewObj.path, testCase.testNewObj.version, testCase.testNewObj.targets)


version can be passed as a parameter here, but let's not change the code

Fixed here - CR comments - move kubeClientSet to dlx.options, avoid tests addition…

rokatyy · 2025-07-15T16:00:22Z

pkg/dlx/dlx.go

 	return &DLX{
 		logger:  childLogger,
 		handler: handler,
 		server: &http.Server{
 			Addr: options.ListenAddress,
 		},
+		cache:   cache,


Why passing cache here if we already have it in watcher?

The watcher updates the cache, but the DLX itself will need to read from it when receiving a request.

@TomerShor but we can get it by dlx.watcher.cache, I think here it's better to stick with a single ownership and single source of truth (can also be done via encapsulation like watcher.Cache() or something if we need to only read it)

Technically, we could access the cache via watcher.cache, but I think it's cleaner to initialize the cache in the main function and pass it explicitly to both the watcher and (in the upcoming PR) to the request handler.

This approach promotes loose coupling — the request handler and the watcher both interact with the cache, but neither should own it. More importantly, it’s preferable that the request handler remains unaware of the watcher operator to maintain a clear separation of concerns. This approach helps minimize unnecessary dependencies between components.

Since the cache is fully wired during initialization, having it stored on the DLX struct may be redundant — I’ll consider removing it if it’s not directly needed there.

Let me know what you think =]

I removed the cache from the DLX struct - CR comments - remove the cache from the DLX struct

@weilerN Watcher writes to the cache, and DLX reads it. Since watcher is a part of DLX, the idea of initializing and passing the cache separately to both contradicts the Single Responsibility Principle. Only the watcher should have the ability to modify the cache; DLX should not — it merely reads from it. Allowing both components to be independently wired with the same cache introduces unnecessary risk.

This approach helps minimize unnecessary dependencies between components.

The question here is: why do we want to minimize dependencies in this specific case? In fact, the dependency between DLX and watcher isn't unnecessary — it's inherent. DLX already owns the watcher; reading data from a cache managed by its child is a natural and clear dependency.

Trying to artificially decouple them reduce "visible" coupling, it weakens architectural clarity. It also opens the door to subtle bugs — for example, if someone mistakenly passes a copy or separate instance of the cache, we’ll end up with divergence that’s hard to track. This kind of split ownership violates the principle of having a single source of truth.

@rokatyy Just to clarify — it’s the request handler, not the DLX itself, that reads from the cache. So either we pass the cache directly to the handler, or we pass the watcher and have the handler access the cache through the watcher.

If the concern is about restricting write access, exposing the full cache still leaves that possibility open. We’d need to either wrap it in the watcher (i.e. - watcher.ResolveFromCache() or something like that).

Since your main concern is ownership and making that responsibility explicit within the handler’s boundaries, I’ll follow your direction.
Let me know which approach you prefer ( watcher.cache.Get() or watcher.ResolveFromCache()), and I’ll update the code accordingly.

@rokatyy @TomerShor I did some changes here (CR comment - move the cache into the watcher and expose only the cach…) to address all the points discussed in this thread:

Exposing only the cache.Get to the handler

Single Responsibility Principle- the watcher is the responsible for the cache

Single source of truth- the cache inside the watcher

TomerShor

Minor comment. Overall looks great!

TomerShor · 2025-07-15T18:29:01Z

pkg/kube/ingress.go

+		iw.logger.DebugWith("No changes in resource, skipping",
+			"resourceVersion", oldIngressResource.ResourceVersion,
+			"ingressName", oldIngressResource.Name)


This will be super spammy - most of the time we won't have changes in the ingress, and with a small resync interval will get 1 log line for every nuclio ingress - could be a very large number.
No need for log here at all.

That's why this log is DEBUG =]
But ok I will remove

Fixed here - CR comments - move kubeClientSet to dlx.options, avoid tests addition…

…s to code structs and remove flooding log

rokatyy

Very well!

Just a minor comment, but anyway approved ✅

rokatyy · 2025-07-16T08:32:02Z

cmd/dlx/app/dlx.go

@@ -82,12 +82,12 @@ func Run(kubeconfigPath string,
 		return errors.Wrap(err, "Failed to get client configuration")
 	}

-	kubeClientSet, err := kubernetes.NewForConfig(restConfig)
+	dlxOptions.KubeClientSet, err = kubernetes.NewForConfig(restConfig)


if dlxOptions.KubeClientSet, err = kubernetes.NewForConfig(restConfig); err != nil {}

Fixed here - CR comment - proper err handling

rokatyy

Great idea with a nested reader interface!

…e.Get method

TomerShor · 2025-07-17T08:55:59Z

pkg/scalertypes/types.go

+	ResolveTargetsFromIngressCallback ResolveTargetsFromIngressCallback `json:"-"`
+	ResyncInterval                    Duration
+	KubeClientSet                     kubernetes.Interface `json:"-"`


Why is the json:"-" needed?

When starting the DLX, an error log appears indicating that the kubeClient failed to be marshaled.
Adding this annotation resolves this false-positive error.

### Description This PR integrates the cache into the handler in order to resolve the request target based on the new cache. ### Changes Made - Add the `IngressHostCacheReader` (the cache read-only access) to the handler - Add the logic for the new resolving (i.e. - try to resolve based on the cache, and if failed - resolve by the existing headers' logic) ### Testing - Added a flow test - for the entire request handling - Added unit tests for the resolving per case (i.e. - based on headers and based on ingress) ### References - Jira ticket link: https://iguazio.atlassian.net/browse/NUC-523 - External link: this PR is based on this branch - #76 ### Additional Notes - I left a **TODO** question inside the test file — it seems that when parsing the `targetURL`, the handler appends the path twice. This fix is relatively minor and has been tested manually, but since the current code still works, it's worth deciding whether we want to address it now.

### Description This PR integrates the new DLX from the Scaler package, which introduces a revised internal flow. The changes align the DLX initialization with the new Scaler initialization and configuration patterns by updating the `dlxOptions` and related logic. ### Changes Made - Extracted hardcoded configuration variables - Implemented the `scalertype.ResolveTargetsFromIngressCallback` for extracting Nuclio targets - Added changes to the DLX init (as described in the Jira) ### Testing - Unit tests for the new `resolveCallback` - Manual integration tests to verify the functionalities ### References - Jira ticket link - https://iguazio.atlassian.net/browse/NUC-509 - External links - the related scaler PRs -- v3io/scaler#76 -- v3io/scaler#78 ### Additional Notes - Before merging this PR, the `go.mod` must be modified since it currently relies on a feature branch --------- Co-authored-by: Katerina Molchanova <[email protected]>

Ingress cache logger init

842731f

weilerN changed the title ~~Nuc 510 integrate cache and watcher into dlx~~ [DLX] Integrate cache and watcher into dlx init Jul 13, 2025

weilerN changed the title ~~[DLX] Integrate cache and watcher into dlx init~~ [DLX] Integrate cache and watcher into dlx handler Jul 13, 2025

weilerN mentioned this pull request Jul 14, 2025

[DLX] Integrate the new dlx nuclio/nuclio#3708

Merged

weilerN force-pushed the nuc-510-integrate-cache-and-watcher-into-dlx branch 2 times, most recently from b167f5c to 38a06d3 Compare July 15, 2025 14:27

Ingress watcher init - include context and fix duration init

4ffa9d6

weilerN force-pushed the nuc-510-integrate-cache-and-watcher-into-dlx branch 2 times, most recently from f2f3ca0 to 6163d52 Compare July 15, 2025 15:22

weilerN changed the title ~~[DLX] Integrate cache and watcher into dlx handler~~ [DLX] Integrate cache and watcher into dlx struct Jul 15, 2025

weilerN added 5 commits July 15, 2025 18:31

Move mutual variables to scalerTypes and add dlxOptions

e546acd

Move getClientConfig to common

6755685

Add kubeClientSet to the newDLX

5aa4898

Add cache and watcher to the DLX init

1ca9836

Compare ingress.resourceVersion to avoid redundant updates

316dd12

weilerN force-pushed the nuc-510-integrate-cache-and-watcher-into-dlx branch from 6163d52 to 316dd12 Compare July 15, 2025 15:33

weilerN marked this pull request as ready for review July 15, 2025 15:42

weilerN requested review from rokatyy and TomerShor and removed request for rokatyy July 15, 2025 15:42

rokatyy requested changes Jul 15, 2025

View reviewed changes

TomerShor requested changes Jul 15, 2025

View reviewed changes

weilerN added 2 commits July 16, 2025 11:02

CR comments - move kubeClientSet to dlx.options, avoid tests addition…

abead1c

…s to code structs and remove flooding log

CR comments - remove the cache from the DLX struct

667935b

weilerN requested review from TomerShor and rokatyy July 16, 2025 08:09

rokatyy approved these changes Jul 16, 2025

View reviewed changes

CR comment - proper err handling

adc046b

rokatyy approved these changes Jul 16, 2025

View reviewed changes

CR comment - move the cache into the watcher and expose only the cach…

659d388

…e.Get method

weilerN force-pushed the nuc-510-integrate-cache-and-watcher-into-dlx branch from 44134be to 659d388 Compare July 16, 2025 14:30

weilerN mentioned this pull request Jul 16, 2025

[Handler] Integrate the watcher's cache into request handler #77

Closed

TomerShor reviewed Jul 17, 2025

View reviewed changes

weilerN requested a review from TomerShor July 17, 2025 09:00

TomerShor approved these changes Jul 17, 2025

View reviewed changes

TomerShor merged commit 59f2bc3 into v3io:development Jul 17, 2025
3 checks passed

weilerN mentioned this pull request Jul 17, 2025

[Handler] Integrate the watcher's cache into request handler #78

Merged

	ctx, cancel := context.WithCancel(dlxCtx)
	contextWithCancel, cancel := context.WithCancel(dlxCtx)

		testOldObj := suite.createDummyIngress(testCase.testOldObj.host, testCase.testOldObj.path, testCase.testOldObj.version, testCase.testOldObj.targets)
		testNewObj := suite.createDummyIngress(testCase.testNewObj.host, testCase.testNewObj.path, testCase.testNewObj.version, testCase.testNewObj.targets)

[DLX] Integrate cache and watcher into dlx struct #76

[DLX] Integrate cache and watcher into dlx struct #76

Uh oh!

Conversation

weilerN commented Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes Made

References

Additional Notes

Uh oh!

rokatyy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rokatyy Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomerShor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rokatyy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rokatyy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

weilerN commented Jul 13, 2025 •

edited

Loading

rokatyy Jul 16, 2025 •

edited

Loading