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

container: Set primary process to 1 via LISTEN_PID by default if configuration is missing #721

Merged
merged 1 commit into from
Aug 31, 2021
Merged

container: Set primary process to 1 via LISTEN_PID by default if configuration is missing #721

merged 1 commit into from
Aug 31, 2021

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Aug 25, 2021 •
edited
Loading

Following PR adds a default configuration of seting primary process to 1 via LISTEN_PID if configuration is missing.
Following behavior is similar to runc.

@flouthoc
Copy link
Collaborator Author

@giuseppe PTAL

@flouthoc
Copy link
Collaborator Author

@vrothberg PTAL

giuseppe
giuseppe reviewed Aug 25, 2021
View reviewed changes
src/libcrun/container.c Outdated
@@ -1187,6 +1187,13 @@ container_init_setup (void *args, pid_t own_pid, char *notify_socket, int sync_s
if (clearenv ())
return crun_make_error (err, errno, "clearenv");

// set primary process to 1 explicitly if nothing is configured and LISTEN_FD is not set
if (getenv ("LISTEN_PID") == NULL && entrypoint_args->context->preserve_fds == 0)
Copy link
Member

Choose a reason for hiding this comment

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

it should also check for LISTEN_FDS to be set.

If LISTEN_FDS is not set, then we don't want to set LISTEN_PID

Copy link
Member

Choose a reason for hiding this comment

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

the check should be entrypoint_args->context->preserve_fds > 0

Copy link
Member

Choose a reason for hiding this comment

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

I concur, that's what runc does:

https://github.com/opencontainers/runc/blob/8b59b768a945babf39e2468dc5d8afa54c511852/utils_linux.go#L279-L282

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@giuseppe ah I thought context->preserve_fds is always 0 when LISTEN_FDS is not set but let me confirm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vrothberg @giuseppe fixed in latest commit.

vrothberg
vrothberg reviewed Aug 25, 2021
View reviewed changes
src/libcrun/container.c Outdated Show resolved Hide resolved
giuseppe
giuseppe reviewed Aug 25, 2021
View reviewed changes
src/libcrun/container.c Outdated Show resolved Hide resolved
@flouthoc flouthoc requested a review from giuseppe August 25, 2021 10:08
@giuseppe
Copy link
Member

I am a bit confused by the current implementation. It is adding a systemd specific feature and set the environment variable inside the container no matter how these fds are passed.

runc does it only for file descriptors that are obtained through LISTEN_FDS.

I think we would need to store the value of getenv ("LISTEN_FDS") outside the container and then work on that.

How have you tested this?

Let's hold this PR after the release.

giuseppe
giuseppe reviewed Aug 25, 2021
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
@flouthoc
Copy link
Collaborator Author

@giuseppe Sure I could test this better then we could move this into separate release.

@flouthoc
Copy link
Collaborator Author

@giuseppe We are mostly using context->preserve_fds which makes sure that getenv(LISTEN_FDS) is configured outside of the container scope https://github.com/containers/crun/blob/main/src/run.c#L174 we can also get LISTEN_PID and pass it down via context as that seems more generic approach.

@giuseppe
Copy link
Member

@giuseppe We are mostly using context->preserve_fds which makes sure that getenv(LISTEN_FDS) is configured outside of the container scope https://github.com/containers/crun/blob/main/src/run.c#L174 we can also get LISTEN_PID and pass it down via context as that seems more generic approach.

I think we need to differentiate between the FDs added through LISTEN_FDS thus handled us by systemd, and the generic fds that can be passed through --preserve-fds from the command line

@flouthoc
Copy link
Collaborator Author

makes sense write now preserve_fds could the ones coming from cmd line as well.

@rhatdan
Copy link
Member

rhatdan commented Aug 25, 2021

LGTM

vrothberg
vrothberg approved these changes Aug 26, 2021
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@flouthoc
Copy link
Collaborator Author

@vrothberg @rhatdan There is a change which is still pending which @giuseppe suggested #721 (comment) , I still have to add that.

@flouthoc flouthoc marked this pull request as draft August 26, 2021 09:57
@giuseppe
Copy link
Member

@vrothberg @rhatdan There is a change which is still pending which @giuseppe suggested #721 (comment) , I still have to add that.

do we still need this PR given that we are addressing the issue in Podman anyway?

@rhatdan
Copy link
Member

rhatdan commented Aug 26, 2021

Would it be useful if crun was launched via a different tool? Should it match runc behaviour?

@flouthoc flouthoc force-pushed the set_listen_pid branch 2 times, most recently from e2e5867 to 682a3cf Compare August 31, 2021 09:19
@flouthoc flouthoc requested a review from giuseppe August 31, 2021 09:25
giuseppe
giuseppe requested changes Aug 31, 2021
View reviewed changes
src/libcrun/container.c Outdated
if (entrypoint_args->context->listen_fds > 0)
{
setenv ("LISTEN_PID", "1", 1);
libcrun_warning ("setting LISTEN_PID=1 since no previous configuration was found");
Copy link
Member

Choose a reason for hiding this comment

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

don't we need to make sure the variable wasn't already set like: if (entrypoint_args->context->listen_fds > 0 && getenv ("LISTEN_PID") == NULL)?

I think at this point it is always unset, since we are calling it just after clearenv (). We need to make sure this code snippet is done below the other putenv and setenv.

I think this can be easily tested, and we can add a test for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it has to be below putenv(def->process->env[i]) and set_home_env making changes and adding tests as well.

@flouthoc
container: Set primary process to 1 via LISTEN_PID by default if user…
44377aa 
… configuration is missing

Adds a new field to context listen_fds which differentiates between the
fds coming from preserve_fds and the ones coming from LISTEN_FDS if
LISTEN_FDS is configured set primary process to 1.

Signed-off-by: flouthoc <flouthoc.git@gmail.com>
@flouthoc flouthoc requested a review from giuseppe August 31, 2021 10:32
@flouthoc
Copy link
Collaborator Author

flouthoc commented Aug 31, 2021 •
edited
Loading

@giuseppe PTAL made relevant changes and added tests

@flouthoc flouthoc marked this pull request as ready for review August 31, 2021 10:36
giuseppe
giuseppe approved these changes Aug 31, 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.

LGTM

@flouthoc
Copy link
Collaborator Author

@rhatdan @vrothberg this is good to merge now.

@giuseppe giuseppe merged commit 1ecfd31 into containers:main Aug 31, 2021
@flouthoc flouthoc mentioned this pull request Sep 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giuseppe giuseppe giuseppe approved these changes

@vrothberg vrothberg vrothberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@flouthoc @giuseppe @rhatdan @vrothberg