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

Improve performance by for pid stats (cgroups1) re-using readuint #291

Merged
merged 1 commit into from
Jun 2, 2023
Merged

Improve performance by for pid stats (cgroups1) re-using readuint #291

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
9 changes: 1 addition & 8 deletions cgroup1/pids.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"os"
"path/filepath"
"strconv"
"strings"

v1 "github.com/containerd/cgroups/v3/cgroup1/stats"
specs "github.com/opencontainers/runtime-spec/specs-go"
Expand Down Expand Up @@ -67,16 +66,10 @@ func (p *pidsController) Stat(path string, stats *v1.Metrics) error {
if err != nil {
return err
}
var max uint64
maxData, err := os.ReadFile(filepath.Join(p.Path(path), "pids.max"))
max, err := readUint(filepath.Join(p.Path(path), "pids.max"))
Copy link
Member

@dcantah dcantah Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't the behavior different here? readUint should fail if "max" exists in the file as it only knows how to parse integers, so we'd bail on line 71. Before we'd carry on to filling in the stats, granted the max variable will be left as 0 it seems if "max" was in pids.max, but point being this method would still succeed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right; I changed readUint to return MaxUint64 now. The behavior is still different but it is consistent with cgroups2. I think this change should be fine now.

Copy link
Member

Choose a reason for hiding this comment

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

Yea it is odd to me how we returned 0 here before.. would need to see if any users relied on this behavior with a github search maybe..

Copy link
Member

@dcantah dcantah Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

(I think returning MaxUint64 makes sense, but.. did anyone rely on this? It being 0 I mean, might wanna stay safe)

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 it makes sense at least for v3; v1 is imported by so many packages that I am not confident.

  1. https://pkg.go.dev/github.com/containerd/cgroups?tab=importedby
  2. https://pkg.go.dev/github.com/containerd/cgroups/v3@v3.0.1?tab=importedby

Thinking of reverting back to 0 and adding a comment to say that we preserve it for backward compatibility.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I'd err on the side of staying safe, so your reasoning seems sane and I agree (also sorry for the radio silence here 🫠)

Copy link
Member

Choose a reason for hiding this comment

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

I suppose 0 could be interpreted as no limit, so that might've been the rationale

Copy link
Contributor

Choose a reason for hiding this comment

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

Zero wouldn't be a valid value anyway as zero, so that's a fair rationale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dcantah No worries! thanks for the review! this was a really fun exercise :)
thanks @mmerkes for the review and helping building understanding

if err != nil {
return err
}
if maxS := strings.TrimSpace(string(maxData)); maxS != "max" {
if max, err = parseUint(maxS, 10, 64); err != nil {
return err
}
}
stats.Pids = &v1.PidsStat{
Current: current,
Limit: max,
Expand Down
47 changes: 47 additions & 0 deletions cgroup1/pids_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,3 +186,50 @@ func TestPidsOverflowMax(t *testing.T) {
t.Fatal("expected not nil err")
}
}

func BenchmarkTestPids(b *testing.B) {

mock, err := newMock(b)
if err != nil {
b.Fatal(err)
}
defer func() {
if err := mock.delete(); err != nil {
b.Errorf("failed delete: %v", err)
}
}()

pids := NewPids(mock.root)
if pids == nil {
b.Fatal("pids is nil")
}
resources := specs.LinuxResources{
Pids: &specs.LinuxPids{
Limit: 10,
},
}

err = pids.Create("test", &resources)
if err != nil {
b.Fatal(err)
}

current := filepath.Join(mock.root, "pids", "test", "pids.current")
if err = os.WriteFile(
current,
[]byte(strconv.Itoa(5)),
defaultFilePerm,
); err != nil {
b.Fatal(err)
}

b.ReportAllocs()

for i := 0; i < b.N; i++ {
metrics := v1.Metrics{}
err = pids.Stat("test", &metrics)
if err != nil {
b.Fatal(err)
}
}
}
13 changes: 10 additions & 3 deletions cgroup1/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package cgroup1

import (
"bufio"
"bytes"
"fmt"
"os"
"path/filepath"
Expand Down Expand Up @@ -137,13 +138,19 @@ func readUint(path string) (uint64, error) {
}
defer f.Close()

b := make([]byte, 128) // Chose 128 as some files have leading/trailing whitespaces and alignment
// We should only need 20 bytes for the max uint64, but for a nice power of 2
// lets use 32.
Comment on lines +141 to +142
Copy link
Contributor

Choose a reason for hiding this comment

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

Per the original comment, is it realistic that this might have leading/trailing whitespace? Seems unlikely, but anyone who does would start breaking if they had a lot of it.

Copy link
Member

Choose a reason for hiding this comment

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

It seems the files simply have one line feed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess trailing whitespace isn't actually an issue because it will only read len(b) bytes and stop anyway, so the only way to break this would be to have a bunch of leading whitespace, which seems unlikely in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Right, it's mostly irrelevant if we only read a fixed size of bytes that will always be larger than a uint64's length. The string gets trimmed and all that's left is what we care about.

b := make([]byte, 32)
n, err := f.Read(b)
if err != nil {
return 0, err
}

return parseUint(strings.TrimSpace(string(b[:n])), 10, 64)
s := string(bytes.TrimSpace(b[:n]))
if s == "max" {
// Return 0 for the max value to maintain backward compatibility.
return 0, nil
}
return parseUint(s, 10, 64)
}

func parseUint(s string, base, bitSize int) (uint64, error) {
Expand Down
19 changes: 19 additions & 0 deletions cgroup1/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package cgroup1

import (
"os"
"path/filepath"
"testing"
)

Expand All @@ -30,3 +32,20 @@ func BenchmarkReaduint64(b *testing.B) {
}
}
}

func TestReadUint(t *testing.T) {
tDir := t.TempDir()
pidsmax := filepath.Join(tDir, "pids.max")
err := os.WriteFile(pidsmax, []byte("max"), 0644)
if err != nil {
t.Fatal(err)
}
max, err := readUint(pidsmax)
if err != nil {
t.Fatal(err)
}
// test for backwards compatibility
if max != 0 {
t.Fail()
}
}