fix(runtime): remove container after run #5796
Conversation
| err = c.ContainerRemove(ctx, rsp.ID, container.RemoveOptions{Force: true, RemoveVolumes: true}) | ||
| return errors.Wrap(err, "cannot remove Docker container") |
There was a problem hiding this comment.
Should this be optional? I could imagine wanting to inspect the container after the fact for debugging.
Perhaps remove it by default, with an opt-out flag?
There was a problem hiding this comment.
Cool, I can update this. Good point raised.
There was a problem hiding this comment.
Added in a Remove option, which is default. People can override this by
apiVersion: pkg.crossplane.io/v1beta1
kind: Function
metadata:
name: function-go-templating
annotations.render.crossplane.io/runtime-docker-cleanup: "Stop"
spec:
package: xpkg.upbound.io/crossplane-contrib/function-go-templating:v0.4.1
| @@ -93,6 +96,9 @@ type RuntimeDocker struct { | |||
| // Stop container once rendering is done | |||
| Stop bool | |||
|
|
|||
| // Stop container once rendering is done | |||
| Remove bool | |||
There was a problem hiding this comment.
Now that we have two bools, I think it makes sense to replace both with an enum. For example:
type RuntimeDocker struct {
Image string
Cleanup DockerCleanup
PullPolicy DockerPullPolicy
// etc...
}I prefer the enum, since remove always implies stop. It wouldn't make sense to set Stop = false but Remove = true.
There was a problem hiding this comment.
Agreed, will fix this up
| if r.Stop || r.Remove { | ||
| stop = func(ctx context.Context) error { | ||
| err := c.ContainerStop(ctx, rsp.ID, container.StopOptions{}) | ||
| if err != nil { | ||
| return errors.Wrap(err, "cannot stop Docker container") | ||
| } | ||
|
|
||
| err = c.ContainerRemove(ctx, rsp.ID, container.RemoveOptions{Force: true, RemoveVolumes: true}) | ||
| return errors.Wrap(err, "cannot remove Docker container") | ||
| if r.Remove { | ||
| err = c.ContainerRemove(ctx, rsp.ID, container.RemoveOptions{Force: true, RemoveVolumes: true}) | ||
| if err != nil { | ||
| return errors.Wrap(err, "cannot remove Docker container") | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Since we now have multiple cases inside the stop function, I'd suggest moving everything in there. Assuming you add the Cleanup enum per my other comment, I think something like this would work well:
stop := func(ctx context.Context) error {
switch r.Cleanup {
case AnnotationValueRuntimeDockerCleanupOrphan:
r.log.Debug("Container left running", "container", rsp.ID, "image", r.Image)
return nil
case AnnotationValueRuntimeDockerCleanupStop:
if err := c.ContainerStop(ctx, rsp.ID, container.StopOptions{}); err != nil {
return errors.Wrap(err, "cannot stop Docker container")
}
case AnnotationValueRuntimeDockerCleanupRemove:
if err := c.ContainerStop(ctx, rsp.ID, container.StopOptions{}); err != nil {
return errors.Wrap(err, "cannot stop Docker container")
}
if err := c.ContainerRemove(ctx, rsp.ID, container.RemoveOptions{}); err != nil {
return errors.Wrap(err, "cannot remove Docker container")
}
}
return nil
}(I know the stop is repeated, but I prefer repeating it to nesting it.)
There was a problem hiding this comment.
Thanks for the tip, I was thinking something along the lines and I agree with you on the stop repeated but it's better than nesting it
Signed-off-by: johnathan-sq <[email protected]>
3920e13
to
735c974
Compare
Squashed the commits, appreciate the response on getting the PR over the line |
Description of your changes
This pull request includes a change to the
Startmethod in theRuntimeDockerstruct within thecmd/crank/beta/render/runtime_docker.gofile. The change enhances the stop functionality of a Docker container. Now, in addition to stopping the container, the container is also removed along with its volumes. This is a crucial change as it ensures proper cleanup of resources when a Docker container is stopped.Reason for this change when running
crossplane beta rendercommands the docker containers do not removeI have:
earthly +reviewableto ensure this PR is ready for review.Added or updated unit tests.Added or updated e2e tests.Linked a PR or a [docs tracking issue] to [document this change].Addedbackport release-x.ylabels to auto-backport this PR.Unsure on how to unit test or e2e test this, suggestions would be appreciated.
From my manual testing, I compiled the binary and ran it locally to confirm the containers do get removed.