specgen: improve heuristic for /sys bind mount

[giuseppe]

partially revert 95c4577

restrict the cases where /sys is bind mounted from the host.

The heuristic doesn't detect all the cases where the bind mount is not
necessary, but it is an improvement on the previous version where /sys
was always bind mounted for rootless containers unless --net none was
specified.

Signed-off-by: Giuseppe Scrivano [email protected]

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mheon]

FYI, our upstream CI did not catch any of these - it was the RHEL8 test suite that @edsantiago maintains.

[giuseppe]

FYI, our upstream CI did not catch any of these - it was the RHEL8 test suite that @edsantiago maintains.

I see, the CI completed successfully. @edsantiago please let me know if there is any reproducer I can use

[edsantiago]

@giuseppe I think this is the one where rootless podman failed almost every test with:

Cannot get exit code: failed to get cursor: cannot assign requested address

Look in your aos-containers-internal mail archives for my email of 2020-11-05, subject "podman gating tests again", for a link to a log file.

[giuseppe]

I am not really sure how to solve correctly this issue.

The previous check was an obvious one as a bind mount is used with --net host.

If we want it to work in all the cases though we'd have to check whether the target network namespace is owned by the target user namespace. The check is running too early (at specgen time) to know these details and in general it is error prone to try to duplicate all the checks the kernel does.

I think crun does it correctly as it tries to mount sysfs and if it fails then it fallbacks to a bind mount, so the container engine doesn't have to worry to detect this case earlier. I wonder if we should add a new RuntimeSupportsSysfsCheck in the common image so we can avoid the heuristic when crun is used.

[mheon]

I think that seems like a sane solution. Can we also fix runc to use the improved crun logic?

[giuseppe]

I think that seems like a sane solution. Can we also fix runc to use the improved crun logic?

@kolyshkin do you think it is something runc could do?

[giuseppe]

I think once ready this fix should go in 3.0 @mheon @rhatdan

[mheon]

Deadline for 3.0 is tomorrow, so we don't have long for that, unfortunately.

[mheon]

Though, since this is a bugfix, we can probably get it in later, as long as it's before the final release

[giuseppe]

I am afraid this could introduce some regressions on RHEL and runc, we can deal with it later and better tune the heuristic

[rhatdan]

/lgtm