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

vendor: drop override for github.com/opencontainers/runc #19817

Closed
wants to merge 1 commit into from

Conversation

giuseppe
Copy link
Member

podman builds fine with the latest runc, so we can drop the override.

Closes: #19795

Does this PR introduce a user-facing change?

None

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 31, 2023

[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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2023
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe
Copy link
Member Author

shurg, we are pulling in a huge new dependency (github.com/cilium/ebpf).

As usual, I forgot to run git add vendor before pushing the first version of the patch

@vrothberg
Copy link
Member

I think there's still some vendor-dependency fart. The main branch of runc doesn't require cilium anymore. But trying to vendor that will revert many other dependencies.
$ go mod edit -replace github.com/opencontainers/runc=github.com/opencontainers/runc@24ae5c258c41a122f1b96319c087337bac7b308a moves it to the latest commit and doesn't pull in cilium ... but it still replaces.

@giuseppe
Copy link
Member Author

I still see cilium in the latest revision: https://github.com/opencontainers/runc/blob/main/go.mod#L7

and I get the same bloat locally: it is 59137488 vs 58094336

@kolyshkin do you think it would make sense to have a build tag to drop ebpf?

@vrothberg
Copy link
Member

I noticed that common is also replacing runc. I guess we need to start there.

@giuseppe giuseppe marked this pull request as draft September 1, 2023 12:46
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2023
@giuseppe
Copy link
Member Author

giuseppe commented Sep 1, 2023

opened a PR for runc to allow disabling eBPF support: opencontainers/runc#4005

let's see if that breaks any test

@giuseppe giuseppe force-pushed the drop-runc-override branch 2 times, most recently from da90610 to 1009bb2 Compare September 1, 2023 21:48
@giuseppe
Copy link
Member Author

giuseppe commented Sep 1, 2023

@kolyshkin I am using the latest HEAD version here, still the binary size grew by 836320 bytes

that is why the package is referenced by:

runc/libcontainer/cgroups/fs/devices.go:9:     cgroupdevices "github.com/opencontainers/runc/libcontainer/cgroups/devices"

@giuseppe giuseppe force-pushed the drop-runc-override branch 2 times, most recently from aade16b to bf8fc65 Compare September 1, 2023 23:04
@giuseppe
Copy link
Member Author

giuseppe commented Sep 4, 2023

temporary version bump until there is a new runc version: #19854

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2023
@vrothberg
Copy link
Member

temporary version bump until there is a new runc version: #19854

@cyphar, do you have another runc version bump scheduled?

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2023
@giuseppe
Copy link
Member Author

giuseppe commented Nov 2, 2023

rebased and testing: containers/common#1633

@vrothberg
Copy link
Member

The system failures look consistent

@giuseppe
Copy link
Member Author

giuseppe commented Nov 3, 2023

runc is still bloated, the last version didn't fix the dependency issue with EPBF (reported here: opencontainers/runc#4005), I'll close this PR for now

@giuseppe giuseppe closed this Nov 3, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Feb 2, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none stale-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

github.com/opencontainers/runc not being updated
3 participants