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.

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

container: add a timeout for deletion #47713

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 7 additions & 6 deletions api/types/container/config.go
Expand Up @@ -73,10 +73,11 @@ type Config struct {
// Mac Address of the container.
//
// Deprecated: this field is deprecated since API v1.44. Use EndpointSettings.MacAddress instead.
MacAddress string `json:",omitempty"`
OnBuild []string // ONBUILD metadata that were defined on the image Dockerfile
Labels map[string]string // List of labels set to this container
StopSignal string `json:",omitempty"` // Signal to stop a container
StopTimeout *int `json:",omitempty"` // Timeout (in seconds) to stop a container
Shell strslice.StrSlice `json:",omitempty"` // Shell for shell-form of RUN, CMD, ENTRYPOINT
MacAddress string `json:",omitempty"`
OnBuild []string // ONBUILD metadata that were defined on the image Dockerfile
Labels map[string]string // List of labels set to this container
StopSignal string `json:",omitempty"` // Signal to stop a container
StopTimeout *int `json:",omitempty"` // Timeout (in seconds) to stop a container
DeleteTimeout *int `json:",omitempty"` // Timeout (in seconds) to wait for a container to be deleted
vvoland marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I need to have a closer look at the full proposal (sorry; really behind on my backlog); mostly was curious if this is persisted to disk, and if it needs to be, or only to be passed when performing a delete request.

For StopTimeout and StopSignal, the time-out is related to what's running in the container (e.g. database that you want to provide a time to do a graceful shutdown)

For DeleteTimeout, I wonder if it's a property of the container or not

Shell strslice.StrSlice `json:",omitempty"` // Shell for shell-form of RUN, CMD, ENTRYPOINT
}
14 changes: 12 additions & 2 deletions container/container.go
Expand Up @@ -45,8 +45,9 @@ import (
)

const (
configFileName = "config.v2.json"
hostConfigFileName = "hostconfig.json"
configFileName = "config.v2.json"
hostConfigFileName = "hostconfig.json"
defaultDeleteTimeout = 30
)

// ExitStatus provides exit reasons for a container.
Expand Down Expand Up @@ -562,6 +563,15 @@ func (container *Container) StopTimeout() int {
return defaultStopTimeout
}

// DeleteTimeout returns the timeout (in seconds) to wait for a container to be deleted.
func (container *Container) DeleteTimeout() int {
if container.Config.DeleteTimeout != nil {
return *container.Config.DeleteTimeout
}

return defaultDeleteTimeout
}

// InitDNSHostConfig ensures that the dns fields are never nil.
// New containers don't ever have those fields nil,
// but pre created containers can still have those nil values.
Expand Down
20 changes: 20 additions & 0 deletions container/container_unit_test.go
Expand Up @@ -57,6 +57,26 @@ func TestContainerStopTimeout(t *testing.T) {
}
}

func TestContainerDeleteTimeout(t *testing.T) {
c := &Container{
Config: &container.Config{},
}

s := c.DeleteTimeout()
if s != defaultDeleteTimeout {
t.Fatalf("Expected %v, got %v", defaultDeleteTimeout, s)
}

deleteTimeout := 15
c = &Container{
Config: &container.Config{DeleteTimeout: &deleteTimeout},
}
s = c.DeleteTimeout()
if s != deleteTimeout {
t.Fatalf("Expected %v, got %v", deleteTimeout, s)
}
}

func TestContainerSecretReferenceDestTarget(t *testing.T) {
ref := &swarmtypes.SecretReference{
File: &swarmtypes.SecretReferenceFileTarget{
Expand Down
3 changes: 3 additions & 0 deletions daemon/container.go
Expand Up @@ -254,6 +254,9 @@ func validateContainerConfig(config *containertypes.Config) error {
return err
}
}
if config.DeleteTimeout != nil && *config.DeleteTimeout <= 0 {
return fmt.Errorf("invalid delete timeout %v", config.DeleteTimeout)
}
// Validate if Env contains empty variable or not (e.g., ``, `=foo`)
for _, env := range config.Env {
if _, err := opts.ValidateEnv(env); err != nil {
Expand Down
3 changes: 2 additions & 1 deletion daemon/monitor.go
Expand Up @@ -39,7 +39,8 @@ func (daemon *Daemon) handleContainerExit(c *container.Container, e *libcontaine

tsk, ok := c.Task()
if ok {
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
deleteTimeout := time.Duration(c.DeleteTimeout()) * time.Second
ctx, cancel := context.WithTimeout(context.Background(), deleteTimeout)
es, err := tsk.Delete(ctx)
cancel()
if err != nil {
Expand Down
10 changes: 10 additions & 0 deletions image/cache/compare.go
Expand Up @@ -149,6 +149,16 @@ func compare(a, b *container.Config) bool {
return false
}
}

if (a.DeleteTimeout == nil) != (b.DeleteTimeout == nil) {
return false
}
if a.DeleteTimeout != nil && b.DeleteTimeout != nil {
if *a.DeleteTimeout != *b.DeleteTimeout {
return false
}
}

if (a.Healthcheck == nil) != (b.Healthcheck == nil) {
return false
}
Expand Down