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

test: skip oomd test on a unified container on a hybrid host #20705

Merged
merged 5 commits into from
Sep 12, 2021

Conversation

yuwata
Copy link
Member

@yuwata yuwata commented Sep 10, 2021

Fixes #20593
Fixes #20655.

@yuwata
Copy link
Member Author

yuwata commented Sep 10, 2021

(I have no idea what we should do in oomd on such a case.)

@yuwata
Copy link
Member Author

yuwata commented Sep 10, 2021

cc @anitazha and @DaanDeMeyer

@DaanDeMeyer
Copy link
Contributor

Slightly out of my league as well. Let's wait for anita.

@anitazha
Copy link
Member

I need to check in more detail when I'm not AFK but memory.current is provided when the memory controller is enabled. So I think the correct message would be something like "cgroup2 memory controller is not enabled". And the actual daemon should also include a condition check for the memory controller.

@yuwata
Copy link
Member Author

yuwata commented Sep 11, 2021

Ah, checking the existence of memory controller is better. Thanks. Will update.

@yuwata yuwata added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Sep 11, 2021
@yuwata
Copy link
Member Author

yuwata commented Sep 11, 2021

@anitazha Updated. PTAL.

@yuwata yuwata added cgroups and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Sep 11, 2021
@bluca
Copy link
Member

bluca commented Sep 11, 2021

test-oomd still unhappy

@yuwata yuwata added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Sep 11, 2021
@yuwata yuwata added please-review and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Sep 11, 2021
@yuwata
Copy link
Member Author

yuwata commented Sep 11, 2021

Still not sure if this is a correct way, but at least, now semaphoreci is green. PTAL.

@yuwata
Copy link
Member Author

yuwata commented Sep 11, 2021

What's the difference between cg_kernel_controllers() and cg_mask_supported()??

Copy link
Member

@anitazha anitazha left a comment

Choose a reason for hiding this comment

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

What's the difference between cg_kernel_controllers() and cg_mask_supported()??

It looks like cg_kernel_controllers() parses /proc/cgroup which lists all the controllers (cgroup v1 and v2) compiled in the kernel (including information about how many cgroups are using them and whether they were disabled via boot parameter, etc.). But cg_mask_supported() will figure out which hierarchy systemd is using and list the available controllers based on the current hierarchy. For unified (cgroup v2) this will be /sys/fs/cgroups/cgroup.controllers.

Thanks for fixing this Yu the current approach lgtm. Should be good to merge after removing the calls to cg_kernel_controllers()

src/oom/oomd.c Outdated Show resolved Hide resolved
src/oom/test-oomd-util.c Outdated Show resolved Hide resolved
@yuwata
Copy link
Member Author

yuwata commented Sep 12, 2021

@anitazha Thank you for your review and the explanation. cg_kernel_controllers() are dropped. Upgrading the green label.

@yuwata yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Sep 12, 2021
Copy link
Member

@bluca bluca 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 fixing this, we can go back to seeing all green ticks yay

@bluca bluca merged commit 54966b7 into systemd:main Sep 12, 2021
@yuwata yuwata deleted the test-oomd-util branch September 12, 2021 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cgroups good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed oomd tests units
Development

Successfully merging this pull request may close these issues.

test-oomd-util keeps failing in Semaphore CI test-oomd-util fails in container
4 participants