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

Update runc/libcontainer to v0.0.6 #18612

Merged
merged 1 commit into from
Dec 14, 2015
Merged

Update runc/libcontainer to v0.0.6 #18612

merged 1 commit into from
Dec 14, 2015

Conversation

mrunalp
Copy link
Contributor

@mrunalp mrunalp commented Dec 11, 2015

This fixes the cgroup-parent for systemd cgroup implementation (opencontainers/runc#336)

For other changes, see https://github.com/opencontainers/runc/releases/tag/v0.0.6

Signed-off-by: Mrunal Patel [email protected]

@thaJeztah
Copy link
Member

Does this also fix issues in this repository? (Just checking)

@thaJeztah
Copy link
Member

Ah, this fixes #16681

@mrunalp
Copy link
Contributor Author

mrunalp commented Dec 11, 2015

@LK4D4 @crosbymichael PTAL

@@ -94,5 +97,10 @@ func New() *configs.Config {
container.AppArmorProfile = "docker-default"
}

if SystemdCgroups {
container.Cgroups.Parent = "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.

Now that system.slice is defined as the default systemd parent, it should be used in Apply (L166) and getSubsystemPath (L408) instead of defining default slice values there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

runc could be used with systemd cgroups support (once proper support is added) without docker integration so we could handle that separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair.

@anusha-ragunathan
Copy link
Contributor

I reviewed the "cgroup-parent on systemd" part of the change.

Can you point out which integration test is responsible for testing this option on systemd?

@mrunalp
Copy link
Contributor Author

mrunalp commented Dec 14, 2015

@anusha-ragunathan AFAIK, the integration tests don't start the daemon with systemd cgroups enabled. Is that something we want to add? @LK4D4 ping

@mrunalp
Copy link
Contributor Author

mrunalp commented Dec 14, 2015

Could someone restart the windows/experimental builds. I suspect that the failures are unrelated to this change.

@thaJeztah
Copy link
Member

@mrunalp done. we're investigating some flaky tests though, so it's possible they fail again

@mrunalp
Copy link
Contributor Author

mrunalp commented Dec 14, 2015

@thaJeztah Thanks!

@jessfraz
Copy link
Contributor

LGTM if jenkins is happy

@anusha-ragunathan
Copy link
Contributor

@mrunalp : IMO, integration tests that set native.cgroupdriver to systemd should be added, if they dont exist already. It would fill a big test gap.

@LK4D4
Copy link
Contributor

LK4D4 commented Dec 14, 2015

@anusha-ragunathan tests are running in a docker container, where there is no systemd.

@mrunalp
Copy link
Contributor Author

mrunalp commented Dec 14, 2015

Experimental tests failed with

FAIL: docker_cli_stats_test.go:94: DockerSuite.TestStatsAllNewContainersAdded

I don't think this is related to this change. Is this a known issue with flakiness? @thaJeztah ?

@mrunalp
Copy link
Contributor Author

mrunalp commented Dec 14, 2015

@LK4D4 @anusha-ragunathan Yep, that is the big hurdle. While it is possible to run systemd in a container, it would require quite some work to make it work for docker testing. Something we can target long term for sure but not a quick solution.

@mrunalp
Copy link
Contributor Author

mrunalp commented Dec 14, 2015

This is all green now! :)

@LK4D4
Copy link
Contributor

LK4D4 commented Dec 14, 2015

Okay, at least nothing failed.
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.

6 participants