Implement new runner service name convention #193

juliobbv · 2019-11-11T16:23:09Z

Adopt new runner service name convention:

Linux/systemd: actions.runner.[org-repo|org].<runnername>.service
OSX/Darwin: actions.runner.[org-repo|org].<runnername>
Windows: actions.runner.[org-repo|org].<runnername>

Additionally, because [org-repo|org] and <runnername> can be long enough to create a service name that's longer than 80 characters, this PR adds code that sensibly trims each part to make everything fit inside 80 chars.

Fixes #187, #191

TingluoHuang · 2019-11-11T16:28:20Z

I thought we had this in AZP, did we miss it when we fork the repo?

thboop · 2019-11-11T16:32:53Z

I thought we had this in AZP, did we miss it when we fork the repo?

Yeah, currently we are limited to 7 characters for the name, which is really rough UX

juliobbv · 2019-11-11T16:32:55Z

Correct @TingluoHuang, the code exists in AZP, but we missed when we forked the repo. So this is a straight re-addition.

https://github.com/microsoft/azure-pipelines-agent/blob/master/src/Agent.Listener/Configuration/ServiceControlManager.cs

thboop · 2019-11-11T16:34:46Z

Do we want to fix #187, if so we could do it here, if not we should mark as invalid.

Keep in mind if we fix #187 we will have to address the case of checking the old service name when configuring/removing

thboop · 2019-11-11T16:35:26Z

Can we also fix the issue where the service says it is starting when it actually is not, otherwise can we go ahead and file another issue for that item

src/Runner.Listener/Configuration/ServiceControlManager.cs

thboop · 2019-11-11T16:42:10Z

Changes look good, lets add some tests!

juliobbv · 2019-11-11T16:42:47Z

I'm good with keeping runner service names consistent. I'll make it actionsrunner across every OS (if nobody objects 😄).

Edit: I'm going to defer making the service rename change with this PR, as it's not a bug and it's not pressing in any sort of way to get it merged.

src/Test/L0/ServiceControlManagerL0.cs

thboop

LGTM clean

bryanmacfarlane · 2019-11-15T18:36:37Z

What does a "generated" service name look like now?

bryanmacfarlane · 2019-11-17T15:46:17Z

^^ I would also like to review the generated name and compare to linux and mac. It would be nice if the docs just said, a service name looks like xxx and it's the same.

juliobbv · 2019-11-18T19:27:56Z

@bryanmacfarlane with my proposed PR changes, generated service names look like this:

<= 80 chars

Linux/systemd: actions.runner.<actionshostname>.<poolname>.<agentname>.service
OSX/Darwin: actions.runner.<actionshostname>.<poolname>.<agentname>
Windows: actionsrunner.<actionshostname>.<poolname>.<agentname>

Example (Windows): actionsrunner.o12j2bQOK6Ovig8NeZLyDbKac9njxpVLdqm77oiLduQePH7pxE.Default.MyAgentName

> 80 chars
Linux/systemd: actions.runner.<first 25 chars of actionshostname>.<first 25 chars of poolname>.<agentname, trimmed to fill in 80 chars name total>.service
OSX/Darwin: actions.runner.<first 25 chars of actionshostname>.<first 25 chars of poolname>.<agentname, trimmed to fill in 80 chars>
Windows: actionsrunner.<first 25 chars of actionshostname>.<first 25 chars of poolname>.<agentname, trimmed to fill in 80 chars>

Example (Windows): actionsrunner.o12j2bQOK6Ovig8NeZLyDbKac.thisisaverylongpoolnamewi.longmachinenam

bryanmacfarlane · 2019-11-18T19:37:18Z

We may want to run this by product because the hash is ugly and pool names are irrelevant in actions.

I think it should just be:

Linux/systemd: actions.runner.[repo|org].<agentname>.service
OSX/Darwin: actions.runner.[repo|org].<agentname>
Windows: actions.runner.[repo|org].<agentname>

The key is we want it to be clear but also avoid conflicts (thus the repo | org). The other option is to just tell folks to use distinct names and drops that. The hash should go. Also, the config file has the url and host so that doesn't need to be in the name

@chrispat @thejoebourneidentity

juliobbv · 2019-11-20T19:52:57Z

Hey @bryanmacfarlane, are we good merging this PR? My focus was on fixing the >80 chars bug to keep the changes well-scoped.

We can discuss/implement the proposed naming changes with a separate issue/PR. What do you think?

juliobbv · 2019-11-22T20:42:59Z

Spoke with @bryanmacfarlane and @thejoebourneidentity. We're going to re-purpose this PR to include the proposed service naming changes by Bryan.

