[meta-virtualization][PATCH] crun: provide virtual-runc and honour CRUN_AS_RUNC

[meta-virtualization][PATCH] crun: provide virtual-runc and honour CRUN_AS_RUNC

[Royyan Zahir]

runc.inc declares the provider abstraction the layer builds on:

  PROVIDES += "virtual/runc"
  RPROVIDES:${PN} = "virtual-runc"

crun declares neither, even though it already stands in for runc: with
CRUN_AS_RUNC it installs the /usr/bin/runc symlink and takes RCONFLICTS on
the runc package. So PREFERRED_RPROVIDER_virtual-runc = "crun" cannot
resolve, and every consumer that wants crun as its OCI runtime carries the
same bbappend adding the RPROVIDES by hand.

Join the virtual when the symlink is installed, which is the case where crun
actually substitutes for runc.

While here, make CRUN_AS_RUNC mean what it says. Both guards treat the string
"false" as true, the shell one because -n tests for a non-empty string and the
python one because a non-empty str is truthy, so the only way to turn the
symlink off today is to set the variable to the empty string. Normalize once
with bb.utils.to_boolean() and use that everywhere.

Note that to_boolean() rejects values outside the y/yes/1/true and n/no/0/false
sets, so a recipe or config setting CRUN_AS_RUNC to something else now fails to
parse instead of silently enabling the symlink.

Signed-off-by: Royyan Zahir <royzah@gmail.com>
---
 recipes-containers/crun/crun_git.bb | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/recipes-containers/crun/crun_git.bb b/recipes-containers/crun/crun_git.bb
index 288c953..67e67e9 100644
--- a/recipes-containers/crun/crun_git.bb
+++ b/recipes-containers/crun/crun_git.bb
@@ -31,6 +31,10 @@ CLEANBROKEN = "1"
 # with container stacks
 CRUN_AS_RUNC ?= "true"
 
+# Normalized once: a bare "false" is a non-empty string and would otherwise
+# read as true in both the shell test and the python expansions below.
+CRUN_AS_RUNC_ENABLED = "${@'1' if bb.utils.to_boolean(d.getVar('CRUN_AS_RUNC')) else ''}"
+
 PACKAGECONFIG ??= " \
     caps external-yajl man \
     ${@bb.utils.contains('DISTRO_FEATURES', 'seccomp', 'seccomp', '', d)} \
@@ -56,13 +60,15 @@ do_configure:prepend () {
 
 do_install() {
     oe_runmake 'DESTDIR=${D}' install
-    if [ -n "${CRUN_AS_RUNC}" ]; then
+    if [ -n "${CRUN_AS_RUNC_ENABLED}" ]; then
         ln -sr "${D}/${bindir}/crun" "${D}${bindir}/runc"
     fi
 }
 
-# When crun provides /usr/bin/runc symlink, it conflicts with the runc package
-RCONFLICTS:${PN} = "${@'runc' if d.getVar('CRUN_AS_RUNC') else ''}"
+# When crun provides the /usr/bin/runc symlink it replaces the runc package,
+# so it conflicts with it and stands in for it as the virtual-runc provider.
+RCONFLICTS:${PN} = "${@'runc' if d.getVar('CRUN_AS_RUNC_ENABLED') else ''}"
+RPROVIDES:${PN} += "${@'virtual-runc' if d.getVar('CRUN_AS_RUNC_ENABLED') else ''}"
 
 REQUIRED_DISTRO_FEATURES:class-native ?= ""
 DEPENDS:class-native += "yajl libcap go-md2man m4 libseccomp"
-- 
2.55.0

Re: [meta-virtualization][PATCH] crun: provide virtual-runc and honour CRUN_AS_RUNC

[Bruce Ashfield]

Hi Royyan,

Thanks for the patch, but the premise it's built on is the one thing
I need to push back on.

`virtual/runc` (and its RPROVIDES sibling `virtual-runc`) is a holdover
from when meta-virtualization carried two runc implementations in-tree,
opencontainers/runc and docker's own. Commit 7d6284a0 (2017)
introduced the abstraction so a build could set PREFERRED_PROVIDER to
pick which variant was the C-level runc binary. Once the two variants
were consolidated (cb2d4868 removed the "opencontainers" flavor,
ea15fd06 dropped runc-docker), the mechanism lost its purpose. It's on
a path to removal, not extension, and I'd rather not add new consumers.

For picking crun vs runc as the OCI runtime of a container engine, the
layer's mechanism today is `VIRTUAL-RUNTIME_container_runtime` (plus
the sibling VIRTUAL-RUNTIME_container_* variables) set in the profile
fragments under `conf/distro/include/`. `meta-virt-container-containerd.inc`
already ships with `VIRTUAL-RUNTIME_container_runtime ??= "crun"` -- that
is the intended way to make crun the default runtime, and no bbappend
or RPROVIDES is involved. The bbappend churn you're seeing in
downstreams is a symptom of that pattern not being documented well
enough, not of a missing RPROVIDES on crun.

That said, the second half of your patch is genuinely finding a bug:
`CRUN_AS_RUNC = "false"` reads as true today, because "false" is a
non-empty string in both the shell `-n` test and the python
`if d.getVar(...)` expansions. That fix is worth landing on its own.

Would you resubmit as a smaller patch that keeps just the
`bb.utils.to_boolean()` normalization for `CRUN_AS_RUNC`, without the
`RPROVIDES virtual-runc` addition? I'd also skip the comment
rewordings on the RCONFLICTS line -- the existing comment reads fine
and reworking it on a bugfix makes the change harder to review. I can
take that version as-is.

Cheers,

Bruce