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

cirrus: enable cgroups v2 tests with crun #3796

Merged

Conversation

giuseppe
Copy link
Member

Signed-off-by: Giuseppe Scrivano [email protected]

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2019
@openshift-ci-robot
Copy link
Collaborator

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

rhatdan commented Aug 12, 2019

LGTM
Don't see why this is a WIP. If it passes it should be merged.

@giuseppe giuseppe force-pushed the enable-cgroupsv2-crun branch 2 times, most recently from 89eb57e to a418a2f Compare August 13, 2019 10:53
@giuseppe giuseppe force-pushed the enable-cgroupsv2-crun branch 6 times, most recently from 8c201fb to 003b9c8 Compare August 13, 2019 14:38
@giuseppe
Copy link
Member Author

LGTM
Don't see why this is a WIP. If it passes it should be merged.

I've opened the PR just to see what would fail and keep working on it. I had expected a lot of failures, as the cgroup v2 backend is not really tested. From 70+ failures it went down now to a few ones. There were a couple of issues in crun, but most of the failures were related to the tests not ready for cgroups v2.

@giuseppe giuseppe changed the title [WIP] cirrus: enable cgroups v2 tests with crun cirrus: enable cgroups v2 tests with crun Aug 13, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 13, 2019
@giuseppe
Copy link
Member Author

🎉 🎉 🎉 cgroups 2 tests are passing now 🎉 🎉 🎉

@giuseppe
Copy link
Member Author

it is using the master branch for crun. I'll cut a new release in the next days

the option is --quiet, not --q

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe
Copy link
Member Author

tests are all green

@giuseppe
Copy link
Member Author

@cevich @mheon PTAL

@mheon
Copy link
Member

mheon commented Aug 14, 2019

This is just test changes, right?

I don't like the duplication around determining if we're in V2 mode, but I don't know if we can remove it easily.

Otherwise LGTM

@giuseppe
Copy link
Member Author

yes, it is just tests code changes beside: 04d333f

@giuseppe
Copy link
Member Author

@cevich I think we will need to enable the same tests on the crun repository, so that we don't risk to break Podman. How difficult would it be to run Podman tests there?

Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

I'm with Matt, that's a lot of duplication. IMHO, this PR should depend on #3754 (includes crun package) rather than inline package + source install (see my comments above). It's one of my top priorities this week to get that merged ASAP - CI in other repos depend on it as well.

# yum install -y crun
setenforce 0
yum builddep -y crun
(git clone --depth=1 https://github.com/containers/crun && cd crun && ./autogen.sh && ./configure --prefix=/usr && make -j4 && make install)
Copy link
Member

Choose a reason for hiding this comment

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

The VM images from PR #3754 already include the crun package. Unless there's a really good reason for it, I'm against having both the package installed AND installing from source at the same time. It creates a nightmare for both test/automation maintenance and seriously complicates debugging.

If using the source is required at this early stage, this sub-shell build/install should be moved into a dedicated lib.sh function to support readability and ease of maintenance. Also let me know so I can drop the dnf install -y crun from my PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to install from source for now, as master contains some fixes necessary to get the tests passing

make
make install PREFIX=/usr ETCDIR=/etc
make test-binaries
echo "WARNING: Integration tests not yet ready for cgroups V2"
#TODO: make local${TESTSUITE}
make local${TESTSUITE}
Copy link
Member

Choose a reason for hiding this comment

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

Are you intending to support running of the system-tests? If so, please add to .cirrus.yml:

special_testing_cgroupv2_task
    ...
    system_test_script: '$SCRIPT_BASE/system_test.sh |& ${TIMESTAMP}'

Copy link
Member Author

Choose a reason for hiding this comment

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

I've not yet looked into them, I'd expect them to fail at this point. We can add them later, once we are sure they work with a different OCI runtime

@cevich
Copy link
Member

cevich commented Aug 14, 2019

How difficult would it be to run Podman tests there?

I'm not a big fan of running "child" tests in "parent" projects, it tends to result in very messy environments (in the parent) which don't accurately test compatibility and require an extraordinary amount of maintenance and duct-tape.

OTOH, I see that crun looks to already have some dedicated testing setup in travis?.

If you need help converting or extend that to something more modern than old/crusty Ubuntu Xenial host environment, that's something I could help with. It's not trivial, but having existing/functional automation helps a lot.

If it's a short/medium term automation need, I can get something going in a day or two worth of work (need a jira card). If there's a chance it will be long-term (beyond 6-months), then I'd prefer to setup a dedicated cloud-environment (like we do for c/storage, buildah, etc.). That adds about 4-hours of work for me but a few days waiting for e-mail requests/approvals for funding. I'm fine either way, but would rather get started on dedicated long-term stuff early, if that's what is needed (also needs Jira card)

@giuseppe
Copy link
Member Author

if it is too much effort, I can open WIP PR to containers/libpod once in a while and before cutting a new release to verify it still works

@giuseppe
Copy link
Member Author

I'm with Matt, that's a lot of duplication. IMHO, this PR should depend on #3754 (includes crun package) rather than inline package + source install (see my comments above). It's one of my top priorities this week to get that merged ASAP - CI in other repos depend on it as well.

for now we need to use crun from git as there are fixes required for the tests. I'll cut a new release and then we can use the package

@cevich
Copy link
Member

cevich commented Aug 14, 2019

if it is too much effort, I can open WIP PR to containers/libpod once in a while and before cutting a new release to verify it still works

That would probably be okay a few times, then get to be a major pain-point, don't you think?

Since crun is required for this cgroupsv2 test, it'll get a fairly big safety net that way (it's going to run for everybody's changes and post-merge also). It sounds like you're mainly worried about new, crun-specific changes affecting downstream compatibility, eh?

Traditionally, this would be where crun would have it's own integration and system-tests, as a kind of "contract" with the "outside world" (libpod). So for example, if downstream changes in libpod require different crun behaviors, the tests in crun need to be modified accordingly.

Honestly there are simply not a lot of quick/cheap/easy ways to solve this, which also play nicely with (potentially many) other parallel callers/users/developers of different crun versions/releases (including libpod). Testing and automation is just hard work sometimes.

That said, I'm willing to do most/all the automation implementation work. All I need is you/other community members to support me if I have crun-specific issues/questions along the way. It's way better if we plan and get it right/good early on. Doing/Re-doing it later is almost always much more difficult.

Which jira cards should I write up for this (I'll CC you), short/medium term or long-term or other?

@cevich
Copy link
Member

cevich commented Aug 14, 2019

for now we need to use crun from git as there are fixes required for the tests. I'll cut a new release and then we can use the package

Okay, that's fine if it's just for now. Just please make sure there's a "post-it note" (or jira card or in-line comment) somewhere so it's actually "just for now". I can't say how many times these have slipped and become permanent debugging-nightmares for $FUTURE_DEVELOPER. Best to avoid that.

@giuseppe
Copy link
Member Author

Which jira cards should I write up for this (I'll CC you), short/medium term or long-term or other?

Fedora 31 is getting closer, so I think we should try to get at least these tests running for Podman as soon as we can, and have some sort of confidence with cgroups v2.
We can discuss the details next week on how to coordinate and proceed with the crun integration.

@rhatdan
Copy link
Member

rhatdan commented Aug 16, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2019
@openshift-merge-robot openshift-merge-robot merged commit 2d47f1a into containers:master Aug 16, 2019
@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.
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
Development

Successfully merging this pull request may close these issues.

6 participants