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

use runc cgroup creation logic #936

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Feb 22, 2022

switch c/common to use runc cgroup creation so that we can use resource limits

This entails importing some runc code directly while also recreating a lot of the logic for resource limits locally
so that imports do not run too large.

Signed-off-by: cdoern cbdoer23@g.holycross.edu

@cdoern
Copy link
Contributor Author

cdoern commented Feb 22, 2022

@mheon not sure if this is the proper way of doing this but it seems like a happy medium of getting resource functionality without completely ripping out the guts here.

also, I am running into persistent cross build errors due to undefined unix entities in runc, any way to avoid these?

@cdoern
Copy link
Contributor Author

cdoern commented Feb 25, 2022

I think I am going to need to make ..._linux files for basically all of the resource handlers. Otherwise, the unix specific entities will always be included, @mheon WDYT?

@cdoern cdoern force-pushed the cgroup branch 5 times, most recently from 4796b6f to f61887f Compare February 25, 2022 21:14
@cdoern
Copy link
Contributor Author

cdoern commented Feb 26, 2022

leaving this here as a note for myself: the reason this is broken right now is because our cgroup creation process needs to be modified more from the start rather than each time we update the cgroup. The specific resource files need to exist which currently do not.

@cdoern cdoern force-pushed the cgroup branch 2 times, most recently from a7076cd to 87e9ac8 Compare February 28, 2022 02:24
@rhatdan
Copy link
Member

rhatdan commented Feb 28, 2022

@giuseppe PTAL

@cdoern
Copy link
Contributor Author

cdoern commented Feb 28, 2022

@giuseppe correct me if I am wrong but I think the issue right now is our Apply functions for each handler need a if cgroupv2 condition that uses the fs2 functions and cgroupv1 uses fs1

@cdoern cdoern force-pushed the cgroup branch 4 times, most recently from e1c4916 to 5fba634 Compare March 1, 2022 03:58
@giuseppe
Copy link
Member

giuseppe commented Mar 1, 2022

I've tried to vendor the current PR into podman but I get:

libpod/stats.go:60:41: cgroupStats.CPU undefined (type *"github.com/opencontainers/runc/libcontainer/cgroups".Stats has no field or method CPU)
libpod/stats.go:66:30: cgroupStats.CPU undefined (type *"github.com/opencontainers/runc/libcontainer/cgroups".Stats has no field or method CPU)
libpod/stats.go:71:30: cgroupStats.Memory undefined (type *"github.com/opencontainers/runc/libcontainer/cgroups".Stats has no field or method Memory)
libpod/stats.go:76:27: cgroupStats.Pids undefined (type *"github.com/opencontainers/runc/libcontainer/cgroups".Stats has no field or method Pids)
libpod/stats.go:79:29: cgroupStats.CPU undefined (type *"github.com/opencontainers/runc/libcontainer/cgroups".Stats has no field or method CPU)
libpod/stats.go:80:35: cgroupStats.CPU undefined (type *"github.com/opencontainers/runc/libcontainer/cgroups".Stats has no field or method CPU)
libpod/stats.go:124:33: undefined: "github.com/containers/common/pkg/cgroups".Metrics
libpod/stats.go:137:30: undefined: "github.com/containers/common/pkg/cgroups".Metrics
libpod/stats.go:80:35: too many errors
make: *** [Makefile:321: bin/podman] Error 2

I am afraid all the new dependencies (~21000 new lines of code) will increase significantly the podman binary size.

Could we pick just the pieces we need?

How difficult would be to implement the resource limits part in our library?

@kolyshkin FYI

@cdoern
Copy link
Contributor Author

cdoern commented Mar 1, 2022

@giuseppe it was about 4,000 lines without the inclusion of fs2, but I needed that for the creation. I think figuring out proper cgrouo creation (if we need it at all) is what is necessary here. The resource handlers should probably be imported directly from runc... Not sure though

@cdoern
Copy link
Contributor Author

cdoern commented Mar 1, 2022

Ok... I now have this working locally, I am going to try and pick parts out until we get to a reasonable mix of c/common code and runc code

@cdoern cdoern force-pushed the cgroup branch 2 times, most recently from 98e51db to 2c44075 Compare March 2, 2022 03:59
@cdoern
Copy link
Contributor Author

cdoern commented Mar 2, 2022

@giuseppe I removed all references to fs2. This brings down the total import to 4000 lines. The cgroupv2 code is pretty much a copy of runc without some of their weird native functions.

Cgroupsv1 uses the runc functions directly just for simplicity. I though that bringing in the cgroupsv2 code directly made sense so we have direct control over it in the future.

@rhatdan
Copy link
Member

