Skip to content

Scheduler should honor BuildParameters.DisableInprocNode #6400

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

Merged
Forgind merged 6 commits into from
Jun 4, 2021
Merged

Scheduler should honor BuildParameters.DisableInprocNode #6400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
04655dd
Scheduler should honor BuildParameters.ForceAffinityOutOfProc
cdmihai Apr 30, 2021
d427fac
Do TaskHost too
cdmihai May 5, 2021
56f438f
Put changes behind change wave
cdmihai May 5, 2021
e3503c0
fix syntax error
cdmihai Jun 2, 2021
a61480c
remove useless VerifyThrow
cdmihai Jun 2, 2021
da2aa1e
re-enable tests
cdmihai Jun 4, 2021
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
11 changes: 6 additions & 5 deletions src/Build.UnitTests/BackEnd/BuildManager_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2133,10 +2133,10 @@ public void Regress239661()
}

/// <summary>
/// Verify that disabling the in-proc node when a project requires it will cause the build to fail, but not crash.
/// Verify that disabling the in-proc node when a project requires it will cause the project to build on the out of proc node.
/// </summary>
[Fact]
public void Regress239661_NodeUnavailable()
public void ExplicitInprocAffinityGetsOverruledByDisableInprocNode()
{
string contents = CleanupFileContents(@"
<Project xmlns='msbuildnamespace' ToolsVersion='msbuilddefaulttoolsversion'>
Expand All @@ -2151,14 +2151,15 @@ public void Regress239661_NodeUnavailable()
</Project>
");
BuildRequestData data = GetBuildRequestData(contents);
_env.CreateFile(data.ProjectFullPath, data.ProjectInstance.ToProjectRootElement().RawXml);
_parameters.DisableInProcNode = true;

// Require that this project build on the in-proc node, which will not be available.
data.HostServices.SetNodeAffinity(data.ProjectFullPath, NodeAffinity.InProc);
BuildResult result = _buildManager.Build(_parameters, data);
Assert.Equal(BuildResultCode.Failure, result.OverallResult);
_logger.AssertLogDoesntContain("[success]");
_logger.AssertLogContains("MSB4223");
Assert.Equal(BuildResultCode.Success, result.OverallResult);
_logger.AssertLogContains("[success]");
_logger.AssertLogDoesntContain("MSB4223");
}

/// <summary>
Expand Down
8 changes: 8 additions & 0 deletions src/Build.UnitTests/BackEnd/Scheduler_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,14 @@ public void TestMaxNodeCountNodesNotExceededWithSomeOOPRequests2()
Assert.Equal(2, response[1].NumberOfNodesToCreate);
}

[Fact]
public void SchedulerShouldHonorDisableInprocNode()
{
var s = new Scheduler();
s.InitializeComponent(new MockHost(new BuildParameters {DisableInProcNode = true}));
s.ForceAffinityOutOfProc.ShouldBeTrue();
}