juliobbv · 2019-11-26T16:15:16Z

Hey @lucascosti! FYI, this PR now implements a the service naming convention below:

Linux/systemd: actions.runner.[repo|org].<runnername>.service
OSX/Darwin: actions.runner.[repo|org].<runnername>
Windows: actions.runner.[repo|org].<runnername>

For example: if I'm setting up a service for a Linux/systemd runner that targets the juliobbv/test repo, with the runner name myrunnername, the generated service name will be:
actions.runner.juliobbv-test.myrunnername.service.

src/Runner.Listener/Configuration/ServiceControlManager.cs

thboop · 2019-11-27T15:10:23Z

Hey @lucascosti! FYI, this PR now implements a the service naming convention below:
Linux/systemd: actions.runner.[repo|org].<runnername>.service
OSX/Darwin: actions.runner.[repo|org].<runnername>
Windows: actions.runner.[repo|org].<runnername>
For example: if I'm setting up a service for a Linux/systemd runner that targets the juliobbv/test repo, with the runner name myrunnername, the generated service name will be:
actions.runner.juliobbv-test.myrunnername.service.

We may want to consider doing {Org.Repo | Org} as two repos can have the same name in different orgs.

src/Runner.Listener/Configuration/ServiceControlManager.cs

juliobbv · 2019-11-27T15:50:24Z

We may want to consider doing {Org.Repo | Org} as two repos can have the same name in different orgs.

@thboop The code is currently doing: [Org-Repo | Org]. So e.g. repos example/test and example2/test generate different service names. I updated my PR description to better reflect reality.

src/Runner.Common/ConfigurationStore.cs

thboop

Minor Code thoughts but LGTM

src/Runner.Common/ConfigurationStore.cs

src/Runner.Listener/Configuration/ServiceControlManager.cs

lucascosti · 2019-11-28T23:14:10Z

Hey @lucascosti! FYI, this PR now implements a the service naming convention below:
Linux/systemd: actions.runner.[repo|org].<runnername>.service
OSX/Darwin: actions.runner.[repo|org].<runnername>
Windows: actions.runner.[repo|org].<runnername>
For example: if I'm setting up a service for a Linux/systemd runner that targets the juliobbv/test repo, with the runner name myrunnername, the generated service name will be:
actions.runner.juliobbv-test.myrunnername.service.

Thanks, @juliobbv! I have opened a docs issue for the change: https://github.com/github/help-docs/issues/11694. We are currently frozen for Thanksgiving, but I'll make the docs update early next week. 🚀

lucascosti · 2019-12-02T01:12:24Z

@juliobbv are these changes public? I just tested it, and the Add runner modal currently has instructions for downloading https://githubassets.azureedge.net/runners/2.161.0/actions-runner-win-x64-2.161.0.zip, and that is still creating services with the old naming. Is the runner naming change supposed to be in that runner app version?

I have a docs change ready to go in https://github.com/github/help-docs/pull/11704, but I'll hold off on that until you confirm. 🙂

juliobbv · 2019-12-02T18:03:11Z

@lucascosti the service name changes will be effective with the next runner release (2.162.0), which is scheduled to happen later today (release was frozen due to Thanksgiving weekend). I'll let you know once the release is created so you can merge your PR.

* Limit service name to 80 characters * Add L0 tests * New service name convention * Make RepoOrOrgName a computed property * Add service name sanitizing logic with L0 test

juliobbv force-pushed the users/juliobbv/limit80chars branch from 265fb63 to a390f1b Compare November 11, 2019 16:25

thboop reviewed Nov 11, 2019

View reviewed changes

src/Runner.Listener/Configuration/ServiceControlManager.cs Show resolved Hide resolved

juliobbv force-pushed the users/juliobbv/limit80chars branch from b1162a3 to a390f1b Compare November 11, 2019 17:56

thboop reviewed Nov 11, 2019

View reviewed changes

src/Test/L0/ServiceControlManagerL0.cs Outdated Show resolved Hide resolved

thboop reviewed Nov 11, 2019

View reviewed changes

src/Test/L0/ServiceControlManagerL0.cs Show resolved Hide resolved

juliobbv force-pushed the users/juliobbv/limit80chars branch from 6223624 to e0aacfa Compare November 11, 2019 20:38

thboop approved these changes Nov 12, 2019

View reviewed changes

juliobbv changed the title ~~Limit service name to 80 characters~~ New runner service name convention Nov 25, 2019

juliobbv changed the title ~~New runner service name convention~~ Implement new runner service name convention Nov 25, 2019

