From 55191ecc2069120fc7415f284a7cba2ab1a5407e Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Wed, 12 Oct 2022 10:09:18 +0200 Subject: [PATCH 1/4] Add and use Container.LinuxResource() helper This gets c.config.Spec.Linux.Resources, with some nil checks. Using this means less open coding of the nil-checks, but also the existing user of this field in moveConmonToCgroupAndSignal() was using ctr.Spec().Linux.Resources instead, and the Spec() call is very expensive. [NO NEW TESTS NEEDED] Just minor performance effects Signed-off-by: Alexander Larsson --- libpod/container.go | 8 ++++++++ libpod/kube.go | 18 +++++++++--------- libpod/oci_conmon_linux.go | 3 +-- libpod/stats_freebsd.go | 6 +++--- libpod/stats_linux.go | 6 +++--- pkg/api/handlers/compat/containers_stats.go | 6 +++--- 6 files changed, 27 insertions(+), 20 deletions(-) diff --git a/libpod/container.go b/libpod/container.go index 560f9ab9ef..2f53766492 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -688,6 +688,14 @@ func (c *Container) Terminal() bool { return false } +// LinuxResources return the containers Linux Resources (if any) +func (c *Container) LinuxResources() *spec.LinuxResources { + if c.config.Spec != nil && c.config.Spec.Linux != nil { + return c.config.Spec.Linux.Resources + } + return nil +} + // State Accessors // Require locking diff --git a/libpod/kube.go b/libpod/kube.go index b8047a844f..4be2d107ce 100644 --- a/libpod/kube.go +++ b/libpod/kube.go @@ -700,10 +700,10 @@ func containerToV1Container(ctx context.Context, c *Container) (v1.Container, [] kubeContainer.StdinOnce = false kubeContainer.TTY = c.Terminal() - if c.config.Spec.Linux != nil && - c.config.Spec.Linux.Resources != nil { - if c.config.Spec.Linux.Resources.Memory != nil && - c.config.Spec.Linux.Resources.Memory.Limit != nil { + resources := c.LinuxResources() + if resources != nil { + if resources.Memory != nil && + resources.Memory.Limit != nil { if kubeContainer.Resources.Limits == nil { kubeContainer.Resources.Limits = v1.ResourceList{} } @@ -713,11 +713,11 @@ func containerToV1Container(ctx context.Context, c *Container) (v1.Container, [] kubeContainer.Resources.Limits[v1.ResourceMemory] = *qty } - if c.config.Spec.Linux.Resources.CPU != nil && - c.config.Spec.Linux.Resources.CPU.Quota != nil && - c.config.Spec.Linux.Resources.CPU.Period != nil { - quota := *c.config.Spec.Linux.Resources.CPU.Quota - period := *c.config.Spec.Linux.Resources.CPU.Period + if resources.CPU != nil && + resources.CPU.Quota != nil && + resources.CPU.Period != nil { + quota := *resources.CPU.Quota + period := *resources.CPU.Period if quota > 0 && period > 0 { cpuLimitMilli := int64(1000 * util.PeriodAndQuotaToCores(period, quota)) diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go index 0964d4ea37..5740d70664 100644 --- a/libpod/oci_conmon_linux.go +++ b/libpod/oci_conmon_linux.go @@ -133,8 +133,7 @@ func (r *ConmonOCIRuntime) moveConmonToCgroupAndSignal(ctr *Container, cmd *exec // there are only 2 valid cgroup managers cgroupParent := ctr.CgroupParent() cgroupPath := filepath.Join(ctr.config.CgroupParent, "conmon") - Resource := ctr.Spec().Linux.Resources - cgroupResources, err := GetLimits(Resource) + cgroupResources, err := GetLimits(ctr.LinuxResources()) if err != nil { logrus.StandardLogger().Log(logLevel, "Could not get ctr resources") } diff --git a/libpod/stats_freebsd.go b/libpod/stats_freebsd.go index 53bc3f19a8..be22e86184 100644 --- a/libpod/stats_freebsd.go +++ b/libpod/stats_freebsd.go @@ -100,9 +100,9 @@ func (c *Container) getPlatformContainerStats(stats *define.ContainerStats, prev func (c *Container) getMemLimit() uint64 { memLimit := uint64(math.MaxUint64) - if c.config.Spec.Linux != nil && c.config.Spec.Linux.Resources != nil && - c.config.Spec.Linux.Resources.Memory != nil && c.config.Spec.Linux.Resources.Memory.Limit != nil { - memLimit = uint64(*c.config.Spec.Linux.Resources.Memory.Limit) + resources := c.LinuxResources() + if resources != nil && resources.Memory != nil && resources.Memory.Limit != nil { + memLimit = uint64(*resources.Memory.Limit) } mi, err := system.ReadMemInfo() diff --git a/libpod/stats_linux.go b/libpod/stats_linux.go index ad8f33c91d..aad9d70516 100644 --- a/libpod/stats_linux.go +++ b/libpod/stats_linux.go @@ -86,9 +86,9 @@ func (c *Container) getPlatformContainerStats(stats *define.ContainerStats, prev func (c *Container) getMemLimit() uint64 { memLimit := uint64(math.MaxUint64) - if c.config.Spec.Linux != nil && c.config.Spec.Linux.Resources != nil && - c.config.Spec.Linux.Resources.Memory != nil && c.config.Spec.Linux.Resources.Memory.Limit != nil { - memLimit = uint64(*c.config.Spec.Linux.Resources.Memory.Limit) + resources := c.LinuxResources() + if resources != nil && resources.Memory != nil && resources.Memory.Limit != nil { + memLimit = uint64(*resources.Memory.Limit) } si := &syscall.Sysinfo_t{} diff --git a/pkg/api/handlers/compat/containers_stats.go b/pkg/api/handlers/compat/containers_stats.go index 5196616754..848252a766 100644 --- a/pkg/api/handlers/compat/containers_stats.go +++ b/pkg/api/handlers/compat/containers_stats.go @@ -134,10 +134,10 @@ streamLabel: // A label to flatten the scope InstanceID: "", } - cfg := ctnr.Config() + resources := ctnr.LinuxResources() memoryLimit := cgroupStat.MemoryStats.Usage.Limit - if cfg.Spec.Linux != nil && cfg.Spec.Linux.Resources != nil && cfg.Spec.Linux.Resources.Memory != nil && *cfg.Spec.Linux.Resources.Memory.Limit > 0 { - memoryLimit = uint64(*cfg.Spec.Linux.Resources.Memory.Limit) + if resources != nil && resources.Memory != nil && *resources.Memory.Limit > 0 { + memoryLimit = uint64(*resources.Memory.Limit) } memInfo, err := system.ReadMemInfo() From af38c79e36ef0c92e4273f41933de8b4b8b92b8e Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Wed, 12 Oct 2022 10:18:07 +0200 Subject: [PATCH 2/4] Avoid unnecessary calls to Container.Spec() This call does a deep copy, which is only needed if you want to modify the return value. Instead we use ctr.ConfigNoCopy().Spec which is just a pointer dereference. [NO NEW TESTS NEEDED] Just minor performance effects Signed-off-by: Alexander Larsson --- pkg/specgen/generate/container.go | 2 +- pkg/specgen/generate/storage.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/specgen/generate/container.go b/pkg/specgen/generate/container.go index 736fa5ab3c..834345f4c8 100644 --- a/pkg/specgen/generate/container.go +++ b/pkg/specgen/generate/container.go @@ -474,7 +474,7 @@ func ConfigToSpec(rt *libpod.Runtime, specg *specgen.SpecGenerator, contaierID s } } specg.OverlayVolumes = overlay - _, mounts := c.SortUserVolumes(c.Spec()) + _, mounts := c.SortUserVolumes(c.ConfigNoCopy().Spec) specg.Mounts = mounts specg.HostDeviceList = conf.DeviceHostSrc specg.Networks = conf.Networks diff --git a/pkg/specgen/generate/storage.go b/pkg/specgen/generate/storage.go index c3cd61b36f..b6b80def4d 100644 --- a/pkg/specgen/generate/storage.go +++ b/pkg/specgen/generate/storage.go @@ -288,7 +288,7 @@ func getVolumesFrom(volumesFrom []string, runtime *libpod.Runtime) (map[string]s // Now we get the container's spec and loop through its volumes // and append them in if we can find them. - spec := ctr.Spec() + spec := ctr.ConfigNoCopy().Spec if spec == nil { return nil, nil, fmt.Errorf("retrieving container %s spec for volumes-from", ctr.ID()) } From 03c5f9d02f9301923f103ead740e6a009b4879c0 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Wed, 12 Oct 2022 10:28:09 +0200 Subject: [PATCH 3/4] Container filters: Avoid use of ctr.Config() This is a very expensive function as it does a deep copy. Instead use pre-existing accessors like ctr.CreatedTime() where they exist and ctr.ConfigNoCopy() where not. [NO NEW TESTS NEEDED] Just minor performance effects Signed-off-by: Alexander Larsson --- pkg/domain/filters/containers.go | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/pkg/domain/filters/containers.go b/pkg/domain/filters/containers.go index de62b65824..917f5d61bf 100644 --- a/pkg/domain/filters/containers.go +++ b/pkg/domain/filters/containers.go @@ -84,19 +84,19 @@ func GenerateContainerFilterFuncs(filter string, filterValues []string, r *libpo // - ancestor=([:tag]|| ⟨image@digest⟩) - containers created from an image or a descendant. return func(c *libpod.Container) bool { for _, filterValue := range filterValues { - containerConfig := c.Config() + rootfsImageID, rootfsImageName := c.Image() var imageTag string var imageNameWithoutTag string // Compare with ImageID, ImageName // Will match ImageName if running image has tag latest for other tags exact complete filter must be given - imageNameSlice := strings.SplitN(containerConfig.RootfsImageName, ":", 2) + imageNameSlice := strings.SplitN(rootfsImageName, ":", 2) if len(imageNameSlice) == 2 { imageNameWithoutTag = imageNameSlice[0] imageTag = imageNameSlice[1] } - if (containerConfig.RootfsImageID == filterValue) || - (containerConfig.RootfsImageName == filterValue) || + if (rootfsImageID == filterValue) || + (rootfsImageName == filterValue) || (imageNameWithoutTag == filterValue && imageTag == "latest") { return true } @@ -110,14 +110,12 @@ func GenerateContainerFilterFuncs(filter string, filterValues []string, r *libpo if err != nil { return nil, err } - containerConfig := ctr.Config() - if createTime.IsZero() || createTime.After(containerConfig.CreatedTime) { - createTime = containerConfig.CreatedTime + if createTime.IsZero() || createTime.After(ctr.CreatedTime()) { + createTime = ctr.CreatedTime() } } return func(c *libpod.Container) bool { - cc := c.Config() - return createTime.After(cc.CreatedTime) + return createTime.After(c.CreatedTime()) }, nil case "since": var createTime time.Time @@ -126,19 +124,17 @@ func GenerateContainerFilterFuncs(filter string, filterValues []string, r *libpo if err != nil { return nil, err } - containerConfig := ctr.Config() - if createTime.IsZero() || createTime.After(containerConfig.CreatedTime) { - createTime = containerConfig.CreatedTime + if createTime.IsZero() || createTime.After(ctr.CreatedTime()) { + createTime = ctr.CreatedTime() } } return func(c *libpod.Container) bool { - cc := c.Config() - return createTime.Before(cc.CreatedTime) + return createTime.Before(c.CreatedTime()) }, nil case "volume": //- volume=(|) return func(c *libpod.Container) bool { - containerConfig := c.Config() + containerConfig := c.ConfigNoCopy() var dest string for _, filterValue := range filterValues { arr := strings.SplitN(filterValue, ":", 2) From d08b4c1339c123555c2da0cb1863f36196d042b7 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Wed, 12 Oct 2022 10:33:24 +0200 Subject: [PATCH 4/4] ContainerEngine.SetupRootless(): Avoid calling container.Config() This is a very expensive call as it deep duplicates the Config, and we just need to read a single member, so use ConfigNoCopy() instead. [NO NEW TESTS NEEDED] Just minor performance effects Signed-off-by: Alexander Larsson --- pkg/domain/infra/abi/system.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/domain/infra/abi/system.go b/pkg/domain/infra/abi/system.go index da903df9e9..6dd3165e11 100644 --- a/pkg/domain/infra/abi/system.go +++ b/pkg/domain/infra/abi/system.go @@ -136,7 +136,7 @@ func (ic *ContainerEngine) SetupRootless(_ context.Context, noMoveProcess bool) paths := []string{} for _, ctr := range ctrs { - paths = append(paths, ctr.Config().ConmonPidFile) + paths = append(paths, ctr.ConfigNoCopy().ConmonPidFile) } if len(paths) > 0 {