criu swrk: return child error to caller

[Luap99]

In the podman CI we are seeing a weird flake during criu version detection[1]. The write to the socket just fails with broken pipe. The logical thing to assume here is that the child exited. However the current code never reports back the child error from wait nor does it try to capture the output from it. This fixes both. The cleanup error is now added to the returned error so the caller sees both.

As errors.Join is used from the std lib bump the minimum go version to 1.20.

[1] containers/podman#18856

[Luap99]

Ok so I understand why the CI tests hang.

When capturing output like this in go, the go std lib creates a pipe that is setup on stout/err in the child. After wait() is called on the child go then tries to read all remaining output from the pipe fd set one stdout/err on the child until EOF. However this has one major problem, when the stdout/err file descriptors in the child are then given to other processes. In the hanging tests case this seems to be in the case of CriuReqType_PAGE_SERVER_CHLD which causes a normal fork in the criu swrk process to create criu: page-server which inherits stdout/err.

https://github.com/checkpoint-restore/criu/blob/bf8c134714d6e041261b5129561eeb49aa734ec1/criu/cr-service.c#L1013

I don't have a good answer for this, either we try to make criu close the std fds (dups them to /dev/null) when it forks that there or we do not capture the output at all. Or maybe we special case CriuReqType_PAGE_SERVER_CHLD to not capture output in only this case but I have no idea about criu at all so I don't know if this would effect more options.

Of course I would like to get the output because without debugging failures is basically impossible.

[Luap99]

@rst0git There is no point in rebasing this. I need an answer on the issue raised above. So either I add a special exclude CriuReqType_PAGE_SERVER_CHLD (not sure if there are more we need to worry about) from output capture or I do not try to capture the output at all. I like the output capture of course.

[rst0git]

@Luap99 In general, CRIU writes all output to a log file (e.g., opencontainers/runc@c77aaa3). We don't capture any output from stdout for criu swrk (e.g., libcriu, pycriu), except in runc, where stdin, stdout, and stderr are set to inherit stdio.

[Luap99]

I am not talking about any container runtime involvement here. The fact is that this library just fails with a useless error (write criu-xprt-cln: broken pipe) when the swrk process fails. I can capture the wait error but than we only get the exit code not whatever errors criu pritned to stderr, i.e.
https://github.com/checkpoint-restore/criu/blob/5c3f6217f44acf1edec57a8abf4edcb624accd6f/lib/c/criu.c#L1398

Are you telling me this library should always create a log when starting the swrk process? And then read this file after wait?

[rst0git]

@avagin Do you have any ideas on how we could capture the CRIU logs in go-criu?

For example, the following repository contains a simple program that prints the version of CRIU:
https://github.com/rst0git/test-go-criu-swrk

$ go build
$ ./test-go-criu-swrk 
2024/07/19 14:51:09 31900

If we create the following configuration file, it will cause criu swrk to always fail with an error:

$ echo  test | sudo tee /etc/criu/default.conf
$ criu swrk 2
criu: unrecognized option '--test'
...

However, this error is not captured in stdout/stderr:

$ ./test-go-criu-swrk 
2024/07/19 14:49:53 read criu-xprt-cln: connection reset by peer
criu swrk failed: exit status 1 (<nil>)

A similar problem appears with the CI tests for Podman (containers/podman#18856).

[Luap99]

Ok I understand now, I got confused by the perror lines then as I assumed they print to stderr but if they redirect it into the a log file then it will not help us.

[Luap99]

I guess I push my change without the output capture for now, getting a exit status 1 or killed would already tell us more useful things in podman.

[codecov]

Codecov Report

Attention: Patch coverage is 52.94118% with 8 lines in your changes missing coverage. Please review.

Project coverage is 49.94%. Comparing base (76a006a) to head (1c9f152).
Report is 6 commits behind head on master.

Files	Patch %	Lines
main.go	50.00%	3 Missing and 3 partials ⚠️
phaul/client.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
- Coverage   50.13%   49.94%   -0.19%     
==========================================
  Files          21       21              
  Lines        1889     1898       +9     
==========================================
+ Hits          947      948       +1     
- Misses        810      814       +4     
- Partials      132      136       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rst0git]

LGTM