-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Separate runc binary version from libcontainer version, and remove obsolete build-tags #5036
Conversation
Build succeeded.
|
This was brought up as consideration by @AkihiroSuda in #4717 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
BUILDING.md
Outdated
@@ -209,7 +209,7 @@ Next, let's build `runc`: | |||
|
|||
```sh | |||
cd /go/src/github.com/opencontainers/runc | |||
make BUILDTAGS='seccomp apparmor selinux' && make install | |||
make BUILDTAGS='seccomp' && make install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add:
The "selinux" and "apparmor" buildtags have been removed, and now all runc
builds will have SELinux and AppArmor support enabled. Note that "seccomp"
is still optional (though we very highly recommend you enable it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUILDTAGS=seccomp
is specified by default, so just make && make install
should work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default runc BUILDTAGS gets overwritten if passing any BUILDTAGS to make cri-cni-release
, because it passes the same value through to the runc Makefile if it isn't explicitly set here.
I've been building with make BUILDTAGS=no_btrfs cri-cni-release
and containerd started failing in my test k8s clusters after pulling this change into master.
(in script/setup/install-runc)
HEAD is now at 12644e61 VERSION: release 1.0.0~rc93
make[1]: Entering directory '/tmp/tmp.ZJzc2KtI0A/runc'
go build -trimpath "-mod=vendor" "-buildmode=pie" -tags "no_btrfs" -ldflags "-X main.gitCommit="12644e614e25b05da6fd08a38ffa0cfe1903fdec" -X main.version=1.0.0-rc93 " -o runc .
^^^^^^^^^^^^^^^^
Maybe this should be documented somewhere, or if the runc build tags aren't intended to be changed from the default then go back to setting it here again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... good one; that's because both containerd and runc use the same variable name. Also wondering if runc
looks at environment variables, or make variables 🤔
I guess we can add the BUILDTAGS
back here, because we always build with the same options in our script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened #5184
docs/RUNC.md
Outdated
```bash | ||
make BUILDTAGS='seccomp selinux' && sudo make install | ||
make BUILDTAGS='seccomp' && sudo make install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment around leaving some historical reference so people will know why it's no longer there...
script/setup/install-runc
Outdated
@@ -21,13 +21,15 @@ | |||
set -eu -o pipefail | |||
|
|||
function install_runc() { | |||
RUNC_COMMIT=$(grep opencontainers/runc "$GOPATH"/src/github.com/containerd/containerd/go.mod | awk '{print $2}') | |||
# When updating RUNC_COMMIT, consider updating the runc module as well | |||
: ${RUNC_COMMIT:=12644e614e25b05da6fd08a38ffa0cfe1903fdec} # v1.0.0-rc93 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is a good way to publicize this? we want to highlight this well enough for folks who have now been used to picking up SHA from the go.mod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chase it down with hound and push commits ? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chase it down with hound and push commits ? :-)
Hehehe!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we change the name of the variable to RUNC_VERSION
to encourage using tagged releases (and default to using a tag)?
bb48350
to
b9320aa
Compare
Build succeeded.
|
From the runc v1.0.0-rc93 release notes: > The "selinux" and "apparmor" buildtags have been removed, and now all runc > builds will have SELinux and AppArmor support enabled. Note that "seccomp" > is still optional (though we very highly recommend you enable it). Also adding a note about kmem support. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
b9320aa
to
60c0300
Compare
Build succeeded.
|
script/setup/install-runc
Outdated
@@ -21,13 +21,14 @@ | |||
set -eu -o pipefail | |||
|
|||
function install_runc() { | |||
RUNC_COMMIT=$(grep opencontainers/runc "$GOPATH"/src/github.com/containerd/containerd/go.mod | awk '{print $2}') | |||
# When updating RUNC_VERSION, consider updating the runc module in go.mod as well | |||
: "${RUNC_VERSION:=v1.0.0-rc93}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we please pick up the RUNC_VERSION from a file? ( we could have a file name RUNC_VERSION with the contents of v1.0.0-rc93
) So it will then be easy to curl
the version from a branch or master (directly from github raw url) easily for automation (in downstream or other CI in other projects).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So; pick from file, but allow the RUNC_VERSION
env-var to override, correct? Yes I guess we can do that. Let me have a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works! thanks!
Now that the dependency on runc (libcontaienr) code has been reduced considerably, it is probbaly ok to cut the version dependency between libcontainer and the runc binary that is supported. This patch separates the runc binary version from the version of libcontainer that is defined in go.mod, and updates the documentation accordingly. The RUNC_COMMIT variable in the install-runc script is renamed to RUNC_VERSION to encourage using tagged versions, and the Dockerfile in contrib is updated to allow building with a custom version. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
60c0300
to
8325ba5
Compare
Build succeeded.
|
@dims updated; pushed a commit that moves the version to a separate file |
@@ -49,7 +49,8 @@ Please be aware: nightly builds might have critical bugs, it's not recommended f | |||
|
|||
Runtime requirements for containerd are very minimal. Most interactions with | |||
the Linux and Windows container feature sets are handled via [runc](https://github.com/opencontainers/runc) and/or | |||
OS-specific libraries (e.g. [hcsshim](https://github.com/Microsoft/hcsshim) for Microsoft). The current required version of `runc` is always listed in [RUNC.md](/docs/RUNC.md). | |||
OS-specific libraries (e.g. [hcsshim](https://github.com/Microsoft/hcsshim) for Microsoft). | |||
The current required version of `runc` is described in [RUNC.md](docs/RUNC.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keeping RUNC.md
as the "canonical" place to describe what version to use instead of pointing to the runs-version
file (I think it's useful for readers to get the additional context that the RUNC.md
document provides
Arf.. that didn't work; probably needs abs-path (relative to the script)
|
6412df6
to
7d18415
Compare
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thanks @thaJeztah !
Hmm.. looks like a commit message is not passing validation;
let me check ah! shell script formatting; looks I left a stray space;
|
This moves the runc version to build to scripts/setup/runc-version, which makes it easier for packagers to find the default version to use. The RUNC_VERSION environment variable can still be used to override the version, which can be used (e.g.) to test against different versions in our CI. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
7d18415
to
79a51cd
Compare
Build succeeded.
|
@mikebrow ptal; hope the latest changes LGTY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- bump docker-proxy dep to 0.8.0_p20210525 - unfortunately tests require priveleges and fetching from network - ESYSROOT should be used to locate headers and libraries. - Remove CONFIG_RT_GROUP_SCHED - drop the dependency on runc since this is pulled in by containerd - set a lower limit for the dependency oncontainerd but do not pin it to a specific version. For more information, see the below upstream issue. moby/moby#42117 containerd/containerd#5036
Remove references to apparmor and selinux buildtags for runc
From the runc v1.0.0-rc93 release notes:
Separate runc binary version from libcontainer version
Now that the dependency on runc (libcontaienr) code has been reduced
considerably, it is probbaly ok to cut the version dependency between
libcontainer and the runc binary that is supported.
This patch separates the runc binary version from the version of
libcontainer that is defined in go.mod, and updates the documentation
accordingly.
The RUNC_COMMIT variable in the install-runc script is renamed to
RUNC_VERSION to encourage using tagged versions, and the Dockerfile
in contrib is updated to allow building with a custom version.
move runc version to a separate file for easier consumption
This moves the runc version to build to scripts/setup/runc-version,
which makes it easier for packagers to find the default version
to use.
The RUNC_VERSION environment variable can still be used to override
the version, which can be used (e.g.) to test against different versions
in our CI.