/// <summary>
/// Make sure that traversal projects are marked with an affinity of "InProc", which means that
/// even if multiple are available, we should still only have the single inproc node.
Expand Down
39 changes: 0 additions & 39 deletions src/Build.UnitTests/ProjectCache/ProjectCacheTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,15 +439,6 @@ public void ProjectCacheByBuildParametersAndGraphBuildWorks(GraphCacheResponse t

var graphResult = buildSession.BuildGraph(graph);

if (buildParameters.DisableInProcNode
&& testData.NonCacheMissResults.Any(c => c.Value.ProxyTargets is not null))
{
// TODO: remove this branch when the DisableInProcNode failure is fixed by https://github.com/dotnet/msbuild/pull/6400
graphResult.OverallResult.ShouldBe(BuildResultCode.Failure);
buildSession.Logger.Errors.First().Code.ShouldBe("MSB4223");
return;
}

graphResult.OverallResult.ShouldBe(BuildResultCode.Success);

buildSession.Dispose();
Expand Down Expand Up @@ -478,16 +469,6 @@ public void ProjectCacheByBuildParametersAndBottomUpBuildWorks(GraphCacheRespons
{
var buildResult = buildSession.BuildProjectFile(node.ProjectInstance.FullPath);

if (buildParameters.DisableInProcNode &&
testData.NonCacheMissResults.TryGetValue(GetProjectNumber(node), out var cacheResult) &&
cacheResult.ProxyTargets is not null)
{
// TODO: remove this branch when the DisableInProcNode failure is fixed by https://github.com/dotnet/msbuild/pull/6400
buildResult.OverallResult.ShouldBe(BuildResultCode.Failure);
buildSession.Logger.Errors.First().Code.ShouldBe("MSB4223");
return;
}

buildResult.OverallResult.ShouldBe(BuildResultCode.Success);

nodesToBuildResults[node] = buildResult;
Expand Down Expand Up @@ -659,14 +640,6 @@ public void RunningProxyBuildsOnOutOfProcNodesShouldIssueWarning(bool disableInp

var graphResult = buildSession.BuildGraph(graph);

if (!disableInprocNodeViaEnvironmentVariable)
{
// TODO: remove this branch when the DisableInProcNode failure is fixed by https://github.com/dotnet/msbuild/pull/6400
graphResult.OverallResult.ShouldBe(BuildResultCode.Failure);
buildSession.Logger.Errors.First().Code.ShouldBe("MSB4223");
return;
}

graphResult.OverallResult.ShouldBe(BuildResultCode.Success);

buildSession.Dispose();
Expand Down Expand Up @@ -1268,12 +1241,6 @@ public void EndBuildShouldGetCalledOnceWhenItThrowsExceptionsFromGraphBuilds()
[InlineData(true, true)]
public void CacheShouldBeQueriedInParallelDuringGraphBuilds(bool useSynchronousLogging, bool disableInprocNode)
{
if (disableInprocNode)
{
// TODO: remove this branch when the DisableInProcNode failure is fixed by https://github.com/dotnet/msbuild/pull/6400
return;
}

var referenceNumbers = new []{2, 3, 4};

var testData = new GraphCacheResponse(
Expand Down Expand Up @@ -1354,12 +1321,6 @@ Task<BuildResult> BuildProjectFileAsync(int projectNumber)
[InlineData(true, true)]
public void ParallelStressTest(bool useSynchronousLogging, bool disableInprocNode)
{
if (disableInprocNode)
{
// TODO: remove this branch when the DisableInProcNode failure is fixed by https://github.com/dotnet/msbuild/pull/6400
return;
}

var referenceNumbers = Enumerable.Range(2, NativeMethodsShared.GetLogicalCoreCount() * 2).ToArray();

var testData = new GraphCacheResponse(
Expand Down
12 changes: 9 additions & 3 deletions src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
using Microsoft.Build.BackEnd.Components.Caching;
using System.Reflection;
using Microsoft.Build.Eventing;
using Microsoft.Build.Utilities;

namespace Microsoft.Build.BackEnd
{
Expand All @@ -38,7 +39,7 @@ internal class TaskHost :
/// <summary>
/// True if the "secret" environment variable MSBUILDNOINPROCNODE is set.
/// </summary>
private static bool s_onlyUseOutOfProcNodes = Environment.GetEnvironmentVariable("MSBUILDNOINPROCNODE") == "1";
private static bool s_disableInprocNodeByEnvironmentVariable = Environment.GetEnvironmentVariable("MSBUILDNOINPROCNODE") == "1";

/// <summary>
/// Help diagnose tasks that log after they return.
Expand Down Expand Up @@ -104,6 +105,8 @@ internal class TaskHost :
/// </summary>
private int _yieldThreadId = -1;

private bool _disableInprocNode;

/// <summary>
/// Constructor
/// </summary>
Expand All @@ -123,7 +126,10 @@ public TaskHost(IBuildComponentHost host, BuildRequestEntry requestEntry, Elemen
_targetBuilderCallback = targetBuilderCallback;
_continueOnError = false;
_activeProxy = true;
_callbackMonitor = new Object();
_callbackMonitor = new object();
_disableInprocNode = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_0)
? s_disableInprocNodeByEnvironmentVariable || host.BuildParameters.DisableInProcNode
: s_disableInprocNodeByEnvironmentVariable;
}

/// <summary>
Expand All @@ -137,7 +143,7 @@ public bool IsRunningMultipleNodes
get
{
VerifyActiveProxy();
return _host.BuildParameters.MaxNodeCount > 1 || s_onlyUseOutOfProcNodes;
return _host.BuildParameters.MaxNodeCount > 1 || _disableInprocNode;
}
}

Expand Down
19 changes: 8 additions & 11 deletions src/Build/BackEnd/Components/Scheduler/Scheduler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
using Microsoft.Build.Experimental.ProjectCache;
using Microsoft.Build.Framework;
using Microsoft.Build.Shared;

using Microsoft.Build.Utilities;
using BuildAbortedException = Microsoft.Build.Exceptions.BuildAbortedException;
using ILoggingService = Microsoft.Build.BackEnd.Logging.ILoggingService;
using NodeLoggingContext = Microsoft.Build.BackEnd.Logging.NodeLoggingContext;
Expand Down Expand Up @@ -141,7 +141,7 @@ internal class Scheduler : IScheduler
/// <summary>
/// Flag used for debugging by forcing all scheduling to go out-of-proc.
/// </summary>
private bool _forceAffinityOutOfProc;
internal bool ForceAffinityOutOfProc { get; private set; }

/// <summary>
/// The path into which debug files will be written.
Expand Down Expand Up @@ -177,7 +177,6 @@ internal class Scheduler : IScheduler
public Scheduler()
{
_debugDumpState = Environment.GetEnvironmentVariable("MSBUILDDEBUGSCHEDULER") == "1";
_forceAffinityOutOfProc = Environment.GetEnvironmentVariable("MSBUILDNOINPROCNODE") == "1";
_debugDumpPath = Environment.GetEnvironmentVariable("MSBUILDDEBUGPATH");
_schedulingUnlimitedVariable = Environment.GetEnvironmentVariable("MSBUILDSCHEDULINGUNLIMITED");
_nodeLimitOffset = 0;
Expand Down Expand Up @@ -616,6 +615,10 @@ public void InitializeComponent(IBuildComponentHost host)
_resultsCache = (IResultsCache)_componentHost.GetComponent(BuildComponentType.ResultsCache);
_configCache = (IConfigCache)_componentHost.GetComponent(BuildComponentType.ConfigCache);
_inprocNodeContext = new NodeLoggingContext(_componentHost.LoggingService, InProcNodeId, true);
var inprocNodeDisabledViaEnvironmentVariable = Environment.GetEnvironmentVariable("MSBUILDNOINPROCNODE") == "1";
ForceAffinityOutOfProc = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_0)
? inprocNodeDisabledViaEnvironmentVariable || _componentHost.BuildParameters.DisableInProcNode
: inprocNodeDisabledViaEnvironmentVariable;
}

/// <summary>
Expand Down Expand Up @@ -1355,12 +1358,6 @@ private void AssignUnscheduledRequestToNode(SchedulableRequest request, int node
ErrorUtilities.VerifyThrowArgumentNull(responses, nameof(responses));
ErrorUtilities.VerifyThrow(nodeId != InvalidNodeId, "Invalid node id specified.");

// Currently we cannot move certain kinds of traversals (notably solution metaprojects) to other nodes because
// they only have a ProjectInstance representation, and besides these kinds of projects build very quickly
// and produce more references (more work to do.) This just verifies we do not attempt to send a traversal to
// an out-of-proc node because doing so is inefficient and presently will cause the engine to fail on the remote
// node because these projects cannot be found.
ErrorUtilities.VerifyThrow(nodeId == InProcNodeId || _forceAffinityOutOfProc || !IsTraversalRequest(request.BuildRequest), "Can't assign traversal request to out-of-proc node!");
request.VerifyState(SchedulableRequestState.Unscheduled);

// Determine if this node has seen our configuration before. If not, we must send it along with this request.
Expand Down Expand Up @@ -1388,7 +1385,7 @@ void WarnWhenProxyBuildsGetScheduledOnOutOfProcNode()
if (request.IsProxyBuildRequest() && nodeId != InProcNodeId)
{
ErrorUtilities.VerifyThrow(
_componentHost.BuildParameters.DisableInProcNode || _forceAffinityOutOfProc,
_componentHost.BuildParameters.DisableInProcNode || ForceAffinityOutOfProc,
"Proxy requests should only get scheduled to out of proc nodes when the inproc node is disabled");

var loggedWarnings = Interlocked.CompareExchange(ref _loggedWarningsForProxyBuildsOnOutOfProcNodes, 1, 0);
Expand Down Expand Up @@ -2097,7 +2094,7 @@ private int ComputeClosureOfWaitingRequests(SchedulableRequest request)
/// </summary>
private NodeAffinity GetNodeAffinityForRequest(BuildRequest request)
{
if (_forceAffinityOutOfProc)
if (ForceAffinityOutOfProc)
{
return NodeAffinity.OutOfProc;
}
Expand Down