[FIX]: #4012 follow-ups (fixes and target test binaries) #4180

apostasie · 2025-05-01T23:13:33Z

This addresses a few issues from #4012 as a follow-up.

Besides a couple of mundane fixes, the bulk of it focuses on the ability to test binaries that are not named docker nor nerdctl.

This feature is especially interesting:

for people building forks, or custom cli on top of nerdctl as a library, that may use a different binary name
for gomodjail variants of course
for busy contributors or bisectors who want to be able to quickly test a bunch of differently named nerdctl binaries

Here is a list of things I wish I could test:

~/docker.xyz
nerdctl.gomodjail
lepton
/somerandompath/nerdctl.xyz

The proposal here is to expand on the existing --test.target flag, that currently only allows docker and nerdctl.

The upside is that it strictly minimizes the amount of change required by the rest of the test tooling, is strictly backward compatible, and does not introduce new knobs.

Of course we still need a way to differentiate between docker, and "nerdishctl" binaries (cli that are expected to behave like nerdctl), as we do have specialized test behavior.

We do that here by checking strings.HasPrefix(filepath.Base(binary), "docker").

So:

go test ./cmd/nerdctl/... -test.target=~/docker.xyz is treated as docker, while any other variant above is treated as a "nerdishctl".

NOTE that:

there were a bunch of hardcoded assumptions in the tests assuming that the binary was named nerdctl, so, they got modified
the Target type and a couple of (now useless) consts have been removed

There is still some level of duplication between testutil / testutil/nerdtest - this is not new, and is transitory while we move to the new testing tooling - can be cleaned-up later on to keep this small.

pkg/testutil/nerdtest/command.go

AkihiroSuda

Thanks

apostasie · 2025-05-02T00:29:42Z

Thanks

Thanks @AkihiroSuda
Let me finish this real quick (something is not right with GetTarget / M) and then let's have a good look at the UX and see if that works well.

apostasie · 2025-05-02T01:04:20Z

@AkihiroSuda updated the initial comment with some details about the UX. lmk your thoughts.

Also, I am pretty sure there currently are tests that do not pass with the gomodjail targets (seen on another PR), but I think we can address that separately.

Pending (mostly) green that should be good to go (going to do another proofread rn).

apostasie · 2025-05-02T01:07:33Z

cmd/nerdctl/container/container_restart_linux_test.go

 	testCase := nerdtest.Setup()

-	testCase.Require = require.Not(nerdtest.Gomodjail) // FIXME
+	// XXX Why?


To be removed once we confirm this passes - or uncommented with some details.

apostasie · 2025-05-02T01:07:46Z

