Skip to content

Commit f13a932

Browse files
committed
libct/cg: HugePageSizes: simplify code and test
1. Instead of distinguishing between errors and warnings, let's treat all errors as warnings, thus simplifying the code. This changes the function behaviour for input like hugepages-BadNumberKb -- previously, the error from Atoi("BadNumber") was considered fatal, now it's just another warnings. 2. Move the warning logging to HugePageSizes, thus simplifying the test case, which no longer needs to read what logrus writes. Note that we do not want to log all the warnings (as chances are very low we'll get any, and if we do this means the code need to be updated), only the first one. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1 parent 39d4c8d commit f13a932

File tree

2 files changed

+35
-30
lines changed

2 files changed

+35
-30
lines changed

libcontainer/cgroups/utils.go

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -319,32 +319,44 @@ func HugePageSizes() []string {
319319
return
320320
}
321321

322-
hugePageSizes, _ = getHugePageSizeFromFilenames(files)
322+
hugePageSizes, err = getHugePageSizeFromFilenames(files)
323+
if err != nil {
324+
logrus.Warn("HugePageSizes: ", err)
325+
}
323326
})
324327

325328
return hugePageSizes
326329
}
327330

328331
func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) {
329332
pageSizes := make([]string, 0, len(fileNames))
333+
var warn error
330334

331335
for _, file := range fileNames {
332336
// example: hugepages-1048576kB
333337
val := strings.TrimPrefix(file, "hugepages-")
334338
if len(val) == len(file) {
335-
// unexpected file name: no prefix found
339+
// Unexpected file name: no prefix found, ignore it.
336340
continue
337341
}
338-
// The suffix is always "kB" (as of Linux 5.9)
342+
// The suffix is always "kB" (as of Linux 5.13). If we find
343+
// something else, produce an error but keep going.
339344
eLen := len(val) - 2
340345
val = strings.TrimSuffix(val, "kB")
341346
if len(val) != eLen {
342-
logrus.Warnf("HugePageSizes: %s: invalid filename suffix (expected \"kB\")", file)
347+
// Highly unlikely.
348+
if warn == nil {
349+
warn = errors.New(file + `: invalid suffix (expected "kB")`)
350+
}
343351
continue
344352
}
345353
size, err := strconv.Atoi(val)
346354
if err != nil {
347-
return nil, err
355+
// Highly unlikely.
356+
if warn == nil {
357+
warn = fmt.Errorf("%s: %w", file, err)
358+
}
359+
continue
348360
}
349361
// Model after https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/hugetlb_cgroup.c?id=eff48ddeab782e35e58ccc8853f7386bbae9dec4#n574
350362
// but in our case the size is in KB already.
@@ -358,7 +370,7 @@ func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) {
358370
pageSizes = append(pageSizes, val)
359371
}
360372

361-
return pageSizes, nil
373+
return pageSizes, warn
362374
}
363375

364376
// GetPids returns all pids, that were added to cgroup at path.

libcontainer/cgroups/utils_test.go

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"testing"
99

1010
"github.com/moby/sys/mountinfo"
11-
"github.com/sirupsen/logrus"
1211
)
1312

1413
const fedoraMountinfo = `15 35 0:3 / /proc rw,nosuid,nodev,noexec,relatime shared:5 - proc proc rw
@@ -467,7 +466,6 @@ func TestGetHugePageSizeImpl(t *testing.T) {
467466
input []string
468467
output []string
469468
isErr bool
470-
isWarn bool
471469
}{
472470
{
473471
input: []string{"hugepages-1048576kB", "hugepages-2048kB", "hugepages-32768kB", "hugepages-64kB"},
@@ -487,25 +485,29 @@ func TestGetHugePageSizeImpl(t *testing.T) {
487485
{ // invalid prefix (silently skipped)
488486
input: []string{"whatever-1024kB"},
489487
},
490-
{ // invalid suffix (skipped with a warning)
491-
input: []string{"hugepages-1024gB"},
492-
isWarn: true,
488+
{ // invalid suffix
489+
input: []string{"hugepages-1024gB"},
490+
isErr: true,
491+
},
492+
{ // no suffix
493+
input: []string{"hugepages-1024"},
494+
isErr: true,
493495
},
494-
{ // no suffix (skipped with a warning)
495-
input: []string{"hugepages-1024"},
496-
isWarn: true,
496+
{ // mixed valid and invalid entries
497+
input: []string{"hugepages-4194304kB", "hugepages-2048kB", "hugepages-akB", "hugepages-64kB"},
498+
output: []string{"4GB", "2MB", "64KB"},
499+
isErr: true,
500+
},
501+
{ // mixed valid and invalid entries
502+
input: []string{"hugepages-2048kB", "hugepages-kB", "hugepages-64kB"},
503+
output: []string{"2MB", "64KB"},
504+
isErr: true,
497505
},
498506
}
499507

500-
// Need to catch warnings.
501-
savedOut := logrus.StandardLogger().Out
502-
defer logrus.SetOutput(savedOut)
503-
warns := new(bytes.Buffer)
504-
logrus.SetOutput(warns)
505-
506508
for _, c := range testCases {
507-
warns.Reset()
508509
output, err := getHugePageSizeFromFilenames(c.input)
510+
t.Log("input:", c.input, "; output:", output, "; err:", err)
509511
if err != nil {
510512
if !c.isErr {
511513
t.Errorf("input %v, expected nil, got error: %v", c.input, err)
@@ -515,15 +517,6 @@ func TestGetHugePageSizeImpl(t *testing.T) {
515517
}
516518
if c.isErr {
517519
t.Errorf("input %v, expected error, got error: nil, output: %v", c.input, output)
518-
// no more checks
519-
continue
520-
}
521-
// check for warnings
522-
if c.isWarn && warns.Len() == 0 {
523-
t.Errorf("input %v, expected a warning, got none", c.input)
524-
}
525-
if !c.isWarn && warns.Len() > 0 {
526-
t.Errorf("input %v, unexpected warning: %s", c.input, warns.String())
527520
}
528521
// check output
529522
if len(output) != len(c.output) || (len(output) > 0 && !reflect.DeepEqual(output, c.output)) {

0 commit comments

Comments
 (0)