Some fixes about memory stats

[hqhq]

• Refactor GetStats for memory
• Add oom stat to MemoryStats
• Fix description about runc events
• Omit Failcnt in json

Signed-off-by: Qiang Huang [email protected]

[vishh]

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

[hqhq]

@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)

[vishh]

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.

[hqhq]

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.

[vishh]

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.

[hqhq]

It's obsolete, closing.