Skip to content
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

Systemd 256 loginctl changes #20400

Merged
merged 2 commits into from May 15, 2024

Conversation

jelly
Copy link
Member

@jelly jelly commented Apr 29, 2024

No description provided.

@jelly jelly requested a review from martinpitt April 29, 2024 14:06
raise

# terminate all systemd user services for users who are not logged in
self.machine.execute("systemctl stop user@*.service")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change changes behaviour, as it used to stop non web console sessions. Is that wanted?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, loginctl kill-user should do that as well, but we don't do that. Let's see what the tests say -- if it goes green, then ok. It may also only affect e.g. c-podman, but we can put it back if needed. This cleanup is unfortunately incredibly finnicky and brittle 😢

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you get errors like this from stale sessions 😢

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So hmm should we generalize this? Or add another for loop to iterate over "normal sessions" except where uid == 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I just saw that we already do the loginctl kill-user thing in terminate_sessions(). So I don't know, perhaps reproduce one of the failures locally and see how you can clean up after e.g. the "anton" test?

test/verify/check-session Outdated Show resolved Hide resolved
raise

# terminate all systemd user services for users who are not logged in
self.machine.execute("systemctl stop user@*.service")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, loginctl kill-user should do that as well, but we don't do that. Let's see what the tests say -- if it goes green, then ok. It may also only affect e.g. c-podman, but we can put it back if needed. This cleanup is unfortunately incredibly finnicky and brittle 😢

@martinpitt martinpitt marked this pull request as draft May 9, 2024 05:38
@jelly jelly force-pushed the systemd-256-loginctl-changes branch from d53d53e to daa397c Compare May 15, 2024 10:26
jelly added 2 commits May 15, 2024 15:20
In systemd 256 list-session now also includes a manager session:

4 1000 admin -    538    user          web console no   -
5 1000 admin -    544    manager       -           no   -
@jelly jelly force-pushed the systemd-256-loginctl-changes branch from daa397c to c40c9e6 Compare May 15, 2024 13:21
@jelly jelly marked this pull request as ready for review May 15, 2024 13:21
@jelly jelly requested a review from mvollmer May 15, 2024 13:22
@jelly
Copy link
Member Author

jelly commented May 15, 2024

This now broke fedora-rawhide on TF so we should fix this now :) and this also needs cockpit-project/bots#6385

Copy link
Member

@mvollmer mvollmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to know what these "manager" sessions are.. but if this works, it works. :-)

@jelly
Copy link
Member Author

jelly commented May 15, 2024

Would be nice to know what these "manager" sessions are.. but if this works, it works. :-)

Agreed, created systemd/systemd#32845

@jelly
Copy link
Member Author

jelly commented May 15, 2024

rawhide tests run again, so I consider this a success!

@jelly jelly merged commit 2e968f9 into cockpit-project:main May 15, 2024
79 of 80 checks passed
@jelly jelly deleted the systemd-256-loginctl-changes branch May 15, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants