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.

Sign up for GitHub

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

Jump to bottom

Fix rootless socket activation #9928

Merged
merged 1 commit into from
Apr 5, 2021
Merged

Fix rootless socket activation #9928

merged 1 commit into from
Apr 5, 2021

Conversation

pendulm
Copy link
Contributor

@pendulm pendulm commented Apr 3, 2021

Move socket activation check into init() and set global condition, So rootless setup could use this condition in parent and child, child podman should adjust LISTEN_PID to its self PID.

This fix test for following scenarios:

  1. no container and pause process, start system service in a new namespace
  2. join existed pause namespace, start system service
  3. join existed container namespace while pause process die, start system service

when use Shortcut re-exec, I think the logic is still OK.

Closes: #9280

Signed-off-by: pendulm [email protected]

@pendulm pendulm force-pushed the fix_rootless_socket_activation branch from 17e567a to 286aa42 Compare April 3, 2021 09:09
@rhatdan
Copy link
Member

rhatdan commented Apr 3, 2021

Thanks @pendulm

Could you squash your commits into one commit, and then make sure there is a line break between the heading

Fix rootless socket activation

...

Then tests should pass validation.

@giuseppe PTAL

@pendulm pendulm force-pushed the fix_rootless_socket_activation branch 2 times, most recently from edd76c6 to 5b8f61c Compare April 3, 2021 16:41
@pendulm
Copy link
Contributor Author

pendulm commented Apr 4, 2021

@rhatdan modified according to your sugguestion.

@imperialguy imperialguy mentioned this pull request Apr 4, 2021
Closed
giuseppe
giuseppe reviewed Apr 4, 2021
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

thanks for the patch. Just left some comments

pkg/rootless/rootless_linux.c Outdated

// save systemd socket environment for rootless child
do_socket_activation = true;
char * save_buf = (char *) calloc (len_pid + len_fds + len_fdnames, 1);
Copy link
Member

Choose a reason for hiding this comment

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

can we add a check for calloc failures?

Copy link
Member

Choose a reason for hiding this comment

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

also there is no need to use a single buffer here. Let's split it to multiple assignments and use strdup instead. Each of the strdup must be checked for errors.

@pendulm
Move socket activation check into init() and set global condition.
11917a1 
So rootless setup could use this condition in parent and child, child
podman should adjust LISTEN_PID to its self PID.

Add system test for systemd socket activation

Signed-off-by: pendulm <[email protected]>
@pendulm pendulm force-pushed the fix_rootless_socket_activation branch from 5b8f61c to 11917a1 Compare April 5, 2021 09:49
@rhatdan
Copy link
Member

rhatdan commented Apr 5, 2021

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pendulm, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2021
@rhatdan
Copy link
Member

rhatdan commented Apr 5, 2021

@baude @ashley-cui This might be the issue you were seeing in the podman-machine testing.

@baude
Copy link
Member

baude commented Apr 5, 2021

LGTM

@mheon
Copy link
Member

mheon commented Apr 5, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2021
@openshift-merge-robot openshift-merge-robot merged commit 1c8d3d0 into containers:master Apr 5, 2021
@rhatdan
Copy link
Member

rhatdan commented Apr 5, 2021

I don't believe the calloc checks were ever added. @pendulm could you open a PR to fix @giuseppe suggestions?

@mheon
Copy link
Member

mheon commented Apr 5, 2021

Oh - sorry. I thought @giuseppe added those as review comments on the PR, and since I didn't see any I assumed they'd been fixed.

@pendulm pendulm deleted the fix_rootless_socket_activation branch April 5, 2021 15:30
@pendulm
Copy link
Contributor Author

pendulm commented Apr 5, 2021

@rhatdan sorry for comment lately. I followed @giuseppe's second suggestion, use strdup to eliminate calloc and check for errors, so no further PR is needed.

@rhatdan
Copy link
Member

rhatdan commented Apr 5, 2021

Fixed: #9484

@rhatdan
Copy link
Member

rhatdan commented Apr 5, 2021

Ok great.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@giuseppe giuseppe giuseppe left review comments

Assignees

@mheon mheon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rootless service not work with systemd socket activation
7 participants
@pendulm @rhatdan @openshift-ci-robot @baude @mheon @giuseppe @openshift-merge-robot