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

fix nerdctl ps slow on heavy IO system by using goroutine #3673

Merged
merged 1 commit into from
Dec 2, 2024
Merged
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
46 changes: 36 additions & 10 deletions pkg/cmd/container/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"fmt"
"sort"
"strings"
"sync"
"time"

containerd "github.com/containerd/containerd/v2/client"
Expand All @@ -40,11 +41,11 @@ import (

// List prints containers according to `options`.
func List(ctx context.Context, client *containerd.Client, options types.ContainerListOptions) ([]ListItem, error) {
containers, err := filterContainers(ctx, client, options.Filters, options.LastN, options.All)
containers, cMap, err := filterContainers(ctx, client, options.Filters, options.LastN, options.All)
if err != nil {
return nil, err
}
return prepareContainers(ctx, client, containers, options)
return prepareContainers(ctx, client, containers, cMap, options)
}

// filterContainers returns containers matching the filters.
Expand All @@ -53,14 +54,14 @@ func List(ctx context.Context, client *containerd.Client, options types.Containe
// - all means showing all containers (default shows just running).
// - lastN means only showing n last created containers (includes all states). Non-positive values are ignored.
// In other words, if lastN is positive, all will be set to true.
func filterContainers(ctx context.Context, client *containerd.Client, filters []string, lastN int, all bool) ([]containerd.Container, error) {
func filterContainers(ctx context.Context, client *containerd.Client, filters []string, lastN int, all bool) ([]containerd.Container, map[string]string, error) {
djdongjin marked this conversation as resolved.
Show resolved Hide resolved
containers, err := client.Containers(ctx)
if err != nil {
return nil, err
return nil, nil, err
}
filterCtx, err := foldContainerFilters(ctx, containers, filters)
if err != nil {
return nil, err
return nil, nil, err
}
containers = filterCtx.MatchesFilters(ctx)

Expand All @@ -77,18 +78,37 @@ func filterContainers(ctx context.Context, client *containerd.Client, filters []
}
}

var wg sync.WaitGroup
statusPerContainer := make(map[string]string)
var mu sync.Mutex
// formatter.ContainerStatus(ctx, c) is time consuming so we do it in goroutines and return the container's id with status as a map.
// prepareContainers func will use this map to avoid call formatter.ContainerStatus again.
for _, c := range containers {
if c.ID() == "" {
return nil, nil, fmt.Errorf("container id is nill")
}
wg.Add(1)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
wg.Add(1)
c := c
wg.Add(1)

the above line code simplifies the code and avoid using a mutex, right ?

Copy link
Member

Choose a reason for hiding this comment

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

@ningmingxiao WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

avoid using a mutex

I don't think it will avoid the mutex. Current write to a map still need the mutex.

Copy link
Member

@fahedouch fahedouch Dec 2, 2024

Choose a reason for hiding this comment

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

indeed map are note safe for concurrent writes. thanks @djdongjin for your pointer ;)

go func(ctx context.Context, c containerd.Container) {
defer wg.Done()
cStatus := formatter.ContainerStatus(ctx, c)
mu.Lock()
statusPerContainer[c.ID()] = cStatus
mu.Unlock()
}(ctx, c)
}
wg.Wait()
if all || filterCtx.all {
return containers, nil
return containers, statusPerContainer, nil
}

var upContainers []containerd.Container
for _, c := range containers {
cStatus := formatter.ContainerStatus(ctx, c)
cStatus := statusPerContainer[c.ID()]
if strings.HasPrefix(cStatus, "Up") {
upContainers = append(upContainers, c)
}
}
return upContainers, nil
return upContainers, statusPerContainer, nil
}

type ListItem struct {
Expand All @@ -112,7 +132,7 @@ func (x *ListItem) Label(s string) string {
return x.LabelsMap[s]
}

func prepareContainers(ctx context.Context, client *containerd.Client, containers []containerd.Container, options types.ContainerListOptions) ([]ListItem, error) {
func prepareContainers(ctx context.Context, client *containerd.Client, containers []containerd.Container, statusPerContainer map[string]string, options types.ContainerListOptions) ([]ListItem, error) {
listItems := make([]ListItem, len(containers))
snapshottersCache := map[string]snapshots.Snapshotter{}
for i, c := range containers {
Expand All @@ -136,6 +156,12 @@ func prepareContainers(ctx context.Context, client *containerd.Client, container
if options.Truncate && len(id) > 12 {
id = id[:12]
}
var status string
if s, ok := statusPerContainer[c.ID()]; ok {
status = s
} else {
return nil, fmt.Errorf("can't get container %s status", c.ID())
}
li := ListItem{
Command: formatter.InspectContainerCommand(spec, options.Truncate, true),
CreatedAt: info.CreatedAt,
Expand All @@ -144,7 +170,7 @@ func prepareContainers(ctx context.Context, client *containerd.Client, container
Platform: info.Labels[labels.Platform],
Names: containerutil.GetContainerName(info.Labels),
Ports: formatter.FormatPorts(info.Labels),
Status: formatter.ContainerStatus(ctx, c),
Status: status,
Runtime: info.Runtime.Name,
Labels: formatter.FormatLabels(info.Labels),
LabelsMap: info.Labels,
Expand Down