Skip to content

Some fixes about memory stats #418

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

Closed
wants to merge 4 commits into from
Closed

Some fixes about memory stats #418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b6fc536
Refactor GetStats for memory
hqhq Nov 25, 2015
ce996ab
Add oom stat to MemoryStats
hqhq Nov 25, 2015
eb4da49
Fix description about docker events
hqhq Nov 25, 2015
462204d
Omit Failcnt in json
hqhq Nov 26, 2015
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
2 changes: 1 addition & 1 deletion events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ type event struct {

var eventsCommand = cli.Command{
Name: "events",
Usage: "display container events such as OOM notifications, cpu, memory, IO and network stats",
Usage: "display container events such as OOM, cpu, memory, IO and network stats",
Flags: []cli.Flag{
cli.DurationFlag{Name: "interval", Value: 5 * time.Second, Usage: "set the stats collection interval"},
cli.BoolFlag{Name: "stats", Usage: "display the container's stats then exit"},
Expand Down
68 changes: 48 additions & 20 deletions libcontainer/cgroups/fs/memory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,41 +99,35 @@ func (s *MemoryGroup) Remove(d *cgroupData) error {
}

func (s *MemoryGroup) GetStats(path string, stats *cgroups.Stats) error {
// Set stats from memory.stat.
statsFile, err := os.Open(filepath.Join(path, "memory.stat"))
memoryStat, err := getMemoryStat(path)
if err != nil {
if os.IsNotExist(err) {
return nil
}
return err
}
defer statsFile.Close()

sc := bufio.NewScanner(statsFile)
for sc.Scan() {
t, v, err := getCgroupParamKeyValue(sc.Text())
if err != nil {
return fmt.Errorf("failed to parse memory.stat (%q) - %v", sc.Text(), err)
}
stats.MemoryStats.Stats[t] = v
}
stats.MemoryStats.Cache = stats.MemoryStats.Stats["cache"]

memoryUsage, err := getMemoryData(path, "")
if err != nil {
return err
}
stats.MemoryStats.Usage = memoryUsage
swapUsage, err := getMemoryData(path, "memsw")
if err != nil {
return err
}
stats.MemoryStats.SwapUsage = swapUsage
kernelUsage, err := getMemoryData(path, "kmem")
if err != nil {
return err
}
stats.MemoryStats.KernelUsage = kernelUsage
oomStat, err := getOomStat(path)
if err != nil {
return err
}

stats.MemoryStats = cgroups.MemoryStats{
Stats: memoryStat,
Cache: memoryStat["cache"],
Usage: memoryUsage,
SwapUsage: swapUsage,
KernelUsage: kernelUsage,
OomStat: oomStat,
}

return nil
}
Expand All @@ -147,6 +141,40 @@ func memoryAssigned(cgroup *configs.Cgroup) bool {
cgroup.MemorySwappiness != -1
}

func getMemoryMappingData(path, name string) (map[string]uint64, error) {
data := make(map[string]uint64)

file, err := os.Open(filepath.Join(path, name))
if err != nil {
if os.IsNotExist(err) {
return nil, nil
}
return nil, err
}
defer file.Close()

sc := bufio.NewScanner(file)
for sc.Scan() {
t, v, err := getCgroupParamKeyValue(sc.Text())
if err != nil {
return nil, fmt.Errorf("failed to parse %s (%q) - %v", name, sc.Text(), err)
}
data[t] = v
}

return data, nil
}

func getMemoryStat(path string) (map[string]uint64, error) {
// Get stats from memory.stat.
return getMemoryMappingData(path, "memory.stat")
}

func getOomStat(path string) (map[string]uint64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really a stat. Why are we overloading the stats API to essentially abstract out cgroups API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vishh Why do you think oomStat is not really a stat? From user's point of view, I think it surely is a stat which is also very concerned by users, and all this PR was inspired by help message of runc events

$ runc events --help
NAME:
   events - display container events such as OOM notifications, cpu, memory, IO and network stats

USAGE:
   command events [command options] [arguments...]

OPTIONS:
   --interval "5s"      set the stats collection interval
   --stats              display the container's stats then exit

Which I noticed we don't have oom stats yet but it says we do. (I think original description OOM notifications stats was meat to be OOM stats)

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally oom_control feels more like Status rather than Stats.

On Sun, Dec 20, 2015 at 11:37 PM, Qiang Huang [email protected]
wrote:

In libcontainer/cgroups/fs/memory.go
#418 (comment):

  •   t, v, err := getCgroupParamKeyValue(sc.Text())

  •   if err != nil {

  •       return nil, fmt.Errorf("failed to parse %s (%q) - %v", name, sc.Text(), err)

  •   }

  •   data[t] = v
  • }
  • return data, nil
    +}

+func getMemoryStat(path string) (map[string]uint64, error) {

  • // Get stats from memory.stat.
  • return getMemoryMappingData(path, "memory.stat")
    +}

+func getOomStat(path string) (map[string]uint64, error) {

@vishh https://github.com/vishh Why do you think oomStat is not really
a stat? From user's point of view, I think it surely is a stat which is
also very concerned by users, and all this PR was inspired by help message
of runc events

$ runc events --help
NAME:
events - display container events such as OOM notifications, cpu, memory, IO and network stats

USAGE:
command events [command options] [arguments...]

OPTIONS:
--interval "5s" set the stats collection interval
--stats display the container's stats then exit

Which I noticed we don't have oom stats yet but it says we do. (I think
original description OOM notifications stats was meat to be OOM stats)

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/runc/pull/418/files#r48122236.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I understand, since it's a yes or no, not a number, it looks more like Status, but it's not the same thing as the states we are implementing in #311

So we end up facing states, status, statistic and events. I don't feel good to mix events and statistic into runc events, and it seems to be worse to add status in. Do you think we should separate them all? That would be a lot more commands but seems unavoidable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we start with separating the CLI from the library APIs. For the library, I'd prefer separating Status, Spec, Stats & Events.
For the CLI, we can choose to either expose all these entities separately or have a get that exposes minimal information and a detail or describe option that exposes all the above.

// Get oom stats from memory.oom_control.
return getMemoryMappingData(path, "memory.oom_control")
}

func getMemoryData(path, name string) (cgroups.MemoryData, error) {
memoryData := cgroups.MemoryData{}

Expand Down
94 changes: 91 additions & 3 deletions libcontainer/cgroups/fs/memory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ import (
const (
memoryStatContents = `cache 512
rss 1024`
memoryUsageContents = "2048\n"
memoryMaxUsageContents = "4096\n"
memoryFailcnt = "100\n"
memoryUsageContents = "2048\n"
memoryMaxUsageContents = "4096\n"
memoryFailcnt = "100\n"
memoryOomControlContents = `oom_kill_disable 1
under_oom 0`
)

func TestMemorySetMemory(t *testing.T) {
Expand Down Expand Up @@ -173,6 +175,8 @@ func TestMemoryStatsNoStatFile(t *testing.T) {
helper.writeFileContents(map[string]string{
"memory.usage_in_bytes": memoryUsageContents,
"memory.max_usage_in_bytes": memoryMaxUsageContents,
"memory.failcnt": memoryFailcnt,
"memory.oom_control": memoryOomControlContents,
})

memory := &MemoryGroup{}
Expand All @@ -189,6 +193,8 @@ func TestMemoryStatsNoUsageFile(t *testing.T) {
helper.writeFileContents(map[string]string{
"memory.stat": memoryStatContents,
"memory.max_usage_in_bytes": memoryMaxUsageContents,
"memory.failcnt": memoryFailcnt,
"memory.oom_control": memoryOomControlContents,
})

memory := &MemoryGroup{}
Expand All @@ -205,6 +211,8 @@ func TestMemoryStatsNoMaxUsageFile(t *testing.T) {
helper.writeFileContents(map[string]string{
"memory.stat": memoryStatContents,
"memory.usage_in_bytes": memoryUsageContents,
"memory.failcnt": memoryFailcnt,
"memory.oom_control": memoryOomControlContents,
})

memory := &MemoryGroup{}
Expand All @@ -215,13 +223,51 @@ func TestMemoryStatsNoMaxUsageFile(t *testing.T) {
}
}

func TestMemoryStatsNoFailcntFile(t *testing.T) {
helper := NewCgroupTestUtil("memory", t)
defer helper.cleanup()
helper.writeFileContents(map[string]string{
"memory.stat": memoryStatContents,
"memory.usage_in_bytes": memoryUsageContents,
"memory.max_usage_in_bytes": memoryMaxUsageContents,
"memory.oom_control": memoryOomControlContents,
})

memory := &MemoryGroup{}
actualStats := *cgroups.NewStats()
err := memory.GetStats(helper.CgroupPath, &actualStats)
if err == nil {
t.Fatal("Expected failure")
}
}

func TestMemoryStatsNoOomControlFile(t *testing.T) {
helper := NewCgroupTestUtil("memory", t)
defer helper.cleanup()
helper.writeFileContents(map[string]string{
"memory.stat": memoryStatContents,
"memory.usage_in_bytes": memoryUsageContents,
"memory.max_usage_in_bytes": memoryMaxUsageContents,
"memory.failcnt": memoryFailcnt,
})

memory := &MemoryGroup{}
actualStats := *cgroups.NewStats()
err := memory.GetStats(helper.CgroupPath, &actualStats)
if err != nil {
t.Fatal(err)
}
}

func TestMemoryStatsBadStatFile(t *testing.T) {
helper := NewCgroupTestUtil("memory", t)
defer helper.cleanup()
helper.writeFileContents(map[string]string{
"memory.stat": "rss rss",
"memory.usage_in_bytes": memoryUsageContents,
"memory.max_usage_in_bytes": memoryMaxUsageContents,
"memory.failcnt": memoryFailcnt,
"memory.oom_control": memoryOomControlContents,
})

memory := &MemoryGroup{}
Expand All @@ -239,6 +285,8 @@ func TestMemoryStatsBadUsageFile(t *testing.T) {
"memory.stat": memoryStatContents,
"memory.usage_in_bytes": "bad",
"memory.max_usage_in_bytes": memoryMaxUsageContents,
"memory.failcnt": memoryFailcnt,
"memory.oom_control": memoryOomControlContents,
})

memory := &MemoryGroup{}
Expand All @@ -256,6 +304,46 @@ func TestMemoryStatsBadMaxUsageFile(t *testing.T) {
"memory.stat": memoryStatContents,
"memory.usage_in_bytes": memoryUsageContents,
"memory.max_usage_in_bytes": "bad",
"memory.failcnt": memoryFailcnt,
"memory.oom_control": memoryOomControlContents,
})

memory := &MemoryGroup{}
actualStats := *cgroups.NewStats()
err := memory.GetStats(helper.CgroupPath, &actualStats)
if err == nil {
t.Fatal("Expected failure")
}
}

func TestMemoryStatsBadFailcntFile(t *testing.T) {
helper := NewCgroupTestUtil("memory", t)
defer helper.cleanup()
helper.writeFileContents(map[string]string{
"memory.stat": memoryStatContents,
"memory.usage_in_bytes": memoryUsageContents,
"memory.max_usage_in_bytes": memoryMaxUsageContents,
"memory.failcnt": "bad",
"memory.oom_control": memoryOomControlContents,
})

memory := &MemoryGroup{}
actualStats := *cgroups.NewStats()
err := memory.GetStats(helper.CgroupPath, &actualStats)
if err == nil {
t.Fatal("Expected failure")
}
}

func TestMemoryStatsBadOomControlFile(t *testing.T) {
helper := NewCgroupTestUtil("memory", t)
defer helper.cleanup()
helper.writeFileContents(map[string]string{
"memory.stat": memoryStatContents,
"memory.usage_in_bytes": memoryUsageContents,
"memory.max_usage_in_bytes": memoryMaxUsageContents,
"memory.failcnt": memoryFailcnt,
"memory.oom_control": "bad bad",
})

memory := &MemoryGroup{}
Expand Down
13 changes: 8 additions & 5 deletions libcontainer/cgroups/stats.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type CpuStats struct {
type MemoryData struct {
Usage uint64 `json:"usage,omitempty"`
MaxUsage uint64 `json:"max_usage,omitempty"`
Failcnt uint64 `json:"failcnt"`
Failcnt uint64 `json:"failcnt,omitempty"`
}
type MemoryStats struct {
// memory used for cache
Expand All @@ -44,9 +44,12 @@ type MemoryStats struct {
Usage MemoryData `json:"usage,omitempty"`
// usage of memory + swap
SwapUsage MemoryData `json:"swap_usage,omitempty"`
// usafe of kernel memory
KernelUsage MemoryData `json:"kernel_usage,omitempty"`
Stats map[string]uint64 `json:"stats,omitempty"`
// usage of kernel memory
KernelUsage MemoryData `json:"kernel_usage,omitempty"`
// status of memory usage
Stats map[string]uint64 `json:"stats,omitempty"`
// status of oom
OomStat map[string]uint64 `json:"oom_stat,omitempry"`
}

type BlkioStatEntry struct {
Expand Down Expand Up @@ -74,7 +77,7 @@ type HugetlbStats struct {
// maximum usage ever recorded.
MaxUsage uint64 `json:"max_usage,omitempty"`
// number of times htgetlb usage allocation failure.
Failcnt uint64 `json:"failcnt"`
Failcnt uint64 `json:"failcnt,omitempty"`
}

type Stats struct {
Expand Down