-
Notifications
You must be signed in to change notification settings - Fork 461
Implement changes needed in the Host to decouple workers from the Host release #11111
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: feature/decouple-workers
Are you sure you want to change the base?
Implement changes needed in the Host to decouple workers from the Host release #11111
Conversation
@@ -144,30 +176,16 @@ internal void AddProvider(string workerDir) | |||
|
|||
_logger.LogDebug("Found worker config: {workerConfigPath}", workerConfigPath); | |||
|
|||
var workerConfig = GetWorkerConfigJsonElement(workerConfigPath); |
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.
No change in logic for this line. Just moved the method GetWorkerConfigJsonElement()
to class WorkerConfigurationHelper
@@ -144,30 +176,16 @@ internal void AddProvider(string workerDir) | |||
|
|||
_logger.LogDebug("Found worker config: {workerConfigPath}", workerConfigPath); | |||
|
|||
var workerConfig = GetWorkerConfigJsonElement(workerConfigPath); | |||
var workerDescriptionElement = workerConfig.GetProperty(WorkerConstants.WorkerDescription); |
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.
All these lines (148-170) are moved to a method WorkerConfigurationHelper.GetWorkerDescription()
- https://gist.github.com/surgupta-msft/b416af7ca3c8794849832cc56f7a11de/revisions
@@ -213,61 +231,6 @@ internal void AddProvider(string workerDir) | |||
} | |||
} | |||
|
|||
private static JsonElement GetWorkerConfigJsonElement(string workerConfigPath) |
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.
These methods are also moved to class WorkerConfigurationHelper
as we will need to call these methods from class WorkerConfigurationResolver
as part of compatibility check
namespace Microsoft.Azure.WebJobs.Script.Workers.Rpc.Configuration | ||
{ | ||
internal static class WorkerConfigurationHelper | ||
{ |
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.
All the methods in this class are taken from RPCWorkerConfigFactory
with minor adjustments. See gist - https://gist.github.com/surgupta-msft/7eee560448e21ad1d450eb3e425895cd/revisions
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/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/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/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/WorkerConfigurationResolverFactory.cs
Outdated
Show resolved
Hide resolved
test/TestWorkers/Probingpaths/workers/node/3.10.1/worker.config.json
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/LanguageWorkerOptionsSetup.cs
Show resolved
Hide resolved
Minor comment on branch name, we should use |
private readonly ILogger _logger; | ||
private readonly IConfiguration _configuration; | ||
|
||
public DefaultWorkerConfigurationResolver(IConfiguration configuration, ILogger logger) |
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 missed some of this in the first pass -- but we're trying to move away from passing in IConfiguration or IEnvironment to any classes. They aren't very descriptive -- like right here you have no idea as a consumer what part of IConfiguration is needed.
Instead, using some kind of IOptions<> object that builds the options (using IConfiguration) before passing them in is a much nicer contract and much easier to test/read.
Highly recommend moving away from this where you can. I'll call out some others as I see 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.
If it works, I would prefer to address this issue in a separate PR. I have created an issue - #11185 for it and added it to the "Decoupling workers" milestone. This PR already contains a lot of logic and tests and it is also against a feature branch, so I am not merging changes into dev.
However, if you are really concerned about it, I can raise another PR with the fix targeting this PR branch (surgupta/decoupling-workers
). We can merge that PR first before completing this one. Though it is not my preferred approach, as this PR is becoming long-standing and we are already using this pattern in multiple places in Host code, but I can do it if necessary.
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.
Discussed offline. Will be implementing the improvement first.
src/WebJobs.Script/Workers/Rpc/Configuration/DynamicWorkerConfigurationResolver.cs
Show resolved
Hide resolved
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.
Let's also add the placeholder + specialization use case test to validate that flow.
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationHelper.cs
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/WorkerConfigurationResolverFactory.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Workers/Rpc/Configuration/DefaultWorkerConfigurationResolver.cs
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/DynamicWorkerConfigurationResolver.cs
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/LanguageWorkerOptionsSetup.cs
Outdated
Show resolved
Hide resolved
Yes, this will definitely be included with Integration and E2E tests changes. I am currently working on it - #11140 |
Issue describing the changes in this PR
resolves #10944
Update - working with Brett on addressing the comment - #11111 (comment).
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
Additional PR information
This pull request introduces changes to decouple language workers from the Host release and enable dynamic worker resolution. Key updates include new worker configuration resolvers, environment enhancements, and feature flags to control worker behavior.
Backlog issues - Link
Design doc - Link
Flows covered in this PR -
IWorkerConfigurationResolverFactory
to handle the instantiation logic of the resolver depending on if the feature is enabled.IWorkerConfigurationResolver
when creating an instance ofRPCWorkerConfigFactory
.GetWorkerConfigs()
method ofIWorkerConfigurationResolver
to get the required worker configs.WorkerConfigurationHelper.cs
to enable reusing of workers profile evaluation logic.DefaultWorkerConfigurationResolver
to scan the Host's "workers" directory for worker configurations.DynamicWorkerConfigurationResolver
, which dynamically resolves worker configurations based on -IsWindowsEnvironment()
to determine the windows environment type.IsDynamicWorkerResolutionEnabled
method toEnvironmentExtensions
class to enable/disable dynamic worker resolution using feature flag and hosting configuration.FeatureFlagDisableWorkerProbingPaths
to disable dynamic resolution feature by users.Note: The decoupling workers flow is disabled by default in this PR. We will enable the flow after completing other relevant backlog items which will be included in follow-up PRs.