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.

Sign up for GitHub

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

Enable 'podman run --memory-swappiness=0' #12272

Merged
merged 1 commit into from
Nov 13, 2021
Merged

Enable 'podman run --memory-swappiness=0' #12272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion cmd/podman/containers/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,9 @@ func createPodIfNecessary(s *specgen.SpecGenerator, netOpts *entities.NetOptions
return nil, err
}

infraOpts := entities.ContainerCreateOptions{ImageVolume: "bind", Net: netOpts, Quiet: true}
infraOpts := entities.NewInfraContainerCreateOptions()
infraOpts.Net = netOpts
infraOpts.Quiet = true
imageName := config.DefaultInfraImage
podGen.InfraImage = imageName
podGen.InfraContainerSpec = specgen.NewSpecGenerator(imageName, false)
Expand Down
3 changes: 1 addition & 2 deletions cmd/podman/pods/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ var (

var (
createOptions entities.PodCreateOptions
infraOptions entities.ContainerCreateOptions
infraOptions = entities.NewInfraContainerCreateOptions()
infraImage string
labels, labelFile []string
podIDFile string
Expand All @@ -61,7 +61,6 @@ func init() {
})
flags := createCommand.Flags()
flags.SetInterspersed(false)
infraOptions.IsInfra = true
common.DefineCreateFlags(createCommand, &infraOptions, true)
common.DefineNetFlags(createCommand)

Expand Down
6 changes: 4 additions & 2 deletions pkg/api/handlers/libpod/pods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,10 @@ func PodCreate(w http.ResponseWriter, r *http.Request) {
return
}
if !psg.NoInfra {
infraOptions := &entities.ContainerCreateOptions{ImageVolume: "bind", IsInfra: true, Net: &entities.NetOptions{}, Devices: psg.Devices} // options for pulling the image and FillOutSpec
err = specgenutil.FillOutSpecGen(psg.InfraContainerSpec, infraOptions, []string{}) // necessary for default values in many cases (userns, idmappings)
infraOptions := entities.NewInfraContainerCreateOptions() // options for pulling the image and FillOutSpec
infraOptions.Net = &entities.NetOptions{}
infraOptions.Devices = psg.Devices
err = specgenutil.FillOutSpecGen(psg.InfraContainerSpec, &infraOptions, []string{}) // necessary for default values in many cases (userns, idmappings)
if err != nil {
utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrap(err, "error filling out specgen"))
return
Expand Down
9 changes: 9 additions & 0 deletions pkg/domain/entities/pods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,15 @@ type ContainerCreateOptions struct {
CgroupConf []string
}

func NewInfraContainerCreateOptions() ContainerCreateOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of this, but would it make more sense to make this a struct rather than a function? A struct would more closely resemble our other create options while a function more closely represents our specgenerator process. either way this is a good way to differentiate infra.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A new struct for an infra container would be helpful to clarify what parameter is required for creating an infra container. But, I'm not sure how much change is necessary to introduce the new struct. I chose a function to fix this issue with the smaller change.

options := ContainerCreateOptions{
IsInfra: true,
ImageVolume: "bind",
MemorySwappiness: -1,
}
return options
}

type PodCreateReport struct {
Id string //nolint
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/domain/infra/abi/play.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY

if podOpt.Infra {
infraImage := util.DefaultContainerConfig().Engine.InfraImage
infraOptions := entities.ContainerCreateOptions{ImageVolume: "bind"}
infraOptions := entities.NewInfraContainerCreateOptions()
podSpec.PodSpecGen.InfraImage = infraImage
podSpec.PodSpecGen.NoInfra = false
podSpec.PodSpecGen.InfraContainerSpec = specgen.NewSpecGenerator(infraImage, false)
Expand Down
2 changes: 1 addition & 1 deletion pkg/specgenutil/specgen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func getMemoryLimits(s *specgen.SpecGenerator, c *entities.ContainerCreateOption
memory.Kernel = &mk
hasLimits = true
}
if c.MemorySwappiness > 0 {
if c.MemorySwappiness >= 0 {
swappiness := uint64(c.MemorySwappiness)
memory.Swappiness = &swappiness
hasLimits = true
Expand Down
19 changes: 12 additions & 7 deletions test/e2e/run_memory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package integration

import (
"fmt"
"os"
"strconv"

Expand Down Expand Up @@ -67,13 +68,17 @@ var _ = Describe("Podman run memory", func() {
Expect(session.OutputToString()).To(Equal("41943040"))
})

It("podman run memory-swappiness test", func() {
SkipIfCgroupV2("memory-swappiness not supported on cgroupV2")
session := podmanTest.Podman([]string{"run", "--memory-swappiness=15", ALPINE, "cat", "/sys/fs/cgroup/memory/memory.swappiness"})
session.WaitWithDefaultTimeout()
Expect(session).Should(Exit(0))
Expect(session.OutputToString()).To(Equal("15"))
})
for _, limit := range []string{"0", "15", "100"} {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there no way to reverse this syntax? I feel like its better to declare the test once and run the same code with different values inside of the single test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think separated tests makes it easy to find which case fails.

limit := limit // Keep this value in a proper scope
testName := fmt.Sprintf("podman run memory-swappiness test(%s)", limit)
It(testName, func() {
SkipIfCgroupV2("memory-swappiness not supported on cgroupV2")
session := podmanTest.Podman([]string{"run", fmt.Sprintf("--memory-swappiness=%s", limit), ALPINE, "cat", "/sys/fs/cgroup/memory/memory.swappiness"})
session.WaitWithDefaultTimeout()
Expect(session).Should(Exit(0))
Expect(session.OutputToString()).To(Equal(limit))
})
}

It("podman run kernel-memory test", func() {
if podmanTest.Host.Distribution == "ubuntu" {
Expand Down