lxcfs: fix wrong cpu count when setting cfs in hierarchy

[okhowang]

Resolve #688

[stgraber]

@mihalicyn can you take a look?

[mihalicyn]

Please, reply on my comment in #688 (comment)

[mihalicyn]

Thanks for preparing and submitting this.

I believe that this is on the right direction but we need to slightly rework the code.

Also, please look #688 (comment)

Kind regards,
Alex

[mihalicyn]

Please, also look on #712

It looks like we have two independent contributors who have stepped on the same issue but from a different directions ;-)

[okhowang]

@mihalicyn I couldn't reproduce psi timeout local, it maybe some lag on github actions, can you rerun the CI?

[mihalicyn]

You've started to use get_min_cpu_count_cfs in max_cpu_count, at the same time you still rely on read_cpu_count_cfs here, why?

[okhowang]

it's my fault, I modified here at first, and forgot to updatee here later.

[mihalicyn]

I couldn't reproduce psi timeout local, it maybe some lag on github actions, can you rerun the CI?

yeah, I'll take care of this problem. It is not connected with this PR for sure.

[mihalicyn]

@okhowang the more I look and find issues here, the more I feel that we need to add a test for --enable-cfs. You can take some inspiration from https://github.com/lxc/lxcfs/blob/main/tests/test_proc.in. I believe we should spawn a separate LXCFS daemon with --enable-cfs parameter passed and run tests. You can do a draft implementation of tests to cover your use-case and validate that without this PR tests fails and with fix from this PR test passes. I'll then clean this up and figure out what is the best way to combine it with our CI pipelines.

[okhowang]

sorry for my miss, there are lot of month since my first work on lxcfs.
and I forgot meson install before testing, so I tested with old liblxcfs.so.

I added a commit for test_cfs to test read cpu/memory in child cgroup. you can checkout the commit and test will failed.
the fixes worked after the testing commit.

[mihalicyn]

Hi @okhowang,

please can you rebase your branch on top of recent main. I've added a bunch of new tests (#722), specifically https://github.com/lxc/lxcfs/blob/1881408808ac822867be56dc5b998645d415ac3a/tests/test_cpu_cfs_hierarchy.sh.in

Also, please, remove:

# TODO: remove after fixing https://github.com/lxc/lxcfs/issues/688
echo '300000 100000' > "/sys/fs/cgroup/${cg1}/${cg2}/${cg3}/cpu.max"

chunk from (

lxcfs/tests/test_cpu_cfs_hierarchy.sh.in

Line 72 in 1881408

# TODO: remove after fixing https://github.com/lxc/lxcfs/issues/688

) and put this change into a separate commit.

This will allow us to validate if this PR fixes everything properly.

Kind regards,
Alex

[mihalicyn]

rebase is needed

[okhowang]

rebase is needed需要变基

done.
BTY, is my test commit duplicated? or should I remove it from PR?

I noticed that cgroup v1 has been dropped.
should I remove branch for cgroup v1 in the PR?

[codecov]

Codecov Report

❌ Patch coverage is 47.50000% with 21 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/proc_cpuview.c	48.64%	11 Missing and 8 partials ⚠️
src/proc_fuse.c	33.33%	0 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[mihalicyn]

Hi @okhowang,

yes, please remove the test you've added, because the version we have in main is more advanced and covers this stuff better.

[okhowang]

Hi @okhowang,

yes, please remove the test you've added, because the version we have in main is more advanced and covers this stuff better.

test commit removed

[mihalicyn]

LGTM

Thanks, @okhowang !