out of proc agent system plugins #1537

TingluoHuang · 2018-04-27T16:05:01Z

No description provided.

ericsciple · 2018-05-02T02:00:05Z

src/Agent.Worker/JobRunner.cs

@@ -41,6 +41,87 @@ public sealed class JobRunner : AgentService, IJobRunner

            DateTime jobStartTimeUtc = DateTime.UtcNow;

+            var tfsGit1 = new Pipelines.RepositoryResource()


this is for me to test my change, since the server haven't sent down repository as part of job message.

ericsciple · 2018-05-02T03:48:07Z

src/dir.proj

+    <RemoveDir  Directories="Agent.PluginCore/obj" />
+    <RemoveDir  Directories="Agent.RepositoryPlugin/bin" />
+    <RemoveDir  Directories="Agent.RepositoryPlugin/obj" />
+    <RemoveDir  Directories="Agent.DropPlugin/bin" />


would it be a worthy reduction in overhead to just have two DLLs? PluginHost and Plugins? and let authors namespace their code.

We will quickly hit an issue where folks want shared DLLs for shared logic. That problem goes away if all plugsin are in one DLL. And note, everything is in one DLL today in the worker project.

agent.pluginhost.exe
agent.pluginsdk.dll
agent.plugins.dll <- microsoft.vs.service.agent.plugins.repo/drop/etc...

ericsciple · 2018-05-02T17:28:44Z

src/Agent.PluginCore/CommandPlugin.cs

+
+        public void Output(string message)
+        {
+            Console.WriteLine(message);


is a lock needed here? will lines get mixed from separate threads?

base on doc, console.write is thread safe.

ericsciple · 2018-05-02T17:31:29Z

src/Agent.PluginCore/CommandPlugin.cs

+            Console.WriteLine(message);
+        }
+
+        public void Fail(string message)


STDERR from while process ##command means the command process failed.

ericsciple · 2018-05-02T17:34:22Z

src/Agent.PluginCore/CommandPlugin.cs

+            }
+        }
+
+        private AgentCertificateSettings GetCertConfiguration()


is this consistent with how the libs read the settings? or how the agent reads the settings?

this is how the libs read proxy/cert settings.

ericsciple · 2018-05-02T17:34:42Z

src/Agent.PluginCore/CommandPlugin.cs

+            }
+        }
+
+        private AgentWebProxySettings GetProxyConfiguration()


this is consistent with how the libs read the settings, correct?

yes, same as node/powershell lib, read setting from variables set by the agent..

ericsciple · 2018-05-02T17:38:39Z

src/Agent.PluginCore/PluginUtil.cs

+            return string.IsNullOrEmpty(ProxyAddress) || uri.IsLoopback || IsMatchInBypassList(uri);
+        }
+
+        private bool IsMatchInBypassList(Uri input)


is this bypass stuff intentionally duplicated in commandPlugin.cs?

yes, the one in commandplugin.cs is part of the IWebProxy implementation.

put the calculation part into pluginutil.cs

ericsciple · 2018-05-02T17:54:51Z

src/Agent.PluginCore/PluginUtil.cs

+            DeleteDirectory(path, contentsOnly: false, continueOnContentDeleteError: false, cancellationToken: cancellationToken);
+        }
+
+        public static void DeleteDirectory(string path, bool contentsOnly, bool continueOnContentDeleteError, CancellationToken cancellationToken)


consider moving this to shared DLL so we don't have to maintain a fork of the code. perhaps something to clean up in a following pr

I agree, we need to figure out a good name.

agent.listener.exe
agent.worker.exe
microsoft.vs.service.agent.dll
agent.pluginhost.exe
agent,plugins.dll
agent.sdk.dll

ericsciple · 2018-05-02T17:56:19Z

src/Agent.PluginCore/PluginUtil.cs

+            return path + Path.PathSeparator + currentPath;
+        }
+
+        public static void PrependPath(string directory)


doesnt the git source provider do this? is it an instruction for the agent to prepend path now?

(for the benefit of downstream tasks)

i guess PluginSteps can write commands, commands cannot write commands

yes, you are right! i will change it to write ##vso[task.prepandpath]

ericsciple · 2018-05-02T17:57:47Z

src/Agent.PluginCore/PluginUtil.cs

+            }
+        }
+
+        private async Task CancelAndKillProcessTree()


i'm curious how cancellation works when initiated from the agent. let's discuss offline

decrease cancel timeout 5 secs.

ericsciple · 2018-05-02T17:59:21Z

src/Agent.PluginCore/TaskPlugin.cs

+            Console.WriteLine($"##vso[task.debug]{message}");
+        }
+
+        public void Error(string message)


do we ever use this? i would just remove this overload and rename fail to error. we can always add an overload later.

error will output ##vso[task.setresult result=failed;] by default

ericsciple · 2018-05-02T18:00:29Z

src/Agent.PluginCore/TaskPlugin.cs

+        public Dictionary<string, VariableValue> TaskVariables { get; set; }
+        public Dictionary<string, string> Inputs { get; set; }
+
+        public string GetInput(string name, bool required = false)


we need a vault concept. we can port from powershell, but i'm not sure if it will work on linux.

i forgot that. it should work, we can just use AES.

ericsciple · 2018-05-02T18:14:49Z

src/Agent.PluginHost/Program.cs

+                    string assemblyQualifiedName = args[1];
+                    PluginUtil.NotNullOrEmpty(assemblyQualifiedName, nameof(assemblyQualifiedName));
+
+                    string serializedContext = Console.ReadLine();


is there a limitation to line size?

i tried a input with 16K chars, doesn't seems to have limitation

ericsciple · 2018-05-02T18:16:07Z

