Set StopTimeout for compat API if not set by client #19704

rhatdan · 2023-08-22T21:37:15Z

Currently containers created via DOCKER API without specifying
StopTimeout are defaulting to 0 seconds. This change should
default them to setting in containers.conf normally 10 seconds.

Fixes: #19139

Set StopTimeout for service-container started under podman kube play

Fixes: #19139

Service containers are defaulting to 0 seconds for Timeout rather then
the settings in containers.conf.

Does this PR introduce a user-facing change?

Containers created via compat API default to 10 seconds or containers.conf setting.

vrothberg

LGTM

vrothberg · 2023-08-23T12:06:04Z

pkg/domain/infra/abi/play.go

@@ -72,7 +76,8 @@ func (ic *ContainerEngine) createServiceContainer(ctx context.Context, name stri
 		ReadOnly:         true,
 		ReadWriteTmpFS:   false,
 		// No need to spin up slirp etc.
-		Net: &entities.NetOptions{Network: specgen.Namespace{NSMode: specgen.NoNetwork}},
+		Net:         &entities.NetOptions{Network: specgen.Namespace{NSMode: specgen.NoNetwork}},
+		StopTimeout: rtc.Engine.StopTimeout,


Nice catch!

Reporter actually caught it.

@InvalidInterrupt PTAL

Thinking about it without a fried brain: I am not sure the stop timeout makes that much sense for a service container. There is no workload inside other than catatonit. But catatonit handles SIGTERM just fine so I don't expect any measurable negative performance impact on shut down.

edsantiago · 2023-08-23T13:03:51Z

test/system/260-sdnotify.bats

+            run_podman container inspect --format '{{.Config.StopTimeout}}' $service_container
+            is "$output" "10" "StopTimeout should be initialized to 10"


This is getting run eighteen times. Could there maybe be a better place to do this check?

(I'd love to find it myself but am currently rabbitholing into a weird "Closing sdnotify proxy ... unixgram" error)

Luap99 · 2023-08-23T16:12:09Z

test/apiv2/20-containers.at

@@ -308,6 +308,7 @@ cid_top=$(jq -r '.Id' <<<"$output")
 t GET containers/${cid_top}/json 200 \
  .Config.Entrypoint[0]="top" \
  .Config.Cmd='[]' \
+  .Config.StopTimeput="10" \


typo Timeout, I guess this is why the tests fails

mheon · 2023-08-23T17:51:26Z

Code LGTM

TomSweeneyRedHat · 2023-08-23T23:54:03Z

Code LGTM
Tests are still unhappy

rhatdan · 2023-08-24T01:57:42Z

All bud tests are blowing up because of the removal of podmanhello.

commit 7e0130f75cc27ae32d8843a8be8d2588aa2dcee9
Author: Chris Evich <[email protected]>
Date:   Wed Aug 23 12:10:13 2023 -0400

    Remove `hello` multi-arch image build
    
    Moved to https://github.com/containers/PodmanHello
    
    Signed-off-by: Chris Evich <[email protected]>

Currently containers created via DOCKER API without specifying StopTimeout are defaulting to 0 seconds. This change should default them to setting in containers.conf normally 10 seconds. Fixes: containers#19139 Signed-off-by: Daniel J Walsh <[email protected]>

Fixes: containers#19139 Service containers are defaulting to 0 seconds for Timeout rather then the settings in containers.conf. Signed-off-by: Daniel J Walsh <[email protected]>

vrothberg

/lgtm

vrothberg · 2023-08-25T09:25:22Z

pkg/domain/infra/abi/play.go

@@ -72,7 +76,8 @@ func (ic *ContainerEngine) createServiceContainer(ctx context.Context, name stri
 		ReadOnly:         true,
 		ReadWriteTmpFS:   false,
 		// No need to spin up slirp etc.
-		Net: &entities.NetOptions{Network: specgen.Namespace{NSMode: specgen.NoNetwork}},
+		Net:         &entities.NetOptions{Network: specgen.Namespace{NSMode: specgen.NoNetwork}},
+		StopTimeout: rtc.Engine.StopTimeout,


Thinking about it without a fried brain: I am not sure the stop timeout makes that much sense for a service container. There is no workload inside other than catatonit. But catatonit handles SIGTERM just fine so I don't expect any measurable negative performance impact on shut down.

openshift-ci · 2023-08-25T09:25:32Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [rhatdan,vrothberg]

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

rhatdan · 2023-08-25T12:39:30Z

I thought the reporter noticed an issue on the service timeout, exiting before the containers and pods had a chance to cleanup.

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 22, 2023

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Aug 22, 2023

rhatdan force-pushed the timeout branch from d0d599f to 5c537db Compare August 23, 2023 12:04

vrothberg reviewed Aug 23, 2023

View reviewed changes

edsantiago reviewed Aug 23, 2023

View reviewed changes

rhatdan force-pushed the timeout branch from 5c537db to 6d92307 Compare August 23, 2023 14:23

Luap99 reviewed Aug 23, 2023

View reviewed changes

rhatdan force-pushed the timeout branch from 6d92307 to c3288cb Compare August 23, 2023 18:46

rhatdan added 2 commits August 24, 2023 13:24

Set StopTimeout for service-container started under podman kube play

f32e954

Fixes: containers#19139 Service containers are defaulting to 0 seconds for Timeout rather then the settings in containers.conf. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the timeout branch from c3288cb to f32e954 Compare August 24, 2023 17:24

vrothberg approved these changes Aug 25, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Aug 25, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2023

openshift-merge-robot merged commit 584c1e7 into containers:main Aug 25, 2023

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 Nov 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set StopTimeout for compat API if not set by client #19704

Set StopTimeout for compat API if not set by client #19704

rhatdan commented Aug 22, 2023 •

edited

Loading

vrothberg left a comment

vrothberg Aug 23, 2023

rhatdan Aug 23, 2023

rhatdan Aug 23, 2023 •

edited

Loading

vrothberg Aug 25, 2023

edsantiago Aug 23, 2023

Luap99 Aug 23, 2023

mheon commented Aug 23, 2023

TomSweeneyRedHat commented Aug 23, 2023

rhatdan commented Aug 24, 2023

vrothberg left a comment

vrothberg Aug 25, 2023

openshift-ci bot commented Aug 25, 2023

rhatdan commented Aug 25, 2023

		run_podman container inspect --format '{{.Config.StopTimeout}}' $service_container
		is "$output" "10" "StopTimeout should be initialized to 10"

Set StopTimeout for compat API if not set by client #19704

Set StopTimeout for compat API if not set by client #19704

Conversation

rhatdan commented Aug 22, 2023 • edited Loading

Does this PR introduce a user-facing change?

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Aug 23, 2023

Choose a reason for hiding this comment

rhatdan Aug 23, 2023

Choose a reason for hiding this comment

rhatdan Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

vrothberg Aug 25, 2023

Choose a reason for hiding this comment

edsantiago Aug 23, 2023

Choose a reason for hiding this comment

Luap99 Aug 23, 2023

Choose a reason for hiding this comment

mheon commented Aug 23, 2023

TomSweeneyRedHat commented Aug 23, 2023

rhatdan commented Aug 24, 2023

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Aug 25, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Aug 25, 2023

rhatdan commented Aug 25, 2023

rhatdan commented Aug 22, 2023 •

edited

Loading

rhatdan Aug 23, 2023 •

edited

Loading