Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
af614bb
Forwarding the LanguageWorkerOptions from the host
fabiocav Aug 6, 2024
bfbfe56
Adding a change notification system to refresh Options. Using this to…
kshyju Sep 10, 2024
a1257e7
Fixed a test to reflect the new code changes.
kshyju Sep 11, 2024
eec5806
Shutdown channel when language worker options change
kshyju Sep 18, 2024
706dc1f
Fix test to reflect the exception message change.
kshyju Sep 27, 2024
624a870
Update src/WebJobs.Script/ScriptHostBuilderExtensions.cs
fabiocav Oct 8, 2024
1b8cff0
Update src/WebJobs.Script/ScriptHostBuilderExtensions.cs
fabiocav Oct 8, 2024
219cdf2
Merge branch 'dev' into fabiocav/iomlwo
kshyju Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/WebJobs.Script/Description/FunctionDescriptorProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,20 +181,18 @@ private bool TryParseTriggerParameter(BindingMetadata metadata, out ParameterDes

protected internal virtual void ValidateFunction(FunctionMetadata functionMetadata)
{
HashSet<string> names = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
HashSet<string> names = new(StringComparer.OrdinalIgnoreCase);
foreach (var binding in functionMetadata.Bindings)
{
ValidateBinding(binding);

// Ensure no duplicate binding names
if (names.Contains(binding.Name))
{
throw new InvalidOperationException(string.Format("Multiple bindings with name '{0}' discovered. Binding names must be unique.", binding.Name));
}
else
{
names.Add(binding.Name);
throw new InvalidOperationException($"{nameof(FunctionDescriptorProvider)}: Multiple bindings with name '{binding.Name}' discovered. Binding names must be unique.");
}

names.Add(binding.Name);
}

// Verify there aren't multiple triggers defined
Expand Down
8 changes: 3 additions & 5 deletions src/WebJobs.Script/Host/WorkerFunctionMetadataProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,13 +257,11 @@ internal static FunctionMetadata ValidateBindings(IEnumerable<string> rawBinding
// Ensure no duplicate binding names exist
if (bindingNames.Contains(functionBinding.Name))
{
throw new InvalidOperationException(string.Format("Multiple bindings with name '{0}' discovered. Binding names must be unique.", functionBinding.Name));
}
else
{
bindingNames.Add(functionBinding.Name);
throw new InvalidOperationException($"{nameof(WorkerFunctionDescriptorProvider)}: Multiple bindings with name '{functionBinding.Name}' discovered. Binding names must be unique.");
}

bindingNames.Add(functionBinding.Name);

// add binding to function.Bindings once validation is complete
function.Bindings.Add(functionBinding);
}
Expand Down
14 changes: 11 additions & 3 deletions src/WebJobs.Script/ScriptHostBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,10 +311,9 @@ public static IHostBuilder AddScriptHostCore(this IHostBuilder builder, ScriptAp
// Configuration
services.AddSingleton<IOptions<ScriptApplicationHostOptions>>(new OptionsWrapper<ScriptApplicationHostOptions>(applicationHostOptions));
services.AddSingleton<IOptionsMonitor<ScriptApplicationHostOptions>>(new ScriptApplicationHostOptionsMonitor(applicationHostOptions));

services.ConfigureOptions<ScriptJobHostOptionsSetup>();
services.ConfigureOptions<JobHostFunctionTimeoutOptionsSetup>();
// LanguageWorkerOptionsSetup should be registered in WebHostServiceCollection as well to enable starting worker processing in placeholder mode.
services.ConfigureOptions<LanguageWorkerOptionsSetup>();
services.AddOptions<WorkerConcurrencyOptions>();
services.ConfigureOptions<HttpWorkerOptionsSetup>();
services.ConfigureOptions<ManagedDependencyOptionsSetup>();
Expand All @@ -329,8 +328,17 @@ public static IHostBuilder AddScriptHostCore(this IHostBuilder builder, ScriptAp

services.AddSingleton<IFileLoggingStatusManager, FileLoggingStatusManager>();

if (!applicationHostOptions.HasParentScope)
if (applicationHostOptions.HasParentScope)
{
// Forward th host LanguageWorkerOptions to the Job Host.
liliankasem marked this conversation as resolved.
Show resolved Hide resolved
fabiocav marked this conversation as resolved.
Show resolved Hide resolved
var languageWorkerOptions = applicationHostOptions.RootServiceProvider.GetService<IOptionsMonitor<LanguageWorkerOptions>>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

@fabiocav fabiocav Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Services taking a dependency on IOptionsMonitor<LanguageWorkerOptions>
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

services.AddSingleton(languageWorkerOptions);
services.AddSingleton<IOptions<LanguageWorkerOptions>>((s) => new OptionsWrapper<LanguageWorkerOptions>(languageWorkerOptions.CurrentValue));
fabiocav marked this conversation as resolved.
Show resolved Hide resolved
services.ConfigureOptions<JobHostLanguageWorkerOptionsSetup>();
}
else
{
services.ConfigureOptions<LanguageWorkerOptionsSetup>();
AddCommonServices(services);
}

Expand Down
4 changes: 2 additions & 2 deletions src/WebJobs.Script/Workers/FunctionInvocationDispatcherFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public FunctionInvocationDispatcherFactory(IOptions<ScriptJobHostOptions> script
IHttpWorkerChannelFactory httpWorkerChannelFactory,
IRpcWorkerChannelFactory rpcWorkerChannelFactory,
IOptions<HttpWorkerOptions> httpWorkerOptions,
IOptionsMonitor<LanguageWorkerOptions> rpcWorkerOptions,
IOptionsMonitor<LanguageWorkerOptions> workerOptions,
IEnvironment environment,
IWebHostRpcWorkerChannelManager webHostLanguageWorkerChannelManager,
IJobHostRpcWorkerChannelManager jobHostLanguageWorkerChannelManager,
Expand All @@ -54,7 +54,7 @@ public FunctionInvocationDispatcherFactory(IOptions<ScriptJobHostOptions> script
eventManager,
loggerFactory,
rpcWorkerChannelFactory,
rpcWorkerOptions,
workerOptions,
webHostLanguageWorkerChannelManager,
jobHostLanguageWorkerChannelManager,
managedDependencyOptions,
Expand Down
21 changes: 21 additions & 0 deletions src/WebJobs.Script/Workers/Rpc/Configuration/LanguageWorkerOptionsSetup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using Microsoft.Azure.WebJobs.Script.Diagnostics;
using Microsoft.Azure.WebJobs.Script.Workers.Profiles;
using Microsoft.Extensions.Configuration;
Expand Down Expand Up @@ -56,4 +57,24 @@ public void Configure(LanguageWorkerOptions options)
options.WorkerConfigs = configFactory.GetConfigs();
}
}

internal class JobHostLanguageWorkerOptionsSetup : IPostConfigureOptions<LanguageWorkerOptions>
{
private readonly ILoggerFactory _loggerFactory;

public JobHostLanguageWorkerOptionsSetup(ILoggerFactory loggerFactory)
{
_loggerFactory = loggerFactory;
}

public void PostConfigure(string name, LanguageWorkerOptions options)
{
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, can we clarify that?

  1. Add a comment explaining this is only registered for JobHost scope
  2. 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."


var logger = _loggerFactory.CreateLogger("Host.LanguageWorkerConfig");
logger.LogInformation(message);
}
}
}
8 changes: 6 additions & 2 deletions src/WebJobs.Script/Workers/Rpc/FunctionRegistration/RpcFunctionInvocationDispatcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public RpcFunctionInvocationDispatcher(IOptions<ScriptJobHostOptions> scriptHost
IScriptEventManager eventManager,
ILoggerFactory loggerFactory,
IRpcWorkerChannelFactory rpcWorkerChannelFactory,
IOptionsMonitor<LanguageWorkerOptions> languageWorkerOptions,
IOptionsMonitor<LanguageWorkerOptions> workerOptions,
IWebHostRpcWorkerChannelManager webHostLanguageWorkerChannelManager,
IJobHostRpcWorkerChannelManager jobHostLanguageWorkerChannelManager,
IOptions<ManagedDependencyOptions> managedDependencyOptions,
Expand All @@ -83,7 +83,11 @@ public RpcFunctionInvocationDispatcher(IOptions<ScriptJobHostOptions> scriptHost
_webHostLanguageWorkerChannelManager = webHostLanguageWorkerChannelManager;
_jobHostLanguageWorkerChannelManager = jobHostLanguageWorkerChannelManager;
_eventManager = eventManager;
_workerConfigs = languageWorkerOptions?.CurrentValue?.WorkerConfigs ?? throw new ArgumentNullException(nameof(languageWorkerOptions));

// The state of worker configuration and the LanguageWorkerOptions will match the lifetime of the JobHost and the
// RpcFunctionInvocationDispatcher. So, we can safely cache the workerConfigs and workerRuntime here.
// Using IOptionsMonitor here to get the same cached version used by other copmponents and avoid a new instance initialization.
_workerConfigs = workerOptions?.CurrentValue?.WorkerConfigs ?? throw new ArgumentNullException(nameof(workerOptions));
_managedDependencyOptions = managedDependencyOptions ?? throw new ArgumentNullException(nameof(managedDependencyOptions));
_logger = loggerFactory.CreateLogger<RpcFunctionInvocationDispatcher>();
_rpcWorkerChannelFactory = rpcWorkerChannelFactory;
Expand Down
Loading