-
Notifications
You must be signed in to change notification settings - Fork 429
Fix tracing on windows for the runner, and when using multiple self-hosted actions runners #167
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
Conversation
|
Not sure who is best placed to review this. Possibly @aibaars if you can. I also asked questions to @nickrolfe and @dbartol in case either of you can see anything wrong here. |
|
It think the assumption that the grand-parent of the process is the one that persists between build steps but not between subsequent jobs is reasonable. The process itself will be Of course the assumption might be wrong. The process tree could be slightly deeper for some CI system. I'd recommend adding an "advanced" flag to allow a user to specify some N (default 2) that indicates that the Nth parent should be taken. It's also possible that the depth is variable, although that is less likely. You could also add an additional flag that allows specifying the name of the "worker" process. These two flags generalise the actions case where we search by name for |
I think you're off by one from my assumptions. I think the process itself will be powershell, and then the parent is nodejs, and the grandparent is likely some shell or the CI system, or the parent of that is the CI system process that sticks around. So this sounds like we should increase the default by one level. Do you agree?
I agree with you here. There's no way we can make a guess that will be right in all cases. So giving inputs like the two you mention sould give enough power that we can make it work in almost any situation if we manually work out what to do. This could be annoying to the docs team (cc. @felicitymay) as we're adding new arguments that are quite hard to explain. Since these are meant mainly for manual intervention by a field team member, perhaps we should make them hidden option that don't show up if you run |
Yes, indeed. I forgot about powershell. It would be interesting to print the process trees on Actions, Jenkins, and ideally some other CI systems (TeamCity, Bamboo) for windows workers. This should give you some idea on whether there is a value that would work for most and use that as default. |
I did this on actions at least and the process immediately above the nodejs process is the one that stays around for the length of the job. If you go one level higher you get the process that persists between jobs, so we don't want to touch that one as it would lead to affecting future jobs. |
|
I've pushed code to add With these options I think we should be flexible enough. It would be nice if the default were still a good choice in most cases. I think it'll now work out of the box on anywhere that there's a script in between the CI master process and the runner process. For example on jenkins or actions you specify to run a script, and then that script spawns the runner process. So I think a default of the 3rd parent will work in this case. If the runner is spawned without this intermediate script, then it might go a level too high, but that can now be corrected using the new hidden option. |
|
Unfortunately we ideally want to get this done so we can release tomorrow, or very soon after, to match GHES. Sorry this is happening so close to the deadline and with comparatively little testing work. I hope we can get it out there in time and then make improvements over time and release frequently. |
|
I've tested this on actions, by which I mean calling the runner from a powershell script on actions, and it works there and I'm able to build and analyze a csharp project. I'll try the same thing on jenkins. |
|
I mostly looked at the PowerShell scripts you generate, and they seem reasonable to me. |
|
I had an off-by-one error in my loop, but I've fixed that now. I've tested this on jenkins and actions by running the |
aibaars
left a comment
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.
Code changes look fine to me. Make sure to test as much as you can before the release.
|
I think we're best off merging this as it's definitely better than the current situation of being broken. I'll do more testing this afternoon, including adding more actions workflows that exercise the runner. |
This is a draft at getting windows tracing working outside of actions, and make it more robust when using self-hosted actions runners.
I'm in the process of testing this, and I'll push any changes that are necessary.
Merge / deployment checklist