Skip to content

Review suggestions #35

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

Open
thaJeztah wants to merge 6 commits into
base: psi
Choose a base branch
from
Open

Review suggestions #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
43a0111
address review nits
thaJeztah Oct 6, 2022
95e4875
make types.PSIData, types.PSIStats alias for cgroups.PSIData, PSIStats
thaJeztah Oct 6, 2022
fdfc44a
libcontainer/cgroups: make PSIStats.Some, PSIStats.Full a pointer
thaJeztah Oct 6, 2022
6d7ec79
libcontainer/cgroups/fs2: parsePSIData() include field in errors
thaJeztah Oct 6, 2022
ec72c6d
libcontainer/cgroups/fs2: make parsePSIData() less DRY
thaJeztah Oct 6, 2022
6c1e12d
Use pointers for PSU fields to make "omitempty" work
thaJeztah Oct 6, 2022
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
16 changes: 3 additions & 13 deletions events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,6 @@ information is displayed once every 5 seconds.`,
},
}

func convertPSI(from *cgroups.PSIData, to *types.PSIData) {
to.Avg10 = from.Avg10
to.Avg60 = from.Avg60
to.Avg300 = from.Avg300
to.Total = from.Total
}

func convertLibcontainerStats(ls *libcontainer.Stats) *types.Stats {
cg := ls.CgroupStats
if cg == nil {
Expand All @@ -136,8 +129,7 @@ func convertLibcontainerStats(ls *libcontainer.Stats) *types.Stats {
s.CPU.Throttling.Periods = cg.CpuStats.ThrottlingData.Periods
s.CPU.Throttling.ThrottledPeriods = cg.CpuStats.ThrottlingData.ThrottledPeriods
s.CPU.Throttling.ThrottledTime = cg.CpuStats.ThrottlingData.ThrottledTime
convertPSI(&cg.CpuStats.PSI.Some, &s.CPU.PSI.Some)
convertPSI(&cg.CpuStats.PSI.Full, &s.CPU.PSI.Full)
s.CPU.PSI = cg.CpuStats.PSI

s.CPUSet = types.CPUSet(cg.CPUSetStats)

Expand All @@ -147,8 +139,7 @@ func convertLibcontainerStats(ls *libcontainer.Stats) *types.Stats {
s.Memory.Swap = convertMemoryEntry(cg.MemoryStats.SwapUsage)
s.Memory.Usage = convertMemoryEntry(cg.MemoryStats.Usage)
s.Memory.Raw = cg.MemoryStats.Stats
convertPSI(&cg.MemoryStats.PSI.Some, &s.Memory.PSI.Some)
convertPSI(&cg.MemoryStats.PSI.Full, &s.Memory.PSI.Full)
s.CPU.PSI = cg.CpuStats.PSI

s.Blkio.IoServiceBytesRecursive = convertBlkioEntry(cg.BlkioStats.IoServiceBytesRecursive)
s.Blkio.IoServicedRecursive = convertBlkioEntry(cg.BlkioStats.IoServicedRecursive)
Expand All @@ -158,8 +149,7 @@ func convertLibcontainerStats(ls *libcontainer.Stats) *types.Stats {
s.Blkio.IoMergedRecursive = convertBlkioEntry(cg.BlkioStats.IoMergedRecursive)
s.Blkio.IoTimeRecursive = convertBlkioEntry(cg.BlkioStats.IoTimeRecursive)
s.Blkio.SectorsRecursive = convertBlkioEntry(cg.BlkioStats.SectorsRecursive)
convertPSI(&cg.BlkioStats.PSI.Some, &s.Blkio.PSI.Some)
convertPSI(&cg.BlkioStats.PSI.Full, &s.Blkio.PSI.Full)
s.CPU.PSI = cg.CpuStats.PSI

s.Hugetlb = make(map[string]types.Hugetlb)
for k, v := range cg.HugetlbStats {
Expand Down
6 changes: 3 additions & 3 deletions libcontainer/cgroups/fs2/fs2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,13 @@ func (m *manager) GetStats() (*cgroups.Stats, error) {
// open *.pressure file returns
// - ErrNotExist when kernel < 4.20 or CONFIG_PSI is disabled
// - ENOTSUP when we requires psi=1 in kernel command line to enable PSI support
if err := statPSI(m.dirPath, "cpu.pressure", &st.CpuStats.PSI); err != nil && !errors.Is(err, os.ErrNotExist) && !errors.Is(err, syscall.ENOTSUP) {
if err := statPSI(m.dirPath, "cpu.pressure", st.CpuStats.PSI); err != nil && !errors.Is(err, os.ErrNotExist) && !errors.Is(err, syscall.ENOTSUP) {
errs = append(errs, err)
}
if err := statPSI(m.dirPath, "memory.pressure", &st.MemoryStats.PSI); err != nil && !errors.Is(err, os.ErrNotExist) && !errors.Is(err, syscall.ENOTSUP) {
if err := statPSI(m.dirPath, "memory.pressure", st.MemoryStats.PSI); err != nil && !errors.Is(err, os.ErrNotExist) && !errors.Is(err, syscall.ENOTSUP) {
errs = append(errs, err)
}
if err := statPSI(m.dirPath, "io.pressure", &st.BlkioStats.PSI); err != nil && !errors.Is(err, os.ErrNotExist) && !errors.Is(err, syscall.ENOTSUP) {
if err := statPSI(m.dirPath, "io.pressure", st.BlkioStats.PSI); err != nil && !errors.Is(err, os.ErrNotExist) && !errors.Is(err, syscall.ENOTSUP) {
errs = append(errs, err)
}
// hugetlb (since kernel 5.6)
Expand Down
45 changes: 21 additions & 24 deletions libcontainer/cgroups/fs2/psi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,56 +22,53 @@ func statPSI(dirPath string, file string, stats *cgroups.PSIStats) error {
parts := strings.Fields(sc.Text())
switch parts[0] {
case "some":
data, err := parsePSIData(parts[1:])
stats.Some, err = parsePSIData(parts[1:])
if err != nil {
return err
}
stats.Some = data
case "full":
data, err := parsePSIData(parts[1:])
stats.Full, err = parsePSIData(parts[1:])
if err != nil {
return err
}
stats.Full = data
}
}
if err := sc.Err(); err != nil {
err = sc.Err()
if err != nil {

@dqminh dqminh Oct 11, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

do we need this change ?

return &parseError{Path: dirPath, File: file, Err: err}
}
return nil
}

func parsePSIData(psi []string) (data cgroups.PSIData, err error) {
func parsePSIData(psi []string) (*cgroups.PSIData, error) {
var (
data = &cgroups.PSIData{}
err error
)
for _, f := range psi {
kv := strings.SplitN(f, "=", 2)
if len(kv) != 2 {
return data, fmt.Errorf("invalid psi data: %q", f)
return nil, fmt.Errorf("invalid PSI data: %q", f)
}
switch kv[0] {
case "avg10":
v, err := strconv.ParseFloat(kv[1], 64)
if err != nil {
return data, fmt.Errorf("invalid psi value: %q", f)
}
data.Avg10 = v
case "avg60":
case "avg10", "avg60", "avg300":
v, err := strconv.ParseFloat(kv[1], 64)
if err != nil {
return data, fmt.Errorf("invalid psi value: %q", f)
return nil, fmt.Errorf("invalid PSI value (%s): %q", kv[0], f)
}
data.Avg60 = v
case "avg300":
v, err := strconv.ParseFloat(kv[1], 64)
if err != nil {
return data, fmt.Errorf("invalid psi value: %q", f)
switch kv[0] {
case "avg10":
data.Avg10 = v
case "avg60":
data.Avg60 = v
case "avg300":
data.Avg300 = v
}
data.Avg300 = v
case "total":
v, err := strconv.ParseUint(kv[1], 10, 64)
data.Total, err = strconv.ParseUint(kv[1], 10, 64)
if err != nil {
return data, fmt.Errorf("invalid psi value: %q", f)
return nil, fmt.Errorf("invalid PSI value (%s): %q", kv[0], f)
}
data.Total = v
}
}
return data, nil
Expand Down
6 changes: 3 additions & 3 deletions libcontainer/cgroups/fs2/psi_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
const examplePSIData = `some avg10=1.71 avg60=2.36 avg300=2.57 total=230548833
full avg10=1.00 avg60=1.01 avg300=1.00 total=157622356`

func TestStatCPUPsi(t *testing.T) {
func TestStatCPUPSI(t *testing.T) {
// We're using a fake cgroupfs.
cgroups.TestMode = true

Expand All @@ -29,13 +29,13 @@ func TestStatCPUPsi(t *testing.T) {
}

if !reflect.DeepEqual(psi, cgroups.PSIStats{
Some: cgroups.PSIData{
Some: &cgroups.PSIData{
Avg10: 1.71,
Avg60: 2.36,
Avg300: 2.57,
Total: 230548833,
},
Full: cgroups.PSIData{
Full: &cgroups.PSIData{
Avg10: 1.00,
Avg60: 1.01,
Avg300: 1.00,
Expand Down
10 changes: 5 additions & 5 deletions libcontainer/cgroups/stats.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ type PSIData struct {
}

type PSIStats struct {
Some PSIData `json:"some,omitempty"`
Full PSIData `json:"full,omitempty"`
Some *PSIData `json:"some,omitempty"`
Full *PSIData `json:"full,omitempty"`
}

type CpuStats struct {
CpuUsage CpuUsage `json:"cpu_usage,omitempty"`
ThrottlingData ThrottlingData `json:"throttling_data,omitempty"`
PSI PSIStats `json:"psi,omitempty"`
PSI *PSIStats `json:"psi,omitempty"`

@dqminh dqminh Oct 11, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it's preferable to be consistent with the rest, and do the refactoring later.

}

type CPUSetStats struct {
Expand Down Expand Up @@ -102,7 +102,7 @@ type MemoryStats struct {
UseHierarchy bool `json:"use_hierarchy"`

Stats map[string]uint64 `json:"stats,omitempty"`
PSI PSIStats `json:"psi,omitempty"`
PSI *PSIStats `json:"psi,omitempty"`
}

type PageUsageByNUMA struct {
Expand Down Expand Up @@ -147,7 +147,7 @@ type BlkioStats struct {
IoMergedRecursive []BlkioStatEntry `json:"io_merged_recursive,omitempty"`
IoTimeRecursive []BlkioStatEntry `json:"io_time_recursive,omitempty"`
SectorsRecursive []BlkioStatEntry `json:"sectors_recursive,omitempty"`
PSI PSIStats `json:"psi,omitempty"`
PSI *PSIStats `json:"psi,omitempty"`
}

type HugetlbStats struct {
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/helpers.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ function requires() {
psi)
# If PSI is not compiled in the kernel, the file will not exist.
# If PSI is compiled, but not enabled, read will fail with ENOTSUPP.
if [[ ! $(cat /sys/fs/cgroup/cpu.pressure) ]]; then
if ! cat /sys/fs/cgroup/cpu.pressure &>/dev/null; then
skip_me=1
fi
;;
Expand Down
24 changes: 10 additions & 14 deletions types/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package types

import "github.com/opencontainers/runc/libcontainer/intelrdt"
import (
"github.com/opencontainers/runc/libcontainer/cgroups"
"github.com/opencontainers/runc/libcontainer/intelrdt"
)

// Event struct for encoding the event data to json.
type Event struct {
Expand All @@ -21,17 +24,10 @@ type Stats struct {
NetworkInterfaces []*NetworkInterface `json:"network_interfaces"`
}

type PSIData struct {
Avg10 float64 `json:"avg10"`
Avg60 float64 `json:"avg60"`
Avg300 float64 `json:"avg300"`
Total uint64 `json:"total"`
}

type PSIStats struct {
Some PSIData `json:"some,omitempty"`
Full PSIData `json:"full,omitempty"`
}
type (
PSIData = cgroups.PSIData
PSIStats = cgroups.PSIStats
)

@dqminh dqminh Oct 11, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

my impression was that we tried to make types separate from cgroups as much as possible, hence the disconnection and duplication here.

It's reasonable to say that we do not care about the separation (i.e. the types package is not really useful), in that case i would prefer to do the refactoring separately.


type Hugetlb struct {
Usage uint64 `json:"usage,omitempty"`
Expand Down Expand Up @@ -82,7 +78,7 @@ type CpuUsage struct {
type Cpu struct {
Usage CpuUsage `json:"usage,omitempty"`
Throttling Throttling `json:"throttling,omitempty"`
PSI PSIStats `json:"psi,omitempty"`
PSI *PSIStats `json:"psi,omitempty"`

@dqminh dqminh Oct 11, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it's preferable to be consistent with the rest, and do the refactoring later.

}

type CPUSet struct {
Expand Down Expand Up @@ -113,7 +109,7 @@ type Memory struct {
Kernel MemoryEntry `json:"kernel,omitempty"`
KernelTCP MemoryEntry `json:"kernelTCP,omitempty"`
Raw map[string]uint64 `json:"raw,omitempty"`
PSI PSIStats `json:"psi,omitempty"`
PSI *PSIStats `json:"psi,omitempty"`

@dqminh dqminh Oct 11, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it's preferable to be consistent with the rest, and do the refactoring later.

}

type L3CacheInfo struct {
Expand Down