juliobbv force-pushed the users/juliobbv/limit80chars branch 2 times, most recently from 37ad3da to 446acd6 Compare November 25, 2019 22:57

juliobbv self-assigned this Nov 26, 2019

juliobbv requested a review from thboop November 26, 2019 16:18

thboop reviewed Nov 26, 2019

View reviewed changes

src/Runner.Listener/Configuration/ServiceControlManager.cs Outdated Show resolved Hide resolved

thboop reviewed Nov 26, 2019

View reviewed changes

src/Runner.Listener/Configuration/ServiceControlManager.cs Outdated Show resolved Hide resolved

thboop reviewed Nov 27, 2019

View reviewed changes

src/Runner.Listener/Configuration/ServiceControlManager.cs Outdated Show resolved Hide resolved

juliobbv added 4 commits November 27, 2019 11:18

Limit service name to 80 characters

d7edb1f

Add L0 tests

282466b

New service name convention

f37bc20

Make RepoOrOrgName a computed property

50ca4de

juliobbv force-pushed the users/juliobbv/limit80chars branch from 446acd6 to 50ca4de Compare November 27, 2019 16:20

thboop reviewed Nov 27, 2019

View reviewed changes

src/Runner.Common/ConfigurationStore.cs Show resolved Hide resolved

thboop approved these changes Nov 27, 2019

View reviewed changes

TingluoHuang reviewed Nov 27, 2019

View reviewed changes

src/Runner.Common/ConfigurationStore.cs Show resolved Hide resolved

Add service name sanitizing logic with L0 test

9bb6c25

thboop reviewed Nov 27, 2019

View reviewed changes

src/Runner.Listener/Configuration/ServiceControlManager.cs Show resolved Hide resolved

juliobbv merged commit 7a6d9dc into master Nov 27, 2019

juliobbv deleted the users/juliobbv/limit80chars branch November 27, 2019 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new runner service name convention #193

Implement new runner service name convention #193

juliobbv commented Nov 11, 2019 •

edited

Loading

TingluoHuang commented Nov 11, 2019

thboop commented Nov 11, 2019

juliobbv commented Nov 11, 2019 •

edited

Loading

thboop commented Nov 11, 2019 •

edited

Loading

thboop commented Nov 11, 2019

thboop commented Nov 11, 2019

juliobbv commented Nov 11, 2019 •

edited

Loading

thboop left a comment

bryanmacfarlane commented Nov 15, 2019

bryanmacfarlane commented Nov 17, 2019

juliobbv commented Nov 18, 2019

bryanmacfarlane commented Nov 18, 2019 •

edited

Loading

juliobbv commented Nov 20, 2019

juliobbv commented Nov 22, 2019

juliobbv commented Nov 26, 2019

thboop commented Nov 27, 2019

juliobbv commented Nov 27, 2019 •

edited

Loading

thboop left a comment

lucascosti commented Nov 28, 2019 •

edited

Loading

lucascosti commented Dec 2, 2019

juliobbv commented Dec 2, 2019

Implement new runner service name convention #193

Implement new runner service name convention #193

Conversation

juliobbv commented Nov 11, 2019 • edited Loading

TingluoHuang commented Nov 11, 2019

thboop commented Nov 11, 2019

juliobbv commented Nov 11, 2019 • edited Loading

thboop commented Nov 11, 2019 • edited Loading

thboop commented Nov 11, 2019

thboop commented Nov 11, 2019

juliobbv commented Nov 11, 2019 • edited Loading

thboop left a comment

Choose a reason for hiding this comment

bryanmacfarlane commented Nov 15, 2019

bryanmacfarlane commented Nov 17, 2019

juliobbv commented Nov 18, 2019

bryanmacfarlane commented Nov 18, 2019 • edited Loading

juliobbv commented Nov 20, 2019

juliobbv commented Nov 22, 2019

juliobbv commented Nov 26, 2019

thboop commented Nov 27, 2019

juliobbv commented Nov 27, 2019 • edited Loading

thboop left a comment

Choose a reason for hiding this comment

lucascosti commented Nov 28, 2019 • edited Loading

lucascosti commented Dec 2, 2019

juliobbv commented Dec 2, 2019

juliobbv commented Nov 11, 2019 •

edited

Loading

juliobbv commented Nov 11, 2019 •

edited

Loading

thboop commented Nov 11, 2019 •

edited

Loading

juliobbv commented Nov 11, 2019 •

edited

Loading

bryanmacfarlane commented Nov 18, 2019 •

edited

Loading

juliobbv commented Nov 27, 2019 •

edited

Loading

lucascosti commented Nov 28, 2019 •

edited

Loading