Enabled windows ProcessorAffinity support#629
Enabled windows ProcessorAffinity support#629RakeshwarK wants to merge 19 commits intomicrosoft:mainfrom
Conversation
nchapagain001
left a comment
There was a problem hiding this comment.
Let's make changes directly in CreateProcessWithAffinity() to allow windows process affinity. Users shouldn't have to make additional call for the same thing.
There was a problem hiding this comment.
Pull request overview
This PR enables CPU affinity support for Virtual Client processes on Windows by using the Process.ProcessorAffinity API. The implementation follows a different approach than Linux: Windows applies affinity immediately after starting the process (creating a small ~1ms timing window), while Linux uses numactl to wrap the process execution command (zero timing window). The Windows implementation is limited to 64 cores due to processor group constraints.
Changes:
- Added
WindowsProcessAffinityConfigurationclass to handle Windows-specific CPU affinity using bitmask-based ProcessorAffinity API - Created
ApplyAffinityextension method forIProcessProxyto apply Windows affinity after process start - Added comprehensive unit and functional tests for Windows affinity support
- Provided example executor (
ExampleWorkloadWithAffinityExecutor) and profile demonstrating CPU affinity usage - Updated documentation to explain CPU affinity support and platform differences
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/VirtualClient/VirtualClient.Common/ProcessAffinity/WindowsProcessAffinityConfiguration.cs | Core implementation of Windows CPU affinity using ProcessorAffinity bitmask, includes reflection-based approach to access underlying process |
| src/VirtualClient/VirtualClient.Common/ProcessAffinity/ProcessAffinityConfiguration.cs | Updated factory method to support Windows platform in addition to Linux |
| src/VirtualClient/VirtualClient.Core/ProcessExtensions.cs | Added ApplyAffinity extension method for applying Windows affinity to running processes |
| src/VirtualClient/VirtualClient.TestFramework/InMemoryProcess.cs | Added OnApplyAffinity delegate to support testing affinity application |
| src/VirtualClient/VirtualClient.Common.UnitTests/ProcessAffinity/WindowsProcessAffinityConfigurationTests.cs | Unit tests for Windows affinity configuration including bitmask calculation and validation |
| src/VirtualClient/VirtualClient.Core.UnitTests/ProcessExtensionsAffinityTests.cs | Unit tests for process extension methods, verifying Windows throws on pre-start affinity methods |
| src/VirtualClient/VirtualClient.Actions/Examples/ExampleWorkloadWithAffinityExecutor.cs | Reference implementation showing how to use CPU affinity in workload executors (contains critical bug in Linux command construction) |
| src/VirtualClient/VirtualClient.Actions.FunctionalTests/ExampleWorkloadWithAffinityProfileTests.cs | Functional tests for the example affinity workload on both Linux and Windows platforms |
| src/VirtualClient/VirtualClient.Main/profiles/PERF-CPU-EXAMPLE-AFFINITY.json | Example profile demonstrating CPU affinity configuration |
| website/docs/developing/0030-workload-onboarding.md | Documentation for CPU affinity feature including core specification formats and platform differences |
| website/docs/developing/0010-develop-guide.md | Updated developer guide to reference CPU affinity support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/VirtualClient/VirtualClient.Actions/Examples/ExampleWorkloadWithAffinityExecutor.cs
Show resolved
Hide resolved
src/VirtualClient/VirtualClient.Common/ProcessAffinity/WindowsProcessAffinityConfiguration.cs
Outdated
Show resolved
Hide resolved
...rtualClient/VirtualClient.Actions.FunctionalTests/ExampleWorkloadWithAffinityProfileTests.cs
Outdated
Show resolved
Hide resolved
We cannot do that because for windows we can only apply the affinity once the process is started, In the executor there is explicit calls for StartAndWait after process creation so we need to call Start then apply affinity and call WaitAsync. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| process.OnApplyAffinity = (mask) => | ||
| { | ||
| // Verify affinity mask is set while process is running | ||
| Assert.IsFalse(hasExited, "Affinity should be applied while process is running"); | ||
| Assert.Greater(mask.ToInt64(), 0); | ||
| }; | ||
|
|
||
| // Simulate process completion when WaitForExitAsync is called | ||
| process.WaitForExitAsync(CancellationToken.None); | ||
| process.StandardOutput.Append("{ \"metric1\": 100, \"metric2\": 200 }"); | ||
| hasExited = true; |
There was a problem hiding this comment.
The test has a logic issue: it calls WaitForExitAsync synchronously (without await), then immediately sets hasExited = true and appends output. This means the test setup happens synchronously before the actual executor code runs.
The correct flow should be:
- Set up OnWaitForExit delegate (similar to OnStart) to set hasExited = true and append output when WaitForExitAsync is actually called by the executor
- Or, ensure the StandardOutput is appended before the OnStart completes
Currently, the affinity assertion at line 138 will always pass because hasExited is set to true after line 143 executes synchronously, but the executor's WaitForExitAsync call hasn't happened yet.
| process.OnApplyAffinity = (mask) => | |
| { | |
| // Verify affinity mask is set while process is running | |
| Assert.IsFalse(hasExited, "Affinity should be applied while process is running"); | |
| Assert.Greater(mask.ToInt64(), 0); | |
| }; | |
| // Simulate process completion when WaitForExitAsync is called | |
| process.WaitForExitAsync(CancellationToken.None); | |
| process.StandardOutput.Append("{ \"metric1\": 100, \"metric2\": 200 }"); | |
| hasExited = true; | |
| process.OnApplyAffinity = (mask) => | |
| { | |
| // Verify affinity mask is set while process is running | |
| Assert.IsFalse(hasExited, "Affinity should be applied while process is running"); | |
| Assert.Greater(mask.ToInt64(), 0); | |
| }; | |
| // Simulate process completion when WaitForExitAsync is actually called | |
| process.OnWaitForExitAsync = (token) => | |
| { | |
| process.StandardOutput.Append("{ \"metric1\": 100, \"metric2\": 200 }"); | |
| hasExited = true; | |
| return Task.CompletedTask; | |
| }; |
| The Virtual Client runtime platform execute operating system processes often as part of just about every workload/test executor, monitor or dependency installer/handler. In addition | ||
| there are times when processes need to be launched with elevated privileges. The responsibility for creating and managing processes within the runtime is encapsulated in the | ||
| following interfaces/classes. |
There was a problem hiding this comment.
The formatting of the "Process Management" section description has changed from being indented (as in other sections like "Package Management") to not being indented. This creates inconsistency with the rest of the document.
The original formatting had the description text indented with two spaces after the section header. To maintain consistency with sections like "Package Management", "Disk Management", etc., the description text on lines 344-346 should be indented.
| The Virtual Client runtime platform execute operating system processes often as part of just about every workload/test executor, monitor or dependency installer/handler. In addition | |
| there are times when processes need to be launched with elevated privileges. The responsibility for creating and managing processes within the runtime is encapsulated in the | |
| following interfaces/classes. | |
| The Virtual Client runtime platform execute operating system processes often as part of just about every workload/test executor, monitor or dependency installer/handler. In addition | |
| there are times when processes need to be launched with elevated privileges. The responsibility for creating and managing processes within the runtime is encapsulated in the | |
| following interfaces/classes. |
| // Check if this is a test/mock process with OnApplyAffinity delegate | ||
| // We use reflection to avoid a hard dependency on VirtualClient.TestFramework | ||
| System.Reflection.PropertyInfo affinityDelegateProperty = process.GetType().GetProperty( | ||
| "OnApplyAffinity", | ||
| System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance); | ||
|
|
||
| if (affinityDelegateProperty != null) | ||
| { | ||
| // This is a test/mock process - invoke the delegate instead of accessing real process | ||
| object delegateValue = affinityDelegateProperty.GetValue(process); | ||
| if (delegateValue != null) | ||
| { | ||
| (delegateValue as Action<IntPtr>)?.Invoke(this.AffinityMask); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The reflection-based approach to detect test/mock processes works, but it creates a tight coupling between production code and test infrastructure. If the InMemoryProcess class property name "OnApplyAffinity" changes, or if someone creates a different test double without this property, the production code will silently skip the affinity application without any error indication in tests.
Consider adding a marker interface (e.g., ITestProcessProxy) that InMemoryProcess can implement, and check for that interface instead of using reflection. This would make the test infrastructure dependency explicit and more maintainable.
| if (command.Contains("bash") && arguments.Contains("numactl")) | ||
| { | ||
| // Verify numactl wrapper is used for CPU affinity on Linux | ||
| process.StandardOutput.Append("{ \"metric1\": 100, \"metric2\": 200 }"); | ||
| } | ||
|
|
||
| return process; | ||
| }; | ||
|
|
||
| using (ProfileExecutor executor = TestDependencies.CreateProfileExecutor(profile, this.mockFixture.Dependencies)) | ||
| { | ||
| await executor.ExecuteAsync(ProfileTiming.OneIteration(), CancellationToken.None).ConfigureAwait(false); | ||
|
|
||
| // Verify numactl was used for CPU affinity | ||
| WorkloadAssert.CommandsExecuted(this.mockFixture, "\"numactl -C"); |
There was a problem hiding this comment.
The test assertions are too weak to catch the incorrect command construction. The test only checks that "bash" is in the command and "numactl" is in the arguments (line 95), and that "numactl -C" appears somewhere (line 109).
The test should verify the exact command structure. For the correct implementation, it should verify something like:
- command is "/bin/bash"
- arguments match the pattern:
-c "numactl -C 0-3 /path/to/ExampleWorkload Workload --duration=00:00:30"
This would catch the bug in lines 245-247 where GetCommandWithAffinity is called incorrectly.
This PR enables CPU affinity support for Virtual Client process on windows, by setting affinity via Process.ProcessorAffinity API.
Why Different Approaches for Windows vs. Linux?
-On Linux, we use numactl to wrap the process execution command.
-Windows doesn't have a built-in numactl equivalent, the Process.ProcessorAffinity property is the standard .NET approach which leverages the existing process management infrastructure.
process.Start();
process.ProcessorAffinity = affinityMask;
Limitations for Windows:
Set thread group + CPU mask > Create thread in a group > Query group topology > Multi‑group thread affinity