rhatdan commented Mar 2, 2022

/approve
LGTM
@giuseppe PTAL

@openshift-ci openshift-ci bot added the approved label Mar 2, 2022
@giuseppe
Copy link
Member

giuseppe commented Mar 2, 2022

so we are using libcontainer for cgroupv1 but our code for cgroupv2?

@giuseppe
Copy link
Member

giuseppe commented Mar 2, 2022

@kolyshkin PTAL

@rhatdan
Copy link
Member

rhatdan commented Mar 2, 2022

It would probably be best if we used libcontainer for both, if possible.

@cdoern
Copy link
Contributor Author

cdoern commented Mar 2, 2022

@rhatdan @giuseppe the V2 implementation is basically libcontainers code minus their usage if the manager interface. Importing their V2 code brings this PR to 20k lines

@cdoern cdoern marked this pull request as ready for review March 2, 2022 16:01
@rhatdan
Copy link
Member

rhatdan commented Jun 7, 2022

Try with this one
go.mod

@rhatdan
Copy link
Member

rhatdan commented Jun 7, 2022

Upgrading to go 1.17 should not be an issue.

@cdoern
Copy link
Contributor Author

cdoern commented Jun 7, 2022

strange... why is this linting things I did not even edit?

@cdoern cdoern force-pushed the cgroup branch 3 times, most recently from 325d551 to f3a36c1 Compare June 7, 2022 20:02
@rhatdan
Copy link
Member

rhatdan commented Jun 7, 2022

I think the linter fires on any file that you change, and forces you to fix other issues.

@cevich
Copy link
Member

cevich commented Jun 7, 2022

@cdoern how about this (untested nor spellchecked):

diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml
index 5eae1b05..8988512b 100644
--- a/.github/workflows/validate.yml
+++ b/.github/workflows/validate.yml
@@ -41,11 +41,15 @@ jobs:
       uses: golangci/golangci-lint-action@537aa1903e5d359d0b27dbc19ddd22c5087f3fbc # v3
       with:
         version: "${{ env.LINT_VERSION }}"
+        only-new-issues: true
     # Extra linters, only checking new code from a pull request.
+    - name: Extra golangci-lint config. switcharoo
+      run: mv .gilangci-extra.yml golangci.yml
     - name: lint-extra
-      if: github.event_name == 'pull_request'
-      run: |
-        golangci-lint run --config .golangci-extra.yml --new-from-rev=HEAD~1 --out-format=github-actions
+      uses: golangci/golangci-lint-action@537aa1903e5d359d0b27dbc19ddd22c5087f3fbc # v3
+      with:
+        version: "${{ env.LINT_VERSION }}"
+        only-new-issues: true
 
     - name: validate seccomp
       run: ./tools/validate_seccomp.sh ./pkg/seccomp

@cdoern cdoern force-pushed the cgroup branch 2 times, most recently from 085f334 to 4b55840 Compare June 7, 2022 20:27
switch c/common to use runc cgroup creation so that we can use resource limits

This entails importing the newly refactored runc code to manage reading from and writing to cgroup.
vendoring in directly an unreleased runc commit from opencontainers/runc#3452

Signed-off-by: cdoern <cdoern@redhat.com>
@cdoern
Copy link
Contributor Author

cdoern commented Jun 8, 2022

@giuseppe @rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Jun 8, 2022

LGTM

@rhatdan
Copy link
Member

rhatdan commented Jun 8, 2022

@cevich @giuseppe @vrothberg PTAL

@cdoern
Copy link
Contributor Author

cdoern commented Jun 8, 2022

vendored into podman: ls -ln bin/podman -rwxrwxr-x. 1 1000 1000 45730352 Jun 8 09:46 bin/podman

head of upstream/main podman: ls -ln bin/podman -rwxrwxr-x. 1 1000 1000 45325592 Jun 8 09:47 bin/podman

very similar binary sizes.

@giuseppe
Copy link
Member

giuseppe commented Jun 8, 2022

vendored into podman: ls -ln bin/podman -rwxrwxr-x. 1 1000 1000 45730352 Jun 8 09:46 bin/podman

does this happen with disable_cgroup_devices?

@cdoern
Copy link
Contributor Author

cdoern commented Jun 8, 2022

@giuseppe I think @kolyshkin 's most recent version of the PR removes the build tag and separates the packages in such a way that you do not import the devices code unless you excitability use it. so yes, this is without the devices,

@cevich
Copy link
Member

cevich commented Jun 8, 2022

The github-action changes LGTM

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 8, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, giuseppe, rhatdan

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

@rhatdan
Copy link
Member

rhatdan commented Jun 8, 2022

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants