-
Notifications
You must be signed in to change notification settings - Fork 469
Implementing a resolver that resolves worker configurations from specified probing paths #11258
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.
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
base: dev
Are you sure you want to change the base?
Conversation
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/WebHostServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/DefaultWorkerConfigurationResolver.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/DynamicWorkerConfigurationResolver.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/DynamicWorkerConfigurationResolver.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationHelper.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/DynamicWorkerConfigurationResolver.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/DynamicWorkerConfigurationResolver.cs
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/DynamicWorkerConfigurationResolver.cs
Show resolved
Hide resolved
var loggerFactory = p.GetService<ILoggerFactory>(); | ||
var metricsLogger = p.GetService<IMetricsLogger>(); | ||
|
||
return workerConfigurationResolverOptions?.CurrentValue?.IsDynamicWorkerResolutionEnabled is true ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is IsDynamicWorkerResolutionEnabled
expected to change value in the lifetime of a host process? You are using IOptionsMonitor
and .CurrentValue
, which makes it look like you expect it to change. But you are registering a singleton here. This factory will only ever be run once. So, once a dynamic or default config resolver is selected it will never change.
src/WebJobs.Script/Workers/Rpc/Configuration/DefaultWorkerConfigurationResolver.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/DefaultWorkerConfigurationResolver.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/DefaultWorkerConfigurationResolver.cs
Outdated
Show resolved
Hide resolved
public static readonly long DefaultMaxRequestBodySize = 104857600; | ||
|
||
public static readonly ImmutableArray<string> SystemLogCategoryPrefixes = ImmutableArray.Create("Microsoft.Azure.WebJobs.", "Function.", "Worker.", "Host."); | ||
public static readonly ImmutableHashSet<string> HostCapabilities = ImmutableHashSet.Create<string>(StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is immutable, readonly
, and has no values. It will never contain a value. What is the purpose of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today, we don't have any capability that requires negotiation. But if in future, we introduce a capability then we can add in this structure?
For example, there is a new feature "feature1" and during worker probing we want to make sure that host supports "feature1" then we can add as follows in that Host version.
HostCapabilities = ImmutableHashSet.Create<string>("feature1", "feature2");
/// Retrieves a dictionary of worker configurations, keyed by language name. | ||
/// </summary> | ||
WorkerConfigurationInfo GetConfigurationInfo(); | ||
Dictionary<string, RpcWorkerConfig> GetWorkerConfigs(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider returning IReadOnlyDictionary
, unless you expect callers to mutate it.
Preferring IReadOnlyDictionary
opens up the door for implementations to cache their result and return it in future calls, as they can assert no caller has mutated it underneath them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverOptionsSetup.cs
Outdated
Show resolved
Hide resolved
/// <summary> | ||
/// Gets or sets the worker runtimes available for resolution via Hosting configuration. | ||
/// </summary> | ||
public ImmutableHashSet<string> WorkersAvailableForResolution { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only ever use immutable collections for a very specific case: when you expect callers to take your value AND "mutate" their own copy of it, but you don't want the original instance to be affected.
If these are expected to be purely read only (you don't expect callers to make adjustments), then use a readonly interface.
IReadOnlySet<T>
, IReadOnlyDictionary<TKey, TValue>
To give an example of a good immutable collection use case is Roslyn analyzers. Roslyn will provide a bunch of collections to all the different analyzers that are expected to make changes to them and return those changes. But these analyzers are all ran in parallel, so regular collections wouldn't be thread safe! So Roslyn uses immutable collects to send the initial values to all the analyzers, then evaluates any changes the analyzers made afterwards and merges them together. (this is my very basic understanding of Roslyn, it may not be 100% accurate)
src/WebJobs.Script/Workers/Rpc/Configuration/DynamicWorkerConfigurationResolver.cs
Show resolved
Hide resolved
ArgumentNullException.ThrowIfNull(loggerFactory); | ||
_logger = loggerFactory.CreateLogger(ScriptConstants.LogCategoryWorkerConfig); | ||
_workerConfigurationResolverOptions = workerConfigurationResolverOptions ?? throw new ArgumentNullException(nameof(workerConfigurationResolverOptions)); | ||
_metricsLogger = metricsLogger ?? throw new ArgumentNullException(nameof(metricsLogger)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ArgumentNullException.ThrowIfNull(metricsLogger)
similar recommendation for other arguments as well.
Issue describing the changes in this PR
resolves #10944
Validation
E2E scenarios have been validated as outlined in the doc. We can run another round of E2E validations if there are significant changes in the code. Otherwise, these are not required for general changes and tests in the PR are sufficient.
Gists
A lot of code has been moved from RPCWorkerConfigFactory to WorkerConfigurationHelper so it can be shared by 2 resolvers. Adding below gists help with the review. Most of it is just refactoring and no logic changes.
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-proc
branch to be included in Core Tools and non-Flex deployments.in-proc
branch is not requiredrelease_notes.md
Additional information
This pull request introduces changes to decouple language workers from the Host release and enable dynamic worker resolution.
Backlog issues - Link
Design doc - Link
Flows covered in this PR -
DynamicWorkerConfigurationResolver
, which dynamically resolves worker configurations based on -WorkerConfigurationHelper.cs
to enable reusing of workers profile evaluation logic between 2 resolvers.FeatureFlagDisableWorkerProbingPaths
to disable dynamic resolution feature by users.WorkerConfigurationResolverOptions
by getting required values from IConfiguration and IEnvironment such as worker probing paths, release channel, workers available for resolution via hosting config.Note: The decoupling workers flow is disabled by default in this PR. The flow can be enabled by setting the Hosting config which will be done after completing other relevant backlog items.