Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(22.04): add crun and uidmap slices #222

Merged
merged 16 commits into from
Sep 27, 2024

Conversation

endersonmaia
Copy link

@endersonmaia endersonmaia commented Apr 17, 2024

Proposed changes

Add crun and uidmap slices to 22.04.

Close #221

Related issues/PRs

Forward porting

Testing

Will

# on a local copy in the current branch run:
mkdir rootfs
chisel cut \
    --release ./ \
    --root rootfs/ \
    --arch=amd64 \
    base-files_base \
    base-files_release-info \
    crun_bins
cat <<EOF | docker build -t crun-container . -f -
FROM scratch
COPY rootfs/ /
EOF
docker run --rm crun-container crun --version
rm -rf rootfs/

Expected:

crun version 0.17
commit: 0e9229ae34caaebcb86f1fde18de3acaf18c6d9a
spec: 1.0.0
+SYSTEMD +SELINUX +APPARMOR +CAP +SECCOMP +EBPF +YAJL

Checklist

Additional Context

Copy link

github-actions bot commented Apr 17, 2024

Diff of dependencies:
None found.


@endersonmaia endersonmaia force-pushed the crun-to-22.04 branch 2 times, most recently from 9918164 to 9353b0e Compare April 17, 2024 14:05
@endersonmaia endersonmaia marked this pull request as ready for review April 17, 2024 14:34
@endersonmaia endersonmaia changed the title add crun to jammy (22.04) feat(22.04): add crun slice Apr 17, 2024
@endersonmaia endersonmaia changed the title feat(22.04): add crun slice feat(22.04): add crun and uidmap slices Apr 18, 2024
@endersonmaia endersonmaia force-pushed the crun-to-22.04 branch 4 times, most recently from 0a9ce98 to 815a5f5 Compare April 19, 2024 19:33
@rebornplusplus rebornplusplus self-assigned this May 16, 2024
Copy link
Collaborator

@zhijie-yang zhijie-yang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. There are some issues regarding the linting (see workflow runs) to be fixed, and it is suggested to add the copyright files explicitly in the slice definitions.

@linostar linostar self-requested a review May 16, 2024 09:04
Copy link
Collaborator

@linostar linostar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slice names to sorted, and copyright need to be added:

  • crum: /usr/share/doc/crun/copyright:
  • libaudit-common: /usr/share/doc/libaudit-common/copyright:
  • libaudit1: /usr/share/doc/libaudit1/copyright:
  • libcap-ng0: /usr/share/doc/libcap-ng0/copyright:
  • libcap2: /usr/share/doc/libcap2/copyright:
  • liblz4-1: /usr/share/doc/liblz4-1/copyright:
  • libseccomp2: /usr/share/doc/libseccomp2/copyright:
  • libsystemd0: /usr/share/doc/libsystemd0/copyright:
  • libyajl2: /usr/share/doc/libyajl2/copyright:
  • uidmap: /usr/share/doc/uidmap/copyright:

Copy link
Collaborator

@linostar linostar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

slices/crun.yaml Show resolved Hide resolved
slices/uidmap.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@zhijie-yang zhijie-yang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good to me. Thanks!

Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The slices look good, thanks! Only left a few comments below.

With the recent changes around testing in the upstream branch, I think it would be nice to have a spread test for crun and uidmap as well.

slices/crun.yaml Show resolved Hide resolved
slices/libgcrypt20.yaml Outdated Show resolved Hide resolved
Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the trivial changes myself. Looks good to me, thank you!

Tried to add a smoke test by running crun --version in a chroot env, but crun runs into an error. Probably this is related: containers/crun#1434. Reverted the commit I added for smoke test. Please feel free to add a smoke test of your own!

@cjdcordeiro
Copy link
Collaborator

@rebornplusplus this is actually a very similar issue to #274 -> crun needs procfs, which doesn't exist in the chroot env and we can't create it cause the test runs inside an unprivileged container.

If we can't find a workaround until tomorrow, let's just add a TODO comment in the SDF and move on.

@cjdcordeiro
Copy link
Collaborator

@endersonmaia how urgent is this? I'm trying to find a different testing strategy to support such privileged tests like yours, but it could take a few days. If you need this now, I'm happy to merge it with the simplified tests and improve those later on.

@endersonmaia
Copy link
Author

@endersonmaia how urgent is this? I'm trying to find a different testing strategy to support such privileged tests like yours, but it could take a few days. If you need this now, I'm happy to merge it with the simplified tests and improve those later on.

I'm using my fork's branch to have this in my build pipeline, so it's not urgent.

@endersonmaia
Copy link
Author

... I'm trying to find a different testing strategy to support such privileged tests like yours ...

Have you considered running qemu or even firecracker?

I use the crane tool to generate a rootfs and together with a kernel image you could start a firecracker microVM using something like firectl

@cjdcordeiro
Copy link
Collaborator

Thanks @endersonmaia and thanks for the suggestions. I already have #317 up to introduce LXD as a backend for the tests execution. I've tested it with your PR and seemed to work just fine. So once it's merged, we can rebase your PR.

If you have some tests, please add them and I'll re-trigger the CI after the rebase

@cjdcordeiro
Copy link
Collaborator

Updated the trivial changes myself. Looks good to me, thank you!

Tried to add a smoke test by running crun --version in a chroot env, but crun runs into an error. Probably this is related: containers/crun#1434. Reverted the commit I added for smoke test. Please feel free to add a smoke test of your own!

@rebornplusplus (or @endersonmaia ) could you please add such test here (and counterpart releases)? 🙏

@cjdcordeiro
Copy link
Collaborator

Alright, I've added the crun test. Looking good. Merging.

@cjdcordeiro cjdcordeiro merged commit c399e84 into canonical:ubuntu-22.04 Sep 27, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants