From f6a8c9b816927368a7ce96f2451168e562cbdbd3 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 8 May 2024 10:33:04 -0700 Subject: [PATCH 1/5] libct: checkCriuFeatures: return underlying error Signed-off-by: Kir Kolyshkin --- libcontainer/criu_linux.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libcontainer/criu_linux.go b/libcontainer/criu_linux.go index 66178fc16..fd3aa530b 100644 --- a/libcontainer/criu_linux.go +++ b/libcontainer/criu_linux.go @@ -48,8 +48,7 @@ func (c *Container) checkCriuFeatures(criuOpts *CriuOpts, rpcOpts *criurpc.CriuO err := c.criuSwrk(nil, req, criuOpts, nil) if err != nil { - logrus.Debugf("%s", err) - return errors.New("CRIU feature check failed") + return fmt.Errorf("CRIU feature check failed: %w", err) } var missingFeatures []string From e676dac5238377ae868e5ec0318015e0760d3ea4 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 8 May 2024 11:36:57 -0700 Subject: [PATCH 2/5] libct/criu: simplify checkCriuFeatures Since criu 2.12, rpcOpts is not needed when checking criu features. As we requires criu >= 3.0 in Checkpoint, we can remove rpcOpts. Signed-off-by: Kir Kolyshkin --- libcontainer/criu_linux.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/libcontainer/criu_linux.go b/libcontainer/criu_linux.go index fd3aa530b..fda16bead 100644 --- a/libcontainer/criu_linux.go +++ b/libcontainer/criu_linux.go @@ -30,7 +30,7 @@ var criuFeatures *criurpc.CriuFeatures var ErrCriuMissingFeatures = errors.New("criu is missing features") -func (c *Container) checkCriuFeatures(criuOpts *CriuOpts, rpcOpts *criurpc.CriuOpts, criuFeat *criurpc.CriuFeatures) error { +func (c *Container) checkCriuFeatures(criuOpts *CriuOpts, criuFeat *criurpc.CriuFeatures) error { t := criurpc.CriuReqType_FEATURE_CHECK // make sure the features we are looking for are really not from @@ -38,11 +38,7 @@ func (c *Container) checkCriuFeatures(criuOpts *CriuOpts, rpcOpts *criurpc.CriuO criuFeatures = nil req := &criurpc.CriuReq{ - Type: &t, - // Theoretically this should not be necessary but CRIU - // segfaults if Opts is empty. - // Fixed in CRIU 2.12 - Opts: rpcOpts, + Type: &t, Features: criuFeat, } @@ -397,7 +393,7 @@ func (c *Container) Checkpoint(criuOpts *CriuOpts) error { MemTrack: proto.Bool(true), } - if err := c.checkCriuFeatures(criuOpts, &rpcOpts, &feat); err != nil { + if err := c.checkCriuFeatures(criuOpts, &feat); err != nil { return err } @@ -411,7 +407,7 @@ func (c *Container) Checkpoint(criuOpts *CriuOpts) error { feat := criurpc.CriuFeatures{ LazyPages: proto.Bool(true), } - if err := c.checkCriuFeatures(criuOpts, &rpcOpts, &feat); err != nil { + if err := c.checkCriuFeatures(criuOpts, &feat); err != nil { return err } From 62a314656ab6f3b849ab20c0b2ce1336bcdc7f80 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 9 May 2024 13:21:11 -0700 Subject: [PATCH 3/5] libct/int/cpt: simplify test pre-check criu check --feature userns also tests for the /proc/self/ns/user presense, so remove the redundant check, and simplify the error message. Signed-off-by: Kir Kolyshkin --- libcontainer/integration/checkpoint_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/libcontainer/integration/checkpoint_test.go b/libcontainer/integration/checkpoint_test.go index c5426af19..a62e61b3d 100644 --- a/libcontainer/integration/checkpoint_test.go +++ b/libcontainer/integration/checkpoint_test.go @@ -40,12 +40,9 @@ func showFile(t *testing.T, fname string) { } func TestUsernsCheckpoint(t *testing.T) { - if _, err := os.Stat("/proc/self/ns/user"); os.IsNotExist(err) { - t.Skip("Test requires userns.") - } cmd := exec.Command("criu", "check", "--feature", "userns") if err := cmd.Run(); err != nil { - t.Skip("Unable to c/r a container with userns") + t.Skip("Test requires userns") } testCheckpoint(t, true) } From e42d981d6705bb27306e7941cce6e8018f5dfbf0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 8 May 2024 10:25:31 -0700 Subject: [PATCH 4/5] libct/int: rm double logging in checkpoint_test Since commit c77aaa3f the tail of criu log is printed by runc, so there's no need to do the same thing in tests. This also fixes a test failure on ARM where showLog fails (because there's no log file) and thus the conditional t.Skip is not called. Signed-off-by: Kir Kolyshkin --- libcontainer/integration/checkpoint_test.go | 31 --------------------- 1 file changed, 31 deletions(-) diff --git a/libcontainer/integration/checkpoint_test.go b/libcontainer/integration/checkpoint_test.go index a62e61b3d..6c3ddf63b 100644 --- a/libcontainer/integration/checkpoint_test.go +++ b/libcontainer/integration/checkpoint_test.go @@ -1,7 +1,6 @@ package integration import ( - "bufio" "bytes" "errors" "os" @@ -15,30 +14,6 @@ import ( "golang.org/x/sys/unix" ) -func showFile(t *testing.T, fname string) { - t.Helper() - t.Logf("=== %s ===\n", fname) - - f, err := os.Open(fname) - if err != nil { - t.Log(err) - return - } - defer f.Close() //nolint: errcheck - - scanner := bufio.NewScanner(f) - for scanner.Scan() { - t.Log(scanner.Text()) - } - - if err := scanner.Err(); err != nil { - t.Log(err) - return - } - - t.Logf("=== END ===\n") -} - func TestUsernsCheckpoint(t *testing.T) { cmd := exec.Command("criu", "check", "--feature", "userns") if err := cmd.Run(); err != nil { @@ -106,10 +81,8 @@ func testCheckpoint(t *testing.T, userns bool) { WorkDirectory: parentDir, PreDump: true, } - preDumpLog := filepath.Join(preDumpOpts.WorkDirectory, "dump.log") if err := container.Checkpoint(preDumpOpts); err != nil { - showFile(t, preDumpLog) if errors.Is(err, libcontainer.ErrCriuMissingFeatures) { t.Skip(err) } @@ -130,11 +103,8 @@ func testCheckpoint(t *testing.T, userns bool) { WorkDirectory: imagesDir, ParentImage: "../criu-parent", } - dumpLog := filepath.Join(checkpointOpts.WorkDirectory, "dump.log") - restoreLog := filepath.Join(checkpointOpts.WorkDirectory, "restore.log") if err := container.Checkpoint(checkpointOpts); err != nil { - showFile(t, dumpLog) t.Fatal(err) } @@ -168,7 +138,6 @@ func testCheckpoint(t *testing.T, userns bool) { _ = restoreStdinR.Close() defer restoreStdinW.Close() //nolint: errcheck if err != nil { - showFile(t, restoreLog) t.Fatal(err) } From 36be6d0510e7bd28a160545e23dae52907d0e7e9 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 9 May 2024 14:48:31 -0700 Subject: [PATCH 5/5] libct/int: checkpoint test: skip pre-dump if not avail Since we're now testing on ARM, the test case fails when trying to do pre-dump since MemTrack is not available. Skip the pre-dump part if so. This also reverts part of commit 3f4a73d6 as it is no longer needed (now, instead of skipping the whole test, we're just skipping the pre-dump). [Review with --ignore-all-space] Signed-off-by: Kir Kolyshkin --- libcontainer/integration/checkpoint_test.go | 51 +++++++++++---------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/libcontainer/integration/checkpoint_test.go b/libcontainer/integration/checkpoint_test.go index 6c3ddf63b..8d4d6fe47 100644 --- a/libcontainer/integration/checkpoint_test.go +++ b/libcontainer/integration/checkpoint_test.go @@ -2,7 +2,6 @@ package integration import ( "bytes" - "errors" "os" "os/exec" "path/filepath" @@ -14,11 +13,11 @@ import ( "golang.org/x/sys/unix" ) +func criuFeature(feature string) bool { + return exec.Command("criu", "check", "--feature", feature).Run() == nil +} + func TestUsernsCheckpoint(t *testing.T) { - cmd := exec.Command("criu", "check", "--feature", "userns") - if err := cmd.Run(); err != nil { - t.Skip("Test requires userns") - } testCheckpoint(t, true) } @@ -41,6 +40,10 @@ func testCheckpoint(t *testing.T, userns bool) { t.Skip("Test requires criu >= 3.17-4 on CentOS Stream 9.") } + if userns && !criuFeature("userns") { + t.Skip("Test requires userns") + } + config := newTemplateConfig(t, &tParam{userns: userns}) stateDir := t.TempDir() @@ -74,26 +77,28 @@ func testCheckpoint(t *testing.T, userns bool) { ok(t, err) tmp := t.TempDir() + var parentImage string - parentDir := filepath.Join(tmp, "criu-parent") - preDumpOpts := &libcontainer.CriuOpts{ - ImagesDirectory: parentDir, - WorkDirectory: parentDir, - PreDump: true, - } - - if err := container.Checkpoint(preDumpOpts); err != nil { - if errors.Is(err, libcontainer.ErrCriuMissingFeatures) { - t.Skip(err) + // Test pre-dump if mem_dirty_track is available. + if criuFeature("mem_dirty_track") { + parentImage = "../criu-parent" + parentDir := filepath.Join(tmp, "criu-parent") + preDumpOpts := &libcontainer.CriuOpts{ + ImagesDirectory: parentDir, + WorkDirectory: parentDir, + PreDump: true, } - t.Fatal(err) - } - state, err := container.Status() - ok(t, err) + if err := container.Checkpoint(preDumpOpts); err != nil { + t.Fatal(err) + } - if state != libcontainer.Running { - t.Fatal("Unexpected preDump state: ", state) + state, err := container.Status() + ok(t, err) + + if state != libcontainer.Running { + t.Fatal("Unexpected preDump state: ", state) + } } imagesDir := filepath.Join(tmp, "criu") @@ -101,14 +106,14 @@ func testCheckpoint(t *testing.T, userns bool) { checkpointOpts := &libcontainer.CriuOpts{ ImagesDirectory: imagesDir, WorkDirectory: imagesDir, - ParentImage: "../criu-parent", + ParentImage: parentImage, } if err := container.Checkpoint(checkpointOpts); err != nil { t.Fatal(err) } - state, err = container.Status() + state, err := container.Status() ok(t, err) if state != libcontainer.Stopped {