Allow containers to --restart on-failure with --rm #8263
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci-robot
added
the
approved
rhatdan
force-pushed
the
restart
branch
2 times, most recently
from
2eed98d to
7fa9206
Compare
rhatdan
changed the title
| @@ -714,3 +714,39 @@ func (c *Container) Restore(ctx context.Context, options ContainerCheckpointOpti | |||
| defer c.newContainerEvent(events.Restore) | |||
| return c.restore(ctx, options) | |||
| } | |||
|
|
|||
| // Indicate whether or not the container should restart | |||
| func (c *Container) ShouldRestart(ctx context.Context) bool { | |||
There was a problem hiding this comment.
Did you duplicate all of this logic? It looks like it was copied from container_internal
There was a problem hiding this comment.
It is, I will use the same function in both places.
| @@ -9,7 +9,7 @@ import ( | |||
| // by validate must not need any state information on the flag (i.e. changed) | |||
| func (c *ContainerCLIOpts) validate() error { | |||
| var () | |||
| if c.Rm && c.Restart != "" && c.Restart != "no" { | |||
| if c.Rm && (c.Restart != "" && c.Restart != "no" && c.Restart != "on-failure") { | |||
There was a problem hiding this comment.
It would be nice to have "on-failure" defined in a variable somewhere rather than a bare string scattered about. Not for this pr though.
There was a problem hiding this comment.
It is defined in the libpod package as RestartPolicyOnFailure.
The same for no it should be RestartPolicyNo.
Line 99 in dc58d4e
rhatdan
force-pushed
the
restart
branch
4 times, most recently
from
2f8e18f to
f87b474
Compare
|
@containers/podman-maintainers PTAL |
|
lgtm |
| @@ -390,3 +390,15 @@ func ContainerInit(ctx context.Context, nameOrID string) error { | |||
| } | |||
| return response.Process(nil) | |||
| } | |||
|
|
|||
| func ShouldRestart(ctx context.Context, nameOrID string) (bool, error) { | |||
There was a problem hiding this comment.
Why do we need this in the API and Bindings? This should be local-only, podman container cleanup should never be invoked remotely
There was a problem hiding this comment.
In pkg/domain/infra/tunnel/containers.go when we do a non detached container, we Remove the container when it completes. Added this ShouldRestart check to make sure that this does not happen.
There was a problem hiding this comment.
I don't know if it's worth adding a complete new API endpoint to handle this. Maybe we should just disable removing the container in that case if it detects that the container has this restart policy?
There was a problem hiding this comment.
Then we end up with the race conditions again. But for only a small subset of calls.
There was a problem hiding this comment.
I removed the bindings and just stopped rm the container in run if the restart policy is set to something other then no.
There was a problem hiding this comment.
Removing the bindings proved to fail testing. So I added them back.
rhatdan
force-pushed
the
restart
branch
6 times, most recently
from
288345d to
cced37e
Compare
Signed-off-by: Daniel J Walsh <[email protected]>
|
Tests aren't hip. |
github-actions
bot
added
the
locked - please file new issue/PR
Fixes: #7906
Signed-off-by: Daniel J Walsh [email protected]