Forwarding the LanguageWorkerOptions from the host to the job host scope and ensuring we use the provided instances. #10369

fabiocav · 2024-08-06T00:57:49Z

Issue describing the changes in this PR

Addressing an issue leading to double initialization of worker configuration and profile evaluation.

Pull request checklist

IMPORTANT: Currently, changes must be backported to the in-proc branch to be included in Core Tools and non-Flex deployments.

Backporting to the in-proc branch is not required
- Otherwise: Link to backporting PR
My changes do not require documentation changes
- Otherwise: Documentation issue linked to PR
My changes should not be added to the release notes for the next release
- Otherwise: I've added my notes to release_notes.md
My changes do not need to be backported to a previous version
- Otherwise: Backport tracked by issue/PR #issue_or_pr
My changes do not require diagnostic events changes
- Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
I have added all required tests (Unit tests, E2E tests)

to the job host scope and ensuring we use the provided instances.

src/WebJobs.Script/ScriptHostBuilderExtensions.cs

liliankasem · 2024-09-10T21:22:26Z

Is it possible to add tests to validate this change?

… refresh LanguageWorkerOption after script host is built. This will cause the LanguageWorkerOption to be refreshed after specialization.

kshyju · 2024-09-28T15:06:39Z

@brettsam Ping on the review.

brettsam · 2024-10-08T17:34:56Z

src/WebJobs.Script/Workers/Rpc/Configuration/LanguageWorkerOptionsSetup.cs