cmd/nerdctl/container/container_run_linux_test.go

 func TestRunSigProxy(t *testing.T) {
 	testCase := nerdtest.Setup()
-	testCase.Require = require.Not(nerdtest.Gomodjail) // FIXME
+	// XXX Why?


AkihiroSuda · 2025-05-02T02:40:25Z

Follow-up fixes

The PR title isn't descriptive

cmd/nerdctl/container/container_run_linux_test.go

AkihiroSuda · 2025-05-02T04:47:58Z

https://github.com/containerd/nerdctl/actions/runs/14787855221/job/41521670427?pr=4180

TestRunSigProxy is consistently failing

        |         | 🟠 time=2025-05-02T04:37:41.054Z level=WARN msg=***Blocked*** pid=47791 exe=/home/rootless/.cach |
        |         | e/gomodjail/1f381f194c295770/nerdctl syscall=openat entry=/go/pkg/mod/github.com/syndtr/gocapabi |
        |         | [email protected]/capability/capability_linux.go:53:github.com/syndtr/goca |
        |         | pability/capability.initLastCap module=github.com/syndtr/gocapability                            |
        |         | time=2025-05-02T04:37:41.055Z level=WARN msg=***Blocked*** pid=47791 exe=/home/rootless/.cache/g |
        |         | omodjail/1f381f194c295770/nerdctl syscall=fcntl entry=/go/pkg/mod/github.com/syndtr/gocapability |
        |         | @v0.0.0-20200815063812-42c35b437635/capability/capability_linux.go:53:github.com/syndtr/gocapabi |
        |         | lity/capability.initLastCap module=github.com/syndtr/gocapability                                |
        |         | time=2025-05-02T04:37:41.055Z level=WARN msg=***Blocked*** pid=47791 exe=/home/rootless/.cache/g |
        |         | omodjail/1f381f194c295770/nerdctl syscall=fcntl entry=/go/pkg/mod/github.com/syndtr/gocapability |
        |         | @v0.0.0-20200815063812-42c35b437635/capability/capability_linux.go:53:github.com/syndtr/gocapabi |
        |         | lity/capability.initLastCap module=github.com/syndtr/gocapability                                |
        |         | time=2025-05-02T04:37:41.055Z level=WARN msg=***Blocked*** pid=47791 exe=/home/rootless/.cache/g |
        |         | omodjail/1f381f194c295770/nerdctl syscall=fcntl entry=/go/pkg/mod/github.com/syndtr/gocapability |
        |         | @v0.0.0-20200815063812-42c35b437635/capability/capability_linux.go:53:github.com/syndtr/gocapabi |
        |         | lity/capability.initLastCap module=github.com/syndtr/gocapability                                |
        |         | time=2025-05-02T04:37:41.084Z level=WARN msg=***Blocked*** pid=47791 exe=/home/rootless/.cache/g |
        |         | omodjail/1f381f194c295770/nerdctl syscall=openat entry=/go/pkg/mod/github.com/syndtr/gocapabilit |
        |         | [email protected]/capability/capability_linux.go:53:github.com/syndtr/gocapab |
        |         | ility/capability.initLastCap module=github.com/syndtr/gocapability                               |
        |         | time=2025-05-02T04:37:41.084Z level=WARN msg=***Blocked*** pid=47791 exe=/home/rootless/.cache/g |
        |         | omodjail/1f381f194c295770/nerdctl syscall=fcntl entry=/go/pkg/mod/github.com/syndtr/gocapability |
        |         | @v0.0.0-20200815063812-42c35b437635/capability/capability_linux.go:53:github.com/syndtr/gocapabi |
        |         | lity/capability.initLastCap module=github.com/syndtr/gocapability                                |
        |         | time=2025-05-02T04:37:41.085Z level=WARN msg=***Blocked*** pid=47791 exe=/home/rootless/.cache/g |
        |         | omodjail/1f381f194c295770/nerdctl syscall=fcntl entry=/go/pkg/mod/github.com/syndtr/gocapability |
        |         | @v0.0.0-20200815063812-42c35b437635/capability/capability_linux.go:53:github.com/syndtr/gocapabi |
        |         | lity/capability.initLastCap module=github.com/syndtr/gocapability                                |
        |         | time=2025-05-02T04:37:41.085Z level=WARN msg=***Blocked*** pid=47791 exe=/home/rootless/.cache/g |
        |         | omodjail/1f381f194c295770/nerdctl syscall=fcntl entry=/go/pkg/mod/github.com/syndtr/gocapability |
        |         | @v0.0.0-20200815063812-42c35b437635/capability/capability_linux.go:53:github.com/syndtr/gocapabi |
        |         | lity/capability.initLastCap module=github.com/syndtr/gocapability                                |

gocapability seems to need //gomodjail:unconfined

apostasie · 2025-05-02T04:54:28Z

https://github.com/containerd/nerdctl/actions/runs/14787855221/job/41521670427?pr=4180

TestRunSigProxy is consistently failing

gocapability seems to need //gomodjail:unconfined

Maybe...

I am reading their source now. Do not like seeing an init in there https://github.com/syndtr/gocapability/blob/master/capability/capability_linux.go#L32-L45 but then this lib is used everywhere so there is no changing that.

Curious why would that prevent the signal from being caught by the bash trap?

Anyhow, I think the priority one is the flock question so that we can debug on a more solid ground.

apostasie · 2025-05-02T04:58:54Z

Why would we even need to actively check capabilities in that code path? 🤔

init should be banned.

apostasie · 2025-05-02T06:16:28Z

Why would we even need to actively check capabilities in that code path? 🤔

init should be banned.

@AkihiroSuda this is new.

v2.0.4 we do not have this.

This got dragged in by cdi.

IMHO this is just wrong (well, especially the init part).

AkihiroSuda · 2025-05-02T06:21:34Z

Thanks, the warning seems legit, ~~but why is the warning printed only on TestRunSigProxy on CI~~ 🤔

EDIT: now it is printed every time, as #4170 (CDI) was merged after #4012 (gomodjail)

apostasie · 2025-05-02T06:24:37Z

It is likely printed everywhere (where we hit one of the code paths above)
https://github.com/containerd/nerdctl/actions/runs/14787855221/job/41521670427?pr=4180#step:8:4964

apostasie · 2025-05-02T06:24:50Z

Thanks, the warning seems legit, ~~but why is the warning printed only on TestRunSigProxy on CI~~ 🤔

EDIT: now it is printed every time, as #4170 (CDI) was merged after #4012 (gomodjail)

Yeah...

AkihiroSuda · 2025-05-02T06:25:59Z

The gocapability warning is noisy, but probably irrelevant to the failure of the test.

The CI is green for the merge commit for #4170
8d7539a

apostasie · 2025-05-02T06:26:01Z

I think it is a red herring.

Well, this thing is a big problem in itself - but I am not convinced it is responsible for the test failure, which is probably a different issue.

apostasie · 2025-05-02T06:33:09Z

Cross posting a lot...
I am too tired. Logging off.
Good night Akihiro.

apostasie · 2025-05-02T17:29:47Z

Morning there (or whatever time it is) :-)

