Use environment variable for handshake Resolves #4961 #5196

Forgind · 2020-03-24T21:53:14Z

Adds use of an environment variable for non-task host nodes as well. Some duplicated logic should ideally be refactored pending acceptance of an altered handshake.

Fixes #4961.

cdmihai · 2020-03-27T03:35:52Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs

-            long baseHandshake = Constants.AssemblyTimestamp;
+            CommunicationsUtilities.Trace("MSBUILDNODEHANDSHAKESALT=\"{0}\", msbuildDirectory=\"{1}\", enableNodeReuse={2}, enableLowPriority={3}", Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"), BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32, enableNodeReuse, enableLowPriority);
+
+            string salt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT") + BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32;


There's also a BuildEnvironment.MSBuildToolsDirectory64. Shouldn't the handshake reflect that bitness as well?

cdmihai · 2020-03-27T03:41:51Z

It would sure be nice if there would one single place that computes the handshake, like a NodeHandshakeBuilder. Also, is that salt and tools directory really useful? Could the unified handshake not contain them? Who sets MSBUILDNODEHANDSHAKESALT? Searched the VS codebase and also googled it and didn't find anyone who sets it.

For HandshakeOptions

rainersigwald

Have to step away, incomplete review.

rainersigwald · 2020-04-20T16:46:23Z

src/Shared/CommunicationsUtilities.cs

 namespace Microsoft.Build.Internal
 {
    /// <summary>
    /// Enumeration of all possible (currently supported) types of task host context.


Update comment for new purpose please

rainersigwald · 2020-04-20T16:50:52Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs

-            baseHandshake = baseHandshake*17 + enableLowPriority.GetHashCode();
-
-            return CommunicationsUtilities.GenerateHostHandshakeFromBase(baseHandshake, GetClientHandshake());
+            CommunicationsUtilities.Trace("MSBUILDNODEHANDSHAKESALT=\"{0}\", msbuildDirectory=\"{1}\", enableNodeReuse={2}, enableLowPriority={3}", Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"), BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32, enableNodeReuse, enableLowPriority);


I wish we didn't have a live environment-variable read here; they're sometimes surprisingly expensive. Can it be moved to a static, maybe in Traits? Then it only needs to be read once.

rainersigwald · 2020-04-20T16:54:40Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs

+        internal static string GetTaskHostNameFromHostContext(HandshakeOptions hostContext)
        {
-            if (hostContext == TaskHostContext.X64CLR4 || hostContext == TaskHostContext.X32CLR4)
+            ErrorUtilities.ThrowInternalErrorUnreachable((hostContext & HandshakeOptions.TaskHost) == HandshakeOptions.TaskHost);


I don't understand this line. Throw if we get to this point with a task host? But below we have cases for task hosts.

In general I don't think I like the conditional ThrowInternalErrorUnreachable. I find throwing in a case or else-if clearer.

If the condition is false, it throws, so this throws if it is not a task host that is passed in. I modeled it after cases like ErrorUtilities.VerifyThrow(bool condition, string unformattedMessage) where the first line of the body is if (!condition).

If I wanted to do it in a switch/case of if/else, I'd either have to start with checking whether it's a task host (i.e., this) or put it at the end with every condition before it verifying that it's a task host. Since it's an error condition rather than an expected case, I think separating it somehow is better, and this is cleaner than adding a condition to every other case.

Suggested change

ErrorUtilities.ThrowInternalErrorUnreachable((hostContext & HandshakeOptions.TaskHost) == HandshakeOptions.TaskHost);

ErrorUtilities.VerifyThrowInternalErrorUnreachable((hostContext & HandshakeOptions.TaskHost) == HandshakeOptions.TaskHost);

cdmihai · 2020-04-22T01:05:24Z

src/Shared/CommunicationsUtilities.cs

        /// Given the appropriate information, return the equivalent TaskHostContext.  
        /// </summary>
-        internal static TaskHostContext GetTaskHostContext(IDictionary<string, string> taskHostParameters)
+        internal static HandshakeOptions GetTaskHostContext(IDictionary<string, string> taskHostParameters)


Rename to GetHandshakeOptions to keep in line with the new enum name? Same for similarly named handshake contructors.

Actually, wouldn't it be cleaner if you moved all the GetTaskHostContext methods as static constructors on the enum instead, and rename to From: HandshakeOptions.From(bool, bool)

In reply to: 412594545 [](ancestors = 412594545)

Can an enum have a method like a class? I think I'd have to replace the enum with a class, and that would make it much bigger.

You can write extension methods for it: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/how-to-create-a-new-method-for-an-enumeration

I put a change here, but I don't think I like it. It would probably be good if there were a lot of classes that were going to touch HandshakeOptions, but since all references to it are focused in this class, it seems unnecessary.

cdmihai · 2020-04-22T01:12:29Z

src/Shared/CommunicationsUtilities.cs

-        /// Client handshake required for comparison purposes only.  Returns the update handshake.  
        /// </summary>
-        internal static long GenerateHostHandshakeFromBase(long baseHandshake, long clientHandshake)
+        internal static long GenerateHostHandshakeFromBase(long baseHandshake)


Make private or inline, no need to expose it anymore.

rainersigwald · 2020-04-24T15:51:04Z

src/Shared/Traits.cs

+        /// <summary>
+        /// Allow the user to specify that two processes should not be communicating via an environment variable.
+        /// </summary>
+        public static readonly int MSBuildNodeHandshakeSalt = ParseIntFromEnvironmentVariableOrDefault("MSBUILDNODEHANDSHAKESALT", -1);


Suggested change

public static readonly int MSBuildNodeHandshakeSalt = ParseIntFromEnvironmentVariableOrDefault("MSBUILDNODEHANDSHAKESALT", -1);

public static readonly string MSBuildNodeHandshakeSalt = ParseIntFromEnvironmentVariableOrDefault("MSBUILDNODEHANDSHAKESALT", -1);

* LOC CHECKIN | microsoft/msbuild vs16.6 | 20200420 (#5299) * Final branding_16.6 (#5273) * merge * Enable handshake timeout on Mono (#5318) I am seeing consistent test hangs on macOS Mono. A node is getting into a bad state and is failing to respond to handshakes. While I do not yet understand the root cause, it is clear that having a timeout on the handshake operation mitigates the issue. The test is now failing but does not hang, saving developer time as well as test pipeline resources. * Revert "Reverted and rebuilt for localizable strings." (#5246) This adds back support for logging an error when a task returns false without logging an error. This was originally added in #4940 but was reverted because of multiple difficulties. * Changed error code * Add escape hatch * Fix typo * Filtering for duplicate content files with referencing projects (#4931) * Updating content filtering based on content copying changes * Add a flag that is enabled by default on Core; otherwise disabled by default. * Adding Shutdown Message to Worker Nodes (#5262) * Changed where Trace is being called and removed old functionality. * Updating .NET Core branding to .NET (#5286) * Override default Arcade Xunit configuration (#5302) * Update Directory.Build.targets * prevent arcade from injecting its own xunit file * Don't log @(ReferencePath) at the end of ImplicitlyExpandDesignTimeFacades (#5317) The actual ItemGroups inside the target already do a good job of logging exactly what items were added and/or removed and in what order and with what metadata. Emitting an extra low-pri message which is unstructured here just adds noise, slows the builds down, wastes binlog space and is otherwise redundant. * Compute hashes in parallel in GetFileHash (#5303) * Compute hashes in parallel. This scales better for larger number of files. * Use a dedicated write lock * Allow disabling logging of task parameters and item metadata (#5268) This enables fine-grained control over whether: * to log log each parameter (whether input or output) * or whether to log item metadata for each ITaskItem[] parameter. When LogTaskInputs is set the default behavior is still to log all parameters and all item metadata for ITaskItem[] parameters. Since this is very verbose and hurts performance without adding any useful information it is valuable to be able to turn this logging off in certain situations. This approach allows controlling logging via setting simple properties or environment variables. I've identified the specific tasks and parameters that we want to restrict logging for that would give us the most gains without losing any significant useful info: https://github.com/KirillOsenkov/MSBuildStructuredLog/wiki/Task-Parameter-Logging * Update dependencies from https://github.com/dotnet/arcade build 20200430.5 (#5325) - Microsoft.DotNet.Arcade.Sdk: 1.0.0-beta.20221.2 -> 1.0.0-beta.20230.5 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Use environment variable for handshake Resolves #4961 (#5196) * Use environment variable for handshake Resolves #4961 * Combine means of hashing * Support transitive project references in static graph (#5326) Transitive project references are a thing now. Added support to static graph, so that buildxl and qb can avoid adding the transitive refs. This PR is independent of #5222. Ideally, review this one first, as QB has a stronger dependency on this PR than on #5222. Design - transitive references are opt-in, per project evaluation - once a project opts-in, transitivity is applied for all ProjectReference items - a project opt-ins by setting the property AddTransitiveProjectReferencesInStaticGraph to true. The sdk does this automatically in Microsoft.Managed.After.Targets. - interaction with crosstargeting: transitive refs are added only to inner builds, not the outer builds. This mimics vanilla msbuild. Co-authored-by: Rainer Sigwald <[email protected]> Co-authored-by: Martin Chromecek (Moravia IT) <[email protected]> Co-authored-by: Ladi Prosek <[email protected]> Co-authored-by: Sarah Oslund <[email protected]> Co-authored-by: Ben Villalobos <[email protected]> Co-authored-by: Mihai Codoban <[email protected]> Co-authored-by: Kirill Osenkov <[email protected]> Co-authored-by: Pranav K <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Rainer Sigwald <[email protected]>

Use environment variable for handshake

6e5fe4c

cdmihai reviewed Mar 27, 2020

View reviewed changes

Forgind changed the title ~~Use environment variable for handshake Resolves #4961~~ (WIP) Use environment variable for handshake Resolves #4961 Mar 30, 2020

Forgind added 3 commits March 30, 2020 15:09

Changes to use different name of enum

9286a72

Combine means of hashing

019af5e

Take into account 64bit tools directory

43395f7

Forgind changed the title ~~(WIP) Use environment variable for handshake Resolves #4961~~ Use environment variable for handshake Resolves #4961 Mar 30, 2020

Forgind added 2 commits April 13, 2020 12:45

Switched to flags enum

d276f95

For HandshakeOptions

Fix flag enum

7f1b4d7

rainersigwald reviewed Apr 20, 2020

View reviewed changes

cdmihai reviewed Apr 22, 2020

View reviewed changes

PR comments

0f57a29

rainersigwald reviewed Apr 24, 2020

View reviewed changes

PR comments

098f624

rainersigwald approved these changes May 1, 2020

View reviewed changes

Forgind merged commit fa3df44 into dotnet:master May 1, 2020

Forgind deleted the salt-update branch May 1, 2020 20:34

KirillOsenkov mentioned this pull request May 27, 2020

64-bit node processes load MSBuild binaries from 32-bit location #5375

Closed

Forgind mentioned this pull request Jun 17, 2020

ItemTransformContainingSemicolon_InTaskHost test failure #5423

Closed

	ErrorUtilities.ThrowInternalErrorUnreachable((hostContext & HandshakeOptions.TaskHost) == HandshakeOptions.TaskHost);
	ErrorUtilities.VerifyThrowInternalErrorUnreachable((hostContext & HandshakeOptions.TaskHost) == HandshakeOptions.TaskHost);

	public static readonly int MSBuildNodeHandshakeSalt = ParseIntFromEnvironmentVariableOrDefault("MSBUILDNODEHANDSHAKESALT", -1);
	public static readonly string MSBuildNodeHandshakeSalt = ParseIntFromEnvironmentVariableOrDefault("MSBUILDNODEHANDSHAKESALT", -1);

Use environment variable for handshake Resolves #4961 #5196

Use environment variable for handshake Resolves #4961 #5196

Uh oh!

Conversation

Forgind commented Mar 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdmihai commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdmihai Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cdmihai commented Mar 27, 2020 •

edited

Loading

cdmihai Apr 22, 2020 •

edited

Loading