Skip to content

[3.1.x Backport] IsSystemdService: make check stricter to avoid false positives#2747

Closed
halter73 wants to merge 1 commit intorelease/3.1from
halter73/tmds-systemd-check-backport
Closed

[3.1.x Backport] IsSystemdService: make check stricter to avoid false positives#2747
halter73 wants to merge 1 commit intorelease/3.1from
halter73/tmds-systemd-check-backport

Conversation

@halter73
Copy link
Copy Markdown
Member

@halter73 halter73 commented Dec 4, 2019

Backport of #2734 to 3.1.x

Checking the INVOCATION_ID envvar is causing false positives.
Instead, we now return true only when the direct parent process is 'systemd'.

@tmds @Tragetaschen @anurse @onyxmaster @Tratcher

Checking the INVOCATION_ID envvar is causing false positives.
Instead, we now return true only when the direct parent process is 'systemd'.
@halter73 halter73 added this to the 3.1.x milestone Dec 4, 2019
@analogrelay
Copy link
Copy Markdown

analogrelay commented Dec 5, 2019

Hrm... in writing the backport request I realized there's a pretty "easy" workaround. You can conditionally call avoid calling [edited for clarity] .UseSystemd based on your own logic (I even remember we covered this in the original issue).

Given that, and the fact that this has a minor breaking change impact, I'm now pretty confident it wouldn't meet the bar.

@tmds
Copy link
Copy Markdown
Member

tmds commented Dec 5, 2019

Hrm... in writing the backport request I realized there's a pretty "easy" workaround. You can conditionally call .UseSystemd based on your own logic

The breaking change is UseSystemd is no-oping in more cases. So calling it won't do anything.

One case that breaks is a user having a script for starting dotnet from the service unit.
He may have to change the script so the dotnet invocation uses the bash 'exec' built-in. Then the dotnet process will be a direct child of systemd (otherwise the shell process sits in-between.)

@tmds
Copy link
Copy Markdown
Member

tmds commented Dec 5, 2019

You can conditionally call .UseSystemd based on your own logic

Ah, I think I should read this as: not-calling it based on your own logic.

For the logic: a heuristic that was suggested in the issue is: only calling UseSystemd when the TERM envvar is not set.

@analogrelay
Copy link
Copy Markdown

Heh, you're right, I wasn't the clearest there :). Updated for clarity.

@analogrelay
Copy link
Copy Markdown

Basically, I don't think this would meet the bar for the 3.1 LTS given the presence of a workaround. I totally support this in 5.0 and we can continue to review the logic for that (and will have previews to get feedback on it and refine it further). Putting it in an LTS patch has a lot more risk associated with it.

@tmds
Copy link
Copy Markdown
Member

tmds commented Dec 5, 2019

@anurse makes sense.

@halter73
Copy link
Copy Markdown
Member Author

halter73 commented Dec 5, 2019

Given the workaround, I agree with @anurse wrt 3.1.x, so I'm closing this.

@halter73 halter73 closed this Dec 5, 2019
@mmitche mmitche deleted the halter73/tmds-systemd-check-backport branch January 4, 2021 21:33
@ghost ghost locked as resolved and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants