-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Remove kmem Initialization check while setting memory configuration #962
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,15 +5,19 @@ package fs | |
import ( | ||
"bufio" | ||
"fmt" | ||
"math" | ||
"io/ioutil" | ||
"os" | ||
"path/filepath" | ||
"strconv" | ||
"strings" | ||
"syscall" | ||
|
||
"github.com/opencontainers/runc/libcontainer/cgroups" | ||
"github.com/opencontainers/runc/libcontainer/configs" | ||
"github.com/opencontainers/runc/libcontainer/system" | ||
) | ||
|
||
const ( | ||
cgroupKernelMemoryLimit = "memory.kmem.limit_in_bytes" | ||
) | ||
|
||
type MemoryGroup struct { | ||
|
@@ -34,9 +38,7 @@ func (s *MemoryGroup) Apply(d *cgroupData) (err error) { | |
return err | ||
} | ||
} | ||
// We have to set kernel memory here, as we can't change it once | ||
// processes have been attached to the cgroup. | ||
if err := s.SetKernelMemory(path, d.config); err != nil { | ||
if err := EnableKernelMemoryAccounting(path); err != nil { | ||
return err | ||
} | ||
} | ||
|
@@ -55,38 +57,43 @@ func (s *MemoryGroup) Apply(d *cgroupData) (err error) { | |
return nil | ||
} | ||
|
||
func (s *MemoryGroup) SetKernelMemory(path string, cgroup *configs.Cgroup) error { | ||
// This has to be done separately because it has special | ||
// constraints (it can only be initialized before setting up a | ||
// hierarchy or adding a task to the cgroups. However, if | ||
// sucessfully initialized, it can be updated anytime afterwards) | ||
if cgroup.Resources.KernelMemory != 0 { | ||
kmemInitialized := false | ||
// Is kmem.limit_in_bytes already set? | ||
kmemValue, err := getCgroupParamUint(path, "memory.kmem.limit_in_bytes") | ||
if err != nil { | ||
return err | ||
} | ||
switch system.GetLongBit() { | ||
case 32: | ||
kmemInitialized = uint32(kmemValue) != uint32(math.MaxUint32) | ||
case 64: | ||
kmemInitialized = kmemValue != uint64(math.MaxUint64) | ||
} | ||
if !kmemInitialized { | ||
// If there's already tasks in the cgroup, we can't change the limit either | ||
tasks, err := getCgroupParamString(path, "tasks") | ||
if err != nil { | ||
return err | ||
} | ||
if tasks != "" { | ||
return fmt.Errorf("cannot set kmem.limit_in_bytes after task have joined this cgroup") | ||
} | ||
} | ||
func EnableKernelMemoryAccounting(path string) error { | ||
// Check if kernel memory is enabled | ||
// We have to limit the kernel memory here as it won't be accounted at all | ||
// until a limit is set on the cgroup and limit cannot be set once the | ||
// cgroup has children, or if there are already tasks in the cgroup. | ||
kernelMemoryLimit := int64(1) | ||
if err := setKernelMemory(path, kernelMemoryLimit); err != nil { | ||
return err | ||
} | ||
kernelMemoryLimit = int64(-1) | ||
if err := setKernelMemory(path, kernelMemoryLimit); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is the only consumer of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SG. Thanks. |
||
return err | ||
} | ||
return nil | ||
} | ||
|
||
if err := writeFile(path, "memory.kmem.limit_in_bytes", strconv.FormatInt(cgroup.Resources.KernelMemory, 10)); err != nil { | ||
return err | ||
func setKernelMemory(path string, kernelMemoryLimit int64) error { | ||
if path == "" { | ||
return fmt.Errorf("no such directory for %s", cgroupKernelMemoryLimit) | ||
} | ||
if !cgroups.PathExists(filepath.Join(path, cgroupKernelMemoryLimit)) { | ||
// kernel memory is not enabled on the system so we should do nothing | ||
return nil | ||
} | ||
if err := ioutil.WriteFile(filepath.Join(path, cgroupKernelMemoryLimit), []byte(strconv.FormatInt(kernelMemoryLimit, 10)), 0700); err != nil { | ||
// Check if the error number returned by the syscall is "EBUSY" | ||
// The EBUSY signal is returned on attempts to write to the | ||
// memory.kmem.limit_in_bytes file if the cgroup has children or | ||
// once tasks have been attached to the cgroup | ||
if pathErr, ok := err.(*os.PathError); ok { | ||
if errNo, ok := pathErr.Err.(syscall.Errno); ok { | ||
if errNo == syscall.EBUSY { | ||
return fmt.Errorf("failed to set %s, because either tasks have already joined this cgroup or it has children", cgroupKernelMemoryLimit) | ||
} | ||
} | ||
} | ||
return fmt.Errorf("failed to write %v to %v: %v", kernelMemoryLimit, cgroupKernelMemoryLimit, err) | ||
} | ||
return nil | ||
} | ||
|
@@ -139,15 +146,18 @@ func (s *MemoryGroup) Set(path string, cgroup *configs.Cgroup) error { | |
return err | ||
} | ||
|
||
if err := s.SetKernelMemory(path, cgroup); err != nil { | ||
return err | ||
if cgroup.Resources.KernelMemory != 0 { | ||
if err := setKernelMemory(path, cgroup.Resources.KernelMemory); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
if cgroup.Resources.MemoryReservation != 0 { | ||
if err := writeFile(path, "memory.soft_limit_in_bytes", strconv.FormatInt(cgroup.Resources.MemoryReservation, 10)); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
if cgroup.Resources.KernelMemoryTCP != 0 { | ||
if err := writeFile(path, "memory.kmem.tcp.limit_in_bytes", strconv.FormatInt(cgroup.Resources.KernelMemoryTCP, 10)); err != nil { | ||
return err | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say again, we need to update the systemd manager as well. Right now the systemd manager just does this to "set the kernel memory":
This code used to call this function (
apply_raw.go:SetKernelMemory
) directly. It no longer does this, meaning that we're now maintaining two versions of different hacks to enable accounting for cgroups. Can we please fix this (combine the two), or at least verify that it works as intended?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we should fix up systemd cgroup manager as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyphar @mrunalp IIUC, currently on systemd we are just creating the memory cgroup and not setting any kmem limit on them during cgroup creation. And it doesn't look like the intended behaviour.
I will update my patch to have the same logic as cgroup fs in the systemd implementation.
The call to SetKernelMemory was removed in the following PR : #790. Maybe @mlaventure would have more idea as to why that change was made.