Skip to content

kube play: set service container as main PID when possible #17469

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

Merged
merged 1 commit into from
Feb 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions pkg/domain/infra/abi/play.go
Original file line number Diff line number Diff line change
Expand Up @@ -314,14 +314,37 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, options
return nil, fmt.Errorf("YAML document does not contain any supported kube kind")
}

// If we started containers along with a service container, we are
// running inside a systemd unit and need to set the main PID.
if options.ServiceContainer && ranContainers {
message := fmt.Sprintf("MAINPID=%d\n%s", os.Getpid(), daemon.SdNotifyReady)
if err := notifyproxy.SendMessage("", message); err != nil {
return nil, err
}
switch len(notifyProxies) {
case 0: // Optimization for containers/podman/issues/17345
// No container needs sdnotify, so we can mark the
// service container's conmon as the main PID and
// return early.
data, err := serviceContainer.Inspect(false)
Copy link
Member

Choose a reason for hiding this comment

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

Inspect is rather expensive since we look out for permanence I think it would be better to add an accessor for the ConmonPid.

Copy link
Member Author

@vrothberg vrothberg Feb 10, 2023

Choose a reason for hiding this comment

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

Nice idea! With the afternoon meetings ahead, I am running out of time but I can add an accessor in another PR.

We did the inspect before 4fa307f so it's at least not a regression.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

if err != nil {
return nil, err
}
message := fmt.Sprintf("MAINPID=%d\n%s", data.State.ConmonPid, daemon.SdNotifyReady)
if err := notifyproxy.SendMessage("", message); err != nil {
return nil, err
}
default:
// At least one container has a custom sdnotify policy,
// so we need to let the sdnotify proxies run for the
// lifetime of the service container. That means, we
// need to wait for the service container to stop.
// Podman will hence be marked as the main PID. That
// comes at the cost of keeping Podman running.
message := fmt.Sprintf("MAINPID=%d\n%s", os.Getpid(), daemon.SdNotifyReady)
if err := notifyproxy.SendMessage("", message); err != nil {
return nil, err
}

if _, err := serviceContainer.Wait(ctx); err != nil {
return nil, fmt.Errorf("waiting for service container: %w", err)
if _, err := serviceContainer.Wait(ctx); err != nil {
return nil, fmt.Errorf("waiting for service container: %w", err)
}
}
}

Expand Down
7 changes: 4 additions & 3 deletions test/system/250-systemd.bats
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ EOF

# Make sure that Podman is the service's MainPID
run systemctl show --property=MainPID --value $service_name
is "$(</proc/$output/comm)" "podman" "podman is the service mainPID"
is "$(</proc/$output/comm)" "conmon" "podman is the service mainPID"

# The name of the service container is predictable: the first 12 characters
# of the hash of the YAML file followed by the "-service" suffix
Expand Down Expand Up @@ -433,12 +433,13 @@ EOF
run_podman pod kill test_pod
for i in {0..20}; do
run systemctl is-active $service_name
if [[ $output == "inactive" ]]; then
if [[ $output == "failed" ]]; then
break
fi
sleep 0.5
done
is "$output" "inactive" "systemd service transitioned to 'inactive' state: $service_name"
# The service is marked as failed as the service container exits non-zero.
is "$output" "failed" "systemd service transitioned to 'inactive' state: $service_name"

# Now stop and start the service again.
systemctl stop $service_name
Expand Down
3 changes: 2 additions & 1 deletion test/system/252-quadlet.bats
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,8 @@ EOF
run_podman container inspect --format "{{.State.Status}}" test_pod-test
is "$output" "running" "container should be started by systemd and hence be running"

service_cleanup $QUADLET_SERVICE_NAME inactive
# The service is marked as failed as the service container exits non-zero.
service_cleanup $QUADLET_SERVICE_NAME failed
run_podman rmi $(pause_image)
}

Expand Down
10 changes: 8 additions & 2 deletions test/system/260-sdnotify.bats
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,14 @@ EOF
_start_socat
wait_for_file $_SOCAT_LOG

# Will run until all containers have stopped.
run_podman play kube --service-container=true --log-driver journald $yaml_source

# The service container is the main PID since no container has a custom
# sdnotify policy.
run_podman container inspect $service_container --format "{{.State.ConmonPid}}"
main_pid="$output"

# Will run until all containers have stopped.
run_podman container wait $service_container test_pod-test

# Make sure the containers have the correct policy.
Expand All @@ -233,7 +239,7 @@ ignore"
echo "$output"

# The "with policies" test below checks the MAINPID.
is "$output" "MAINPID=.*
is "$output" "MAINPID=$main_pid
READY=1" "sdnotify sent MAINPID and READY"

_stop_socat
Expand Down