src/Agent.PluginHost/Program.cs

+                    AgentTaskPluginExecutionContext executionContext = PluginUtil.ConvertFromJson<AgentTaskPluginExecutionContext>(serializedContext);
+                    PluginUtil.NotNull(executionContext, nameof(executionContext));
+
+                    AssemblyLoadContext.Default.Resolving += ResolveAssembly;


are we going to have assembly resolver problems like the functions in the ps lib solve?

ericsciple · 2018-05-02T18:24:13Z

src/Agent.RepositoryPlugin/GitCommandManager.cs

+
+namespace Agent.RepositoryPlugin
+{
+    public class GitCommandManager


Now the "Command" terminology is misleading

GitCLIManager?

GitCliManager

ericsciple · 2018-05-02T18:24:49Z

src/Agent.RepositoryPlugin/GitCommandManager.cs

+
+                // Prepend the PATH.
+                context.Output(PluginUtil.Loc("Prepending0WithDirectoryContaining1", "Path", Path.GetFileName(gitPath)));
+                PluginUtil.PrependPath(Path.GetDirectoryName(gitPath));


Shouldnt this echo a ##command?

ericsciple · 2018-05-02T18:32:29Z

src/Agent.RepositoryPlugin/RepositoryPlugin.cs

+                    throw new NotSupportedException(repo.Type);
+            }
+
+            if (executionContext.Stage == "main")


this is another place a shared dll would help, so these loose strings could be moved into a shared const

agent.sdk.dll

ericsciple · 2018-05-02T18:38:51Z

src/Agent.RepositoryPlugin/RepositoryPlugin.cs

+            }
+        }
+
+        private void MergeInputs(AgentTaskPluginExecutionContext executionContext, Pipelines.RepositoryResource repository)


lets discuss offline. i'm confused why this is here

i will deal with this later when we send repository as part of the message.

ericsciple · 2018-05-02T21:18:46Z

src/Agent.Worker/ExecutionContext.cs

@@ -439,6 +446,15 @@ public void InitializeJob(Pipelines.AgentJobRequestMessage message, Cancellation
                }
            }

+            // Runtime option variables


what is this for? why is this here?

the git source provide needs to know whether to use SChanel or OpenSSl as secure backend

k, lets chat offline. i'm still confused why the variable is set here

ericsciple · 2018-05-02T21:21:27Z

src/Agent.Worker/AgentPluginManager.cs

+            }
+        };
+
+        public Dictionary<string, Dictionary<string, AgentCommandPluginInfo>> SupportedLoggingCommands => _supportedLoggingCommands;


Rather than expose these dictionaries, a nicer interface might be something like GetCommand() and return null if not found

GetTask(guid, version)
GetCommnad(String, string)

ericsciple · 2018-05-02T21:39:17Z

src/Agent.Worker/AgentPluginManager.cs

+
+            // Resolve the working directory.
+            string workingDirectory = HostContext.GetDirectory(WellKnownDirectory.Bin);
+            ArgUtil.Directory(workingDirectory, nameof(workingDirectory));


why not system.defaultWorkingDirectory?

seems fine.

better be workfolder. :D

ericsciple · 2018-05-02T21:42:24Z

src/Agent.Worker/AgentPluginManager.cs

+                _supportedTasks[taskPlugin.Id][taskPlugin.Version].Data = new DefinitionData()
+                {
+                    Author = taskPlugin.Author,
+                    Description = taskPlugin.Description,


what about instance name format? who calculates the instance name format? is it the agent or the server? is it based on the instance name format defined in the task instance or in the task definition? have you thought through how this will work for plugin steps?

i assume server will do it since that how other task been handled as well.

ericsciple · 2018-05-02T21:53:13Z

src/Agent.Worker/AgentPluginManager.cs

+                Inputs = inputs,
+                Stage = stage,
+                Repositories = context.Repositories,
+                Endpoints = context.Endpoints


is the host portion of the urls in the endpoints fixed up already? what about repositories, will they need to be fixed up too?

everything in executioncontext should already be fixed up.

in jobrunner, fix url for repository as well.

TingluoHuang · 2018-05-03T18:51:56Z

I will remove duplicated code between agent.sdk.dll and microsoft.vs.service.agent.dll in next PR

bryanmacfarlane changed the title ~~agent plugins.~~ out of proc agent system plugins Apr 30, 2018

TingluoHuang force-pushed the users/tihuang/agentplugin branch from f2fccd8 to 6103ca7 Compare May 1, 2018 19:15

ericsciple reviewed May 2, 2018

View reviewed changes

TingluoHuang force-pushed the users/tihuang/agentplugin branch 4 times, most recently from 60aa0f9 to e1c3d10 Compare May 4, 2018 14:48

TingluoHuang force-pushed the users/tihuang/agentplugin branch 2 times, most recently from c8be8ea to b3bc6bb Compare May 4, 2018 17:22

Out of proc agent system plugins.

dedfd9b

TingluoHuang force-pushed the users/tihuang/agentplugin branch from b3bc6bb to dedfd9b Compare May 4, 2018 18:15

TingluoHuang merged commit 4d8b211 into master May 4, 2018

TingluoHuang deleted the users/tihuang/agentplugin branch May 4, 2018 18:44

		@@ -41,6 +41,87 @@ public sealed class JobRunner : AgentService, IJobRunner

		DateTime jobStartTimeUtc = DateTime.UtcNow;

		var tfsGit1 = new Pipelines.RepositoryResource()

out of proc agent system plugins #1537

out of proc agent system plugins #1537

Conversation

TingluoHuang commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsciple May 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TingluoHuang commented May 3, 2018

ericsciple May 2, 2018 •

edited