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

system tests for update #15462

Merged
merged 1 commit into from Sep 9, 2022
Merged

system tests for update #15462

merged 1 commit into from Sep 9, 2022

Conversation

edsantiago
Copy link
Collaborator

The e2e tests are incomplete, because they're just too hard
for any human to read/maintain. This defines tests in a
table, so they're easily reviewed and updated.

Signed-off-by: Ed Santiago santiago@redhat.com

None

@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 24, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2022
@edsantiago
Copy link
Collaborator Author

DO NOT MERGE. Piggybacked on #15276, which is still unmerged. I just want to see what happens on Ubuntu and aarch64.

@cdoern
Copy link
Collaborator

cdoern commented Aug 24, 2022

thanks for putting this together, I will get to the bottom of the block io device issues, I have an idea about where they're going wrong

@edsantiago
Copy link
Collaborator Author

Interesting:

# podman update --cpu-rt-period=4096 --cpu-shares=512 --cpus=5 --cpuset-cpus=0 --cpuset-mems=0 --memory=1G --memory-swap=3G --memory-reservation=400M 1887d2589e4a56afe957980ae1de00d157800c98d84d77ded20fdb637a759e78
time="2022-08-24T18:31:14Z" level=warning msg="Setting back cgroup configs failed due to error: openat2 /sys/fs/cgroup/cpu,cpuacct/machine.slice/libpod-1887d2589e4a56afe957980ae1de00d157800c98d84d77ded20fdb637a759e78.scope/cpu.rt_period_us: no such file or directory, your state.json and actual configs might be inconsistent."
time="2022-08-24T18:31:14Z" level=error msg="openat2 /sys/fs/cgroup/cpu,cpuacct/machine.slice/libpod-1887d2589e4a56afe957980ae1de00d157800c98d84d77ded20fdb637a759e78.scope/cpu.rt_period_us: no such file or directory"

cpu-rt-period worked on RHEL8.7, that's why I included it here. Debugging it is way beyond my ability.

@edsantiago
Copy link
Collaborator Author

And here's rootless:

$ podman update --memory=1G --memory-swap=3G --memory-reservation=400M 59229b903af86ea36989b550254427e5d05e44f19ff9eee90aedc7ad515896fa
time="2022-08-24T18:29:26Z" level=warning msg="Setting back cgroup configs failed due to error: cannot set memory limit: container could not join or create cgroup, your state.json and actual configs might be inconsistent."
time="2022-08-24T18:29:26Z" level=error msg="cannot set memory limit: container could not join or create cgroup"
Error: `/usr/sbin/runc update --resources=/var/tmp/podman612936139 59229b903af86ea36989b550254427e5d05e44f19ff9eee90aedc7ad515896fa` failed: exit status 1

@edsantiago edsantiago force-pushed the system_tests_for_update branch 2 times, most recently from d5f6f7b to f15a9bf Compare September 1, 2022 20:52
@edsantiago
System tests for podman-update
3e744e5 
The e2e tests are incomplete, because they're just too hard
for any human to read/maintain. This defines tests in a
table, so they're easily reviewed and updated. This makes
it very easy to see which options are actually tested and
which are not, under root/rootless cgroups v1/v2.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago changed the title WIP: system tests for update system tests for update Sep 1, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2022
@edsantiago
Copy link
Collaborator Author

@cdoern PTAL when convenient; this is ready to go AFAIK

@rhatdan
Copy link
Member

rhatdan commented Sep 6, 2022

LGTM

edsantiago
edsantiago commented Sep 8, 2022
View reviewed changes
test/system/280-update.bats
#
# FIXMEs:
# cpu-rt-period (cgv1 only, on cpu/cpu.rt_period_us) works on RHEL8 but not on Ubuntu
# cpu-rt-runtime (cgv1 only, on cpu/cpu.rt_runtime_us) fails: error setting cgroup config for procHooks ...
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UPDATE: #15666 sheds some light on why cpu-rt-runtime doesn't work. I don't have time right now to delve into it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay... as best I understand #15666, the rt options work only on some systems. I'm inclined to just change the above comment from FIXMEs to Notes, with maybe a link to that issue and an indication that those flags just aren't worth the hassle of testing.

@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2022

@giuseppe PTAL

vrothberg
vrothberg reviewed Sep 9, 2022
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment •
edited

Choose a reason for hiding this comment

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

/lgtm

Let's get this in. The FIXMEs leave breadcrumbs to follow up.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2022
@openshift-merge-robot openshift-merge-robot merged commit b239966 into containers:main Sep 9, 2022
@edsantiago edsantiago deleted the system_tests_for_update branch September 9, 2022 13:34
@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 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg left review comments

Assignees

@vrothberg vrothberg

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. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@edsantiago @cdoern @rhatdan @vrothberg @openshift-merge-robot