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

further reduce uses of libcontainer and update runc v1.0.0-rc93 #4717

Merged
merged 2 commits into from Feb 5, 2021

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 10, 2020

depends on:

After this, we only vendor:

  • libcontainer/user
    • used in oci/spec_opts.go (perhaps this is something to move to runtime-spec?)
  • libcontainer/devices
    • devices.HostDevices() (used in a single test in pkg/cri/server/container_create_linux_test.go)
    • devices.DeviceFromPath(() (used in a single location; pkg/cri/opts/spec_linux.go)
  • libcontainer/nsenter ("fake" dependency; pulled in because it contains C code)

The updated version of runc and gocapability add support for new capabilities
added in kernel 5.9 (syndtr/gocapability@d983527...42c35b4)

  • CAP_PERFMON
  • CAP_BPF
  • CAP_CHECKPOINT_RESTORE

@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

vendor.conf Outdated Show resolved Hide resolved
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah
Copy link
Member Author

Hmm.. something failing; perhaps it doesn't deal well with the fork?

fatal: reference is not a tree: c7f874992f5322b2783580edef6ba6ae3e4aaa6e
Error: Process completed with exit code 128.

@thaJeztah thaJeztah force-pushed the reduce_libcontainer_use branch 2 times, most recently from 010c9a4 to 64cca75 Compare November 10, 2020 11:35
Comment on lines 26 to 27
go get -d github.com/opencontainers/runc
git clone "$RUNC_REPO" /src/github.com/opencontainers/runc
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to check these scripts; I've been bit by similar approaches in docker/docker (given, those didn't use vendoring)

go get -d will use go modules to get the dependency, and therefore first get go modules from master, before switching to the selected commit, which sometimes may cause unexpected results (see moby/moby#41560)

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah
Copy link
Member Author

Interesting; are machined on GH Actions not cleaned up between builds?

Bringing machine 'default' up with 'virtualbox' provider...
==> default: Checking if box 'fedora/32-cloud-base' version '32.20200422.0' is up to date...
==> default: Running provisioner: install-runc (shell)...
    default: Running: inline script
    default: + /root/go/src/github.com/containerd/containerd/script/setup/install-runc
    default: fatal: destination path '/root/go/src/github.com/opencontainers/runc' already exists and is not an empty directory.
The SSH command responded with a non-zero exit status. Vagrant
assumes that this means the command failed. The output for this command
should be in the log above. Please read the output to determine what
went wrong.
Error: Process completed with exit code 1.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 18, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 19, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 19, 2020

Build succeeded.

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 19, 2020

Interesting; https://github.com/containerd/containerd/pull/4717/checks?check_run_id=1423934789

 /usr/bin/install -c -m 644 libseccomp.pc '/usr/local/lib/pkgconfig'
+ ldconfig
+ rm -rf /tmp/tmp.QeIT0Nsktc
Cloning into '/home/runner/work/containerd/containerd/src/github.com/opencontainers/runc'...
fatal: protocol 'temporarily
https' is not supported
Error: Process completed with exit code 128.

"protocol 'temporarily https' is not supported" 🤔

edit: well, of course, I know 😂

It's attempting to parse vendor.conf, and uses this line to vendor a dependency:

# FIXME temporarily vendoring from my fork (https://github.com/opencontainers/runc/pull/2679)
github.com/opencontainers/runc                      isolate_device https://github.com/thaJeztah/runc.git

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 19, 2020

Build succeeded.

@thaJeztah
Copy link
Member Author

Hmmm... looks like something doesn't cleanup between runs

Bringing machine 'default' up with 'virtualbox' provider...
==> default: Checking if box 'fedora/32-cloud-base' version '32.20200422.0' is up to date...
==> default: Running provisioner: install-runc (shell)...
    default: Running: inline script
    default: + /root/go/src/github.com/containerd/containerd/script/setup/install-runc
    default: fatal: destination path '/root/go/src/github.com/opencontainers/runc' already exists and is not an empty directory.
The SSH command responded with a non-zero exit status. Vagrant
assumes that this means the command failed. The output for this command
should be in the log above. Please read the output to determine what
went wrong.
Error: Process completed with exit code 1.

@AkihiroSuda
Copy link
Member

opencontainers/runc#2679 is now merged

@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 3, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 3, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 7, 2020

Build succeeded.

@AkihiroSuda
Copy link
Member

Is this still draft?

Looks like this import was not needed for the test; simplified the test
by just using the device-path (a counter would work, but for debugging,
having the list of paths can be useful).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
full diff: opencontainers/runc@v1.0.0-rc92...v1.0.0-rc93

also removes dependency on libcontainer/configs

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah changed the title [draft] further reduce uses of libcontainer further reduce uses of libcontainer and update runc v1.0.0-rc93 Feb 4, 2021
@thaJeztah thaJeztah marked this pull request as ready for review February 4, 2021 15:15
@thaJeztah
Copy link
Member Author

After this; dependency on runc vendoring is reduced to;

$ tree vendor/github.com/opencontainers/runc/

vendor/github.com/opencontainers/runc/
├── LICENSE
├── NOTICE
└── libcontainer
    ├── devices
    │   ├── device.go
    │   ├── device_unix.go
    │   ├── device_windows.go
    │   └── devices.go
    └── user
        ├── MAINTAINERS
        ├── lookup.go
        ├── lookup_unix.go
        ├── lookup_windows.go
        └── user.go

3 directories, 11 files

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 4, 2021

Build succeeded.

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@AkihiroSuda AkihiroSuda added this to the 1.5 milestone Feb 4, 2021
@AkihiroSuda AkihiroSuda added impact/changelog cherry-pick/1.4.x Change to be cherry picked to release/1.4 branch labels Feb 4, 2021
@estesp
Copy link
Member

estesp commented Feb 4, 2021

almost passed :( test timeout as it just didn't finish by 10m inside the Fedora-in-vagrant-on-macOS 🤓

@thaJeztah
Copy link
Member Author

Does it need a longer timeout? Don't think we can restart only a single job, correct?

@thaJeztah
Copy link
Member Author

/retest

@dims
Copy link
Member

dims commented Feb 4, 2021

LGTM

the failed CI job seems be because of a bad download

2021-02-04T17:00:59.5843050Z     default: Downloading: https://vagrantcloud.com/fedora/boxes/33-cloud-base/versions/33.20201019.0/providers/virtualbox.box
2021-02-04T17:00:59.6362490Z 
2021-02-04T17:00:59.7170450Z �[KProgress: 0% (Rate: 0*/s, Estimated time remaining: --:--:--)
2021-02-04T17:00:59.7172020Z �[KDownload redirected to host: download.fedoraproject.org
2021-02-04T17:00:59.7708170Z 
2021-02-04T17:00:59.9694180Z �[KProgress: 100% (Rate: 1766/s, Estimated time remaining: --:--:--)
2021-02-04T17:01:00.0522640Z �[KProgress: 0% (Rate: 0/s, Estimated time remaining: --:--:--)
2021-02-04T17:01:00.0713810Z An error occurred while downloading the remote file. The error
2021-02-04T17:01:00.0714940Z message, if any, is reproduced below. Please fix this error and try
2021-02-04T17:01:00.0715840Z again.
2021-02-04T17:01:00.0716320Z 
2021-02-04T17:01:00.0716840Z The requested URL returned error: 404 Not Found

@thaJeztah
Copy link
Member Author

ah, yes, the runc maintainers also ran into intermittent 404's for fedora; opencontainers/runc#2787

@thaJeztah
Copy link
Member Author

opened #4999 to add the same hack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/1.4.x Change to be cherry picked to release/1.4 branch impact/changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants