Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

stage1: remount entire subcgroup r/w, instead of each knob #3494

Merged
merged 1 commit into from
Jan 13, 2017

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Dec 22, 2016

This remounts the entire subcgroup (machine slice) read-write, instead of only the exact knobs per-app we need.

This is needed for runc (since it wants to manage cgroups for itself). It also simplifies app add, since we no longer need to manipulate cgroups after pod start time. It also reduces the number of bind mounts to one per controller, making various operations more efficient. (Before, we had one bind mount per controller knob per app).

@@ -332,9 +321,9 @@ func CreateCgroups(m fs.Mounter, root string, enabledCgroups map[int][]string, m

// RemountCgroups remounts the v1 cgroup hierarchy under root.
// It mounts /sys/fs/cgroup/[controller] read-only,
// but leaves needed knobs in the subcgroup for each app read-write,
// but leaves needed knobs in the subcgroup the pod read-write,
Copy link
Contributor

Choose a reason for hiding this comment

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

subcgroup the pod

//
// cpuSet should be <stage1rootfs>/sys/fs/cgroup/cpuset
func fixCpusetKnobs(cpusetPath, subcgroup string) error {
//cgroupPathFix := filepath.Join(cpusetPath, "system.slice")
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

_ = os.MkdirAll(cgroupPathFix, 0755)
// Ensure that the hierarchy has consistent cpu restrictions.
//
// cpuSet should be <stage1rootfs>/sys/fs/cgroup/cpuset
Copy link
Contributor

Choose a reason for hiding this comment

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

cpusetPath

@jonboulle
Copy link
Contributor

		stage1: couldn't mount the container v1 cgroups

		  └─error restricting container cgroups

		    └─error fixing cpuset knobs

		      └─error writing cgroup stage1/rootfs/sys/fs/cgroup/cpuset/user/1003.user/8.session/machine-rkt\x2d70c66eb8\x2de4bf\x2d49fb\x2d8ba9\x2d8c8e44b80b97.scope/cpuset.mems

		        └─write stage1/rootfs/sys/fs/cgroup/cpuset/user/1003.user/8.session/machine-rkt\x2d70c66eb8\x2de4bf\x2d49fb\x2d8ba9\x2d8c8e44b80b97.scope/cpuset.mems: permission denied

@squeed
Copy link
Contributor Author

squeed commented Dec 23, 2016

Yeah, I saw that failure. The previous version of the function I refactored ignored errors, but i started paying attention. Of course, it Works On My Machine(tm).

@squeed squeed force-pushed the cgroup-machine-slice branch from 43ff2d1 to d55a7e6 Compare December 23, 2016 14:00
@squeed
Copy link
Contributor Author

squeed commented Dec 23, 2016

@s-urbaniak would you mind making sure my reasoning is correct?

This is all a bit moot, since we no longer mount /sys/fs/cgroup in to the stage2, but it still seems prudent.

@s-urbaniak s-urbaniak added this to the v1.23.0 milestone Jan 5, 2017
@lucab
Copy link
Member

lucab commented Jan 12, 2017

I'm fine with this change, after a rebase and after adding a description of the current status of stage0/stage1/stage2 cgroups in the devdocs (cgroups.md) with a short reference to the backstory (starting from #2351)

This mounts the entire subcgroup (machine slice) read-write, instead of
mounting only the exact knobs per-app we need.
@squeed squeed force-pushed the cgroup-machine-slice branch from eb0ac46 to 30ecafb Compare January 12, 2017 17:09
@squeed
Copy link
Contributor Author

squeed commented Jan 12, 2017

@lucab rebased and doc updated, thanks!

@lucab
Copy link
Member

lucab commented Jan 12, 2017

"TestNetDefaultConnectivity":

Hostname received by client `semaphore-1611  OK  ] Started rkt supervisor-ready signaling.` doesn't match `semaphore-1611`

I think that test has an overly strict output matcher, or alternatively shouldn't be run with --debug. Re-triggered, just in case.

Copy link
Member

@lucab lucab 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 for the devdocs.

@jonboulle
Copy link
Contributor

ready to go?

@jonboulle jonboulle merged commit 9bd2171 into rkt:master Jan 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants