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.

Sign up for GitHub

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

Jump to bottom

Add an integration test for systemd in a container #3728

Merged
merged 3 commits into from
Aug 28, 2019
Merged

Add an integration test for systemd in a container #3728

merged 3 commits into from
Aug 28, 2019

Conversation

mheon
Copy link
Member

@mheon mheon commented Aug 5, 2019

We regressed really badly here. Add a test so we don't do it again.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

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-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M labels Aug 5, 2019
@mheon
Copy link
Member Author

mheon commented Aug 5, 2019

I fully expect this is failing right now. Will merge once master is fixed.

@rhatdan
Copy link
Member

rhatdan commented Aug 5, 2019

LGTM

@TomSweeneyRedHat
Copy link
Member

LGTM once tests are happy

@giuseppe
Copy link
Member

giuseppe commented Aug 6, 2019

LGTM, PR to fix the issue here: #3731

lslebodn
lslebodn reviewed Aug 6, 2019
View reviewed changes
test/e2e/systemd_test.go Outdated
Expect(pull.ExitCode()).To(Equal(0))

ctrName := "testSystemd"
run := podmanTest.Podman([]string{"run", "--name", ctrName, "-d", systemdImage, "init", "--log-level", "debug"})
Copy link

Choose a reason for hiding this comment

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

IIUC, this line requires oci-systemd-hook to be instalelld on system.

What do you think about another test-case with without that hook? (or with env oci-systemd-hook=disabled)

Copy link
Member

Choose a reason for hiding this comment

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

podman should not require oci-systemd-hook at all.

Copy link

Choose a reason for hiding this comment

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

ok; tahnk you for explanation.

@mheon mheon force-pushed the systemd_container_test branch from 38cd916 to 65d789c Compare August 15, 2019 16:53
@mheon
Copy link
Member Author

mheon commented Aug 15, 2019

Rebased. Hopefully will go green now.

@rhatdan
Copy link
Member

rhatdan commented Aug 22, 2019

@mheon Could you rebase this and see if we can get it merged.

@mheon mheon force-pushed the systemd_container_test branch 3 times, most recently from bd51b7a to 1d5ed6d Compare August 26, 2019 14:21
@mheon
Copy link
Member Author

mheon commented Aug 26, 2019

Alright, should be getting actual logs out of the tests now - so we should know where this is failing. I suspect it's CGroups related but not 100%.

@mheon mheon force-pushed the systemd_container_test branch from 1d5ed6d to 39afba2 Compare August 26, 2019 14:25
@mheon
Copy link
Member Author

mheon commented Aug 26, 2019 

�[0;1;31mFailed to create /machine.slice/libpod-2a66ed884c8a8266f8c9874afad98bc1533541f00c143814031a2e6ece3ada8a.scope/init.scope control group: Permission denied�[0m �[0;1;31mFailed to allocate manager object: Permission denied�[0m [�[0;1;31m!!!!!!�[0m] Failed to allocate manager object. �[0;1;31mExiting PID 1...�[0m

Some broken characters - encoding on our CI logs seems a bit messed up? - but it looks like a CGroup error setting up systemd on every test that isn't remote and actually uses CGroups.

@mheon
Copy link
Member Author

mheon commented Aug 26, 2019

I think this might be a legitimate issue on Podman's side?

@mheon mheon force-pushed the systemd_container_test branch from 39afba2 to 4100102 Compare August 26, 2019 20:38
@mheon
Copy link
Member Author

mheon commented Aug 27, 2019

Aug 26 17:03:44 cirrus-task-5556945475338240 audit[9996]: AVC avc: denied { write } for pid=9996 comm="systemd" name="libpod-7035b3c10c07caf8cdf8f13e9ad4af80e30150007016f798b3af48ab1c07e04b.scope" dev="cgroup" ino=4713 scontext=system_u:system_r:container_t:s0:c219,c823 tcontext=system_u:object_r:cgroup_t:s0 tclass=dir permissive=0

@mheon
Copy link
Member Author

mheon commented Aug 27, 2019

Well, damn. It's the container manage CGroups SEBool. For everything that's not ubuntu.

@mheon
Copy link
Member Author

mheon commented Aug 27, 2019

@cevich PTAL at the last commit - that look like a good place for SELinux config for the tests?

@mheon mheon force-pushed the systemd_container_test branch from 8add937 to 45aa084 Compare August 27, 2019 16:44
@mheon
Copy link
Member Author

mheon commented Aug 27, 2019

Squashed everything down. Hopefully will go green now.

@mheon
Copy link
Member Author

mheon commented Aug 27, 2019

Everything is passing except CGroups v2

@mheon
Copy link
Member Author

mheon commented Aug 27, 2019 

[+0966s] Error: writing file '/sys/fs/cgroup//machine.slice/libpod-dfeb5085da36bb458491e7e3311d0845f8ace4699d1a88b67824c6611fd623f5.scope/cgroup.procs': Device or resource busy: OCI runtime error

I feel like this is an actual error with our V2 support
@giuseppe PTAL when you get back from PTO

@rhatdan
Copy link
Member

rhatdan commented Aug 27, 2019

Can we tell the tests to not run with cgroupsV2 and get this merged and then open an issue for @giuseppe to fix the cgroupsV2 issue.

@mheon
Copy link
Member Author

mheon commented Aug 27, 2019

I'll look into it - should be possible

@rhatdan
Copy link
Member

rhatdan commented Aug 27, 2019 

cgroupsv2, _ := cgroups.IsCgroup2UnifiedMode()
if cgroupv2 {
        skip("systemd does not work in cgroups V2 mode yet")
}

@mheon
Copy link
Member Author

mheon commented Aug 27, 2019

Done. Let's see if things go green now.

cevich
cevich reviewed Aug 28, 2019
View reviewed changes
contrib/cirrus/setup_environment.sh
if [[ "$ADD_SECOND_PARTITION" == "true" ]]; then
bash "$SCRIPT_BASE/add_second_partition.sh"; fi
;;
centos-7) # Current VM is an image-builder-image no local podman/testing
echo "No further setup required for VM image building"
# All SELinux distros need this for systemd-in-a-container
Copy link
Member

Choose a reason for hiding this comment

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

Please leave original comment or update to clarify: CentOS-7 is never used for testing (we would use RHEL instead). This condition is needed because the cache-image building VM shares setup_environment.sh (hence the comment) and 'exit 0' to avoid doing any actual setup for testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, I'll restore the original

cevich
cevich reviewed Aug 28, 2019
View reviewed changes
contrib/cirrus/setup_environment.sh
@@ -44,11 +44,13 @@ case "${OS_REL_VER}" in
;;
fedora-30) ;& # continue to next item
fedora-29)
# All SELinux distros need this for systemd-in-a-container
setsebool container_manage_cgroup true
Copy link
Member

Choose a reason for hiding this comment

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

Yep this is perfectly fine place to do this.

baude and others added 3 commits August 28, 2019 09:28
@baude @mheon
clean up after remote build
8b09b07 
when performing an image build over a varlink connection, we should
clean up tmp files that are a result of sending the file to the host and
untarring it for the build.

Fixes: containers#3869

Signed-off-by: baude <[email protected]>
@mheon
Add an integration test for systemd in a container
63d989a 
Signed-off-by: Matthew Heon <[email protected]>
@mheon
Temporarily disable systemd test for CGroups V2
ca0dfca 
Revert this one CGroups V2 support for systemd containers is
added.

Signed-off-by: Matthew Heon <[email protected]>
@mheon mheon force-pushed the systemd_container_test branch from f7dfc59 to ca0dfca Compare August 28, 2019 14:58
@mheon
Copy link
Member Author

mheon commented Aug 28, 2019

/retest

@mheon
Copy link
Member Author

mheon commented Aug 28, 2019

@baude @rhatdan @vrothberg @TomSweeneyRedHat PTAL

@TomSweeneyRedHat
Copy link
Member

LGTM and all green test buttons.

@baude
Copy link
Member

baude commented Aug 28, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2019
@openshift-merge-robot openshift-merge-robot merged commit ced3ebb into containers:master Aug 28, 2019
This was referenced Aug 28, 2019
Merged
Merged
Closed
Merged
@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 Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lslebodn lslebodn lslebodn left review comments

@rhatdan rhatdan rhatdan left review comments

@cevich cevich cevich left review comments

@baude baude Awaiting requested review from baude

@giuseppe giuseppe Awaiting requested review from giuseppe

Assignees

@baude baude

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mheon @openshift-ci-robot @rhatdan @TomSweeneyRedHat @giuseppe @baude @cevich @lslebodn @openshift-merge-robot