[GOMODJAIL]: suggesting to replace indirect module syndtr/gocapability #4183 is a proposal to address concerns with this third-party dependency
gomodjail signal handling is likely problematic AkihiroSuda/gomodjail#51 has a reproducer and half-the-ass-of-a-diagnosis for the actual issue here

I am going to disable signal related tests for gomodjail for now, so that we can merge this PR.

Signed-off-by: apostasie <[email protected]>

AkihiroSuda

Thanks

apostasie force-pushed the fix-gomodjail-integration branch 2 times, most recently from 4a59607 to b9e19f7 Compare May 2, 2025 00:10

AkihiroSuda reviewed May 2, 2025

View reviewed changes

pkg/testutil/nerdtest/command.go Outdated Show resolved Hide resolved

AkihiroSuda approved these changes May 2, 2025

View reviewed changes

AkihiroSuda mentioned this pull request May 2, 2025

go.mod: experimental integration of gomodjail (library sandbox) #4012

Merged

apostasie force-pushed the fix-gomodjail-integration branch 2 times, most recently from 4a83d6d to f15db06 Compare May 2, 2025 00:57

apostasie commented May 2, 2025

View reviewed changes

apostasie changed the title ~~[WIP]~~ Follow-up fixes May 2, 2025

apostasie mentioned this pull request May 2, 2025

[CI]: gomodjail target catch-all issues #4182

Closed

apostasie marked this pull request as ready for review May 2, 2025 01:40

apostasie closed this May 2, 2025

apostasie reopened this May 2, 2025

AkihiroSuda reviewed May 2, 2025

View reviewed changes

cmd/nerdctl/container/container_run_linux_test.go Outdated Show resolved Hide resolved

apostasie changed the title ~~Follow-up fixes~~ [FIX]: #4012 follow-ups (fixes and target test binaries) May 2, 2025

apostasie force-pushed the fix-gomodjail-integration branch from f15db06 to 836ab28 Compare May 2, 2025 03:07

AkihiroSuda added this to the v2.1.0 milestone May 2, 2025

This was referenced May 2, 2025

[GOMODJAIL]: suggesting to replace indirect module syndtr/gocapability #4183

Closed

gomodjail signal handling is likely problematic AkihiroSuda/gomodjail#51

Open

Follow-up to containerd#4012: address tests issues

4dd9ad9

Signed-off-by: apostasie <[email protected]>

apostasie force-pushed the fix-gomodjail-integration branch from 836ab28 to 4dd9ad9 Compare May 2, 2025 17:49

This was referenced May 2, 2025

[WIP] [CI] cleanup series end-goal #4154

Closed

[CI]: cleanup, breakout 5: separate jobs #4189

Merged

AkihiroSuda approved these changes May 3, 2025

View reviewed changes

AkihiroSuda merged commit 944dbe6 into containerd:main May 3, 2025
40 checks passed

[FIX]: #4012 follow-ups (fixes and target test binaries) #4180

[FIX]: #4012 follow-ups (fixes and target test binaries) #4180

Uh oh!

Conversation

apostasie commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

apostasie commented May 2, 2025

Uh oh!

apostasie commented May 2, 2025

Uh oh!

apostasie May 2, 2025

Choose a reason for hiding this comment

Uh oh!

apostasie May 2, 2025

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented May 2, 2025

Uh oh!

Uh oh!

AkihiroSuda commented May 2, 2025

Uh oh!

apostasie commented May 2, 2025

Uh oh!

apostasie commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apostasie commented May 2, 2025

Uh oh!

AkihiroSuda commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apostasie commented May 2, 2025

Uh oh!

apostasie commented May 2, 2025

Uh oh!

AkihiroSuda commented May 2, 2025

Uh oh!

apostasie commented May 2, 2025

Uh oh!

apostasie commented May 2, 2025

Uh oh!

apostasie commented May 2, 2025

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

apostasie commented May 1, 2025 •

edited

Loading

apostasie commented May 2, 2025 •

edited

Loading

AkihiroSuda commented May 2, 2025 •

edited

Loading