@@ -42,18 +47,53 @@ public void Configure(LanguageWorkerOptions options)
        {
            string workerRuntime = _environment.GetEnvironmentVariable(RpcWorkerConstants.FunctionWorkerRuntimeSettingName);

-            // Parsing worker.config.json should always be done in case of multi language worker
+            // Parsing worker.latestConfiguration.json should always be done in case of multi language worker


Is worker.latestConfiguration.json a new thing?

This is looking like an accidental change to me. I don't expect a different file name.

brettsam · 2024-10-08T17:39:42Z

src/WebJobs.Script/Workers/Rpc/WebHostRpcWorkerChannelManager.cs

+                if (rpcWorkerChannel != null && !UsePlaceholderChannel(rpcWorkerChannel))
+                {
+                    _logger.LogInformation("Language worker options changed, and the placeholder worker channel is invalid for other reasons. Shutting down the channel.");
+                    await ShutdownChannelIfExistsAsync(_workerRuntime, rpcWorkerChannel.Id);


What's the scenario here, and why didn't we have this behavior before?

Will need to review this to comment. That's part of a set of changes pushed by @kshyju .

Changes to the signal used to reload LanguageWorkerOptions were required. I'm concerned that the logic here is assuming this will always change as a result of specialization, which may change in the future.

At the very least, the message needs to be a bit more descriptive.

src/WebJobs.Script/ScriptHostBuilderExtensions.cs

jviau · 2024-10-08T18:34:53Z

src/WebJobs.Script/ScriptHostBuilderExtensions.cs

+                if (applicationHostOptions.HasParentScope)
+                {
+                    // Forward th host LanguageWorkerOptions to the Job Host.
+                    var languageWorkerOptions = applicationHostOptions.RootServiceProvider.GetService<IOptionsMonitor<LanguageWorkerOptions>>();


I am slightly concerned about how this will work in practice. There are subtleties to the way options are registered and calculated. I would feel better if we did the following:

services.AddSingleton(_ => applicationHostOptions.RootServiceProvider.GetService<IOptionsMonitor<LanguageWorkerOptions>>()); services.AddScoped(_ => applicationHostOptions.RootServiceProvider.GetService<IOptionsSnapshot<LanguageWorkerOptions>>()); services.AddSingleton(_ => applicationHostOptions.RootServiceProvider.GetService<IOptions<LanguageWorkerOptions>>());

Could even lift this to an extension method IServiceCollection ForwardOptionsFrom<TOptions>(this IServiceCollection, IServiceProvider source);

Can you expand on the concerns? I like the idea of wrapping that into an extension to make this cleaner and reusable (this is applied to other options). But for the functionality, in this case, we are explicitly trying to avoid another cache miss when using IOptions<T>, for LanguageWorkerOptions specifically, given the cost of running its setup today and the state it tracks (those instances would be identical).

Also, one thing to note is that using the suggested pattern will defer execution of the setup to the time services in the child container request the options, if nothing at the host scope consumes one of those option types.

Also, one thing to note is that using the suggested pattern will defer execution of the setup to the time services in the child container request the options, if nothing at the host scope consumes one of those option types.

That was intentional. I wanted to ensure IOptions in both parent and child container would be the same values. Otherwise, the two containers could potentially have different values if config changes between the two. Unlikely, but that would be one confusing bug if it ever happens.

we are explicitly trying to avoid another cache miss when using IOptions

Would this cache miss not already exist in the parent container?

With the pattern currently used in the code, the values would be the same, also refreshed when using IOptionsMonitor, one of the differences is that when using IOptions<T>, you'd end up with the same instance that is returned by IOptionsMonitor<T> (at the initial construction here), but that's intentional.

Would this cache miss not already exist in the parent container?

Right now, there are two cache misses in the JobHost/ScriptHost scope with every single initialization:

Services taking a dependency on IOptionsMonitor<LanguageWorkerOptions>

Services taking a dependency on IOptions<LanguageWorkerOptions>

If no WebHost services are using IOptions<LanguageWorkerOptions> (or if that ever changes in the future as a result of changes to WebHost scoped services), we'd end up with a cache miss when initializing the JobHost, which would have an impact on cold start in some cases (specialization flows shouldn't be as impacted as the expectation is that placeholders would have forced this to happen).

The IOptions<LanguageWorkerOptions> is the second hit we're trying to avoid.

For the WebHost, in specialization cases, even if the host has services consuming IOptions<LanguageWorkerOptions>, those wouldn't impact customer cold start as they would happen in placeholders.

Ultimately, much of what we're dealing with here is the fact the this current setup implementation is doing way too much and that logic should be refactored into a separate, singleton, service that is reused, avoiding the duplicate execution of that expensive logic that is not expected to run more than once anyway.

The WebHost won't ever be realizing IOptions<LanguageWorkerOptions> when we have a customer payload? If that is the case, then I am fine with the approach as is.

fabiocav · 2024-10-08T18:42:33Z

@jviau / @brettsam / @liliankasem I'm reviewing the latest changes, and will take point on addressing any feedback.

Co-authored-by: Jacob Viau <[email protected]>

jviau · 2024-10-09T16:59:40Z

src/WebJobs.Script/Workers/Rpc/Configuration/LanguageWorkerOptionsSetup.cs

+        {
+            var message = $"Call to configure {nameof(LanguageWorkerOptions)} from the JobHost scope. " +
+                $"If using {nameof(IOptions<LanguageWorkerOptions>)}, please use {nameof(IOptionsMonitor<LanguageWorkerOptions>)} instead.";
+            Debug.Fail(message);


Can you explain the intention behind this? The comment says to use IOptionsMonitor<LanguageWorkerOptions>, but won't this class still run (and fail) for that type?

The message is a bit confusing, but with the forwarding, no setups should run for IOptionsMonitor<LanguageWorkerOptions> within the JobHost scope. The same applies to IOptions<LanguageWorkerOptions, though, so either option would take the host injected instances and not run any setup logic within that scope.

Ah, can we clarify that?

Add a comment explaining this is only registered for JobHost scope

Update debug message to something like "Unexpected configuration of LanguageWorkerOptions from the JobHost scope. LanguageWorkerOptions should be forwarded from the parent scope with no additional configuration."

fabiocav requested a review from a team as a code owner August 6, 2024 00:57

fabiocav requested review from kshyju and brettsam August 6, 2024 00:58

fabiocav force-pushed the fabiocav/iomlwo branch 2 times, most recently from c71bd21 to 1306919 Compare August 6, 2024 01:01

Forwarding the LanguageWorkerOptions from the host

af614bb

to the job host scope and ensuring we use the provided instances.

fabiocav force-pushed the fabiocav/iomlwo branch from 1306919 to af614bb Compare August 13, 2024 23:18

liliankasem reviewed Sep 10, 2024

View reviewed changes

src/WebJobs.Script/ScriptHostBuilderExtensions.cs Outdated Show resolved Hide resolved

kshyju added 2 commits September 11, 2024 07:22

Adding a change notification system to refresh Options. Using this to…

bfbfe56

… refresh LanguageWorkerOption after script host is built. This will cause the LanguageWorkerOption to be refreshed after specialization.

Fixed a test to reflect the new code changes.

a1257e7

fabiocav marked this pull request as draft September 16, 2024 16:24

kshyju added 2 commits September 18, 2024 07:08

Shutdown channel when language worker options change

eec5806

Fix test to reflect the exception message change.

706dc1f

kshyju marked this pull request as ready for review September 28, 2024 00:26

brettsam reviewed Oct 8, 2024

View reviewed changes

jviau reviewed Oct 8, 2024

View reviewed changes

fabiocav and others added 2 commits October 8, 2024 11:49

Update src/WebJobs.Script/ScriptHostBuilderExtensions.cs

624a870

Co-authored-by: Jacob Viau <[email protected]>

Update src/WebJobs.Script/ScriptHostBuilderExtensions.cs

1b8cff0

Co-authored-by: Jacob Viau <[email protected]>

jviau reviewed Oct 9, 2024

View reviewed changes

jviau mentioned this pull request Oct 21, 2024

Address LanguageWorkerOptions null in FunctionMetadataManager #10550

Open

9 tasks

Merge branch 'dev' into fabiocav/iomlwo

219cdf2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forwarding the LanguageWorkerOptions from the host to the job host scope and ensuring we use the provided instances. #10369

Forwarding the LanguageWorkerOptions from the host to the job host scope and ensuring we use the provided instances. #10369

fabiocav commented Aug 6, 2024 •

edited

Loading

liliankasem commented Sep 10, 2024

kshyju commented Sep 28, 2024

brettsam Oct 8, 2024

fabiocav Oct 8, 2024

brettsam Oct 8, 2024

fabiocav Oct 8, 2024

jviau Oct 8, 2024

fabiocav Oct 8, 2024

fabiocav Oct 8, 2024 •

edited

Loading

jviau Oct 8, 2024

fabiocav Oct 8, 2024

jviau Oct 8, 2024

fabiocav commented Oct 8, 2024

jviau Oct 9, 2024

fabiocav Oct 9, 2024

jviau Oct 9, 2024

Forwarding the LanguageWorkerOptions from the host to the job host scope and ensuring we use the provided instances. #10369

Are you sure you want to change the base?

Forwarding the LanguageWorkerOptions from the host to the job host scope and ensuring we use the provided instances. #10369

Conversation

fabiocav commented Aug 6, 2024 • edited Loading

Issue describing the changes in this PR

Pull request checklist

liliankasem commented Sep 10, 2024

kshyju commented Sep 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiocav Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiocav commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiocav commented Aug 6, 2024 •

edited

Loading

fabiocav Oct 8, 2024 •

edited

Loading