[ci] Enable Cgroup support in CI for core #51454
Conversation
1. Add support for privileged containers in linux_tester_container 2. Add cgroup bazel config for writable sandbox Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
correctly! Signed-off-by: Ibrahim Rabbani <[email protected]>
containers with --privilege enabled Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
israbbani
added
go
Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
israbbani
added
go
Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
israbbani
changed the title
| build:ubsan --linkopt -fno-sanitize-recover=all | ||
| build:ubsan --per_file_copt="-external/com_github_grpc_grpc/.*@-fsanitize=undefined" | ||
|
|
||
| build:cgroup --sandbox_writable_path=/sys/fs/cgroup --config=llvm |
There was a problem hiding this comment.
curious what is --config=llvm?
There was a problem hiding this comment.
it means inherit build:llvm
There was a problem hiding this comment.
It's defined in .bazelrc file. I'm not sure if this is the right one for what we want. @aslonnie maybe --config=clang is better because it's consistent with our other cpp tests?
There was a problem hiding this comment.
rename can happen in another follow up.
@israbbani maybe checkin the change of this .bazelrc file first (and I can approve it), and then your iteration on this PR can have much better cache hit rate
There was a problem hiding this comment.
I think for some reason the cache hit rate has improved on its own. Maybe one of the microchecks passed and published the docker image?
There was a problem hiding this comment.
it means inherit build:llvm
Sorry for the confusion, my real question is why do we need the config?
There was a problem hiding this comment.
@aslonnie asked me to include it as the base config for C++ testing. It adds the following properties:
# LLVM (clang & libc++) build flags common across Linux installations and systems.
# On Ubuntu, the remaining configurations can be generated by running ci/env/install-llvm-binaries.sh
build:llvm --action_env=CXXFLAGS=-stdlib=libc++
build:llvm --action_env=LDFLAGS=-stdlib=libc++
build:llvm --action_env=BAZEL_CXXOPTS=-stdlib=libc++
build:llvm --action_env=BAZEL_LINKLIBS=-l%:libc++.a:-l%:libc++abi.a
build:llvm --action_env=BAZEL_LINKOPTS=-lm:-pthread
build:llvm --define force_libcpp=enabled
My understanding is that it's needed if we're going to use the cgroup config to run C++ tests. Are you asking why we need the cgroup config?
There was a problem hiding this comment.
Are you asking why we need the cgroup config?
No, I'm just curious why do we need these llvm config.
Because I don't see it in all C++ related configs, for example
Lines 128 to 134 in 5e05c2f
There was a problem hiding this comment.
This is a good question. We may need to audit these and removes one that we don't need.
|
Alright. I've addressed the comments and I did manual verification. I think this is good to go. @aslonnie PTAL. |
Signed-off-by: Ibrahim Rabbani <[email protected]>
ci/ray_ci/container.py
Outdated
| if self.privileged: | ||
| command.append("--privileged") |
There was a problem hiding this comment.
why it needs to be added at this location? this split of command feels a bit weird..
There was a problem hiding this comment.
I was having trouble getting it to work if the --privileged flag was at the end of the command. It works now so it must've been another problem. I moved it back
Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
dentiny
left a comment
dentiny
left a comment
There was a problem hiding this comment.
Thank you so much for the amazing work!
Enable CI environment with writeable cgroupv2 in CI. To use this environment for testing, you need to use the `--privileged-container` flag in your test definitions. To enable bazel read/write to sys/fs/cgroup in your tests, you need to use the `--build-type cgroup` --------- Signed-off-by: Ibrahim Rabbani <[email protected]>
Enable CI environment with writeable cgroupv2 in CI. To use this environment for testing, you need to use the `--privileged-container` flag in your test definitions. To enable bazel read/write to sys/fs/cgroup in your tests, you need to use the `--build-type cgroup` --------- Signed-off-by: Ibrahim Rabbani <[email protected]> Signed-off-by: Dhakshin Suriakannu <[email protected]>
Enable CI environment with writeable cgroupv2 in CI. To use this environment for testing, you need to use the `--privileged-container` flag in your test definitions. To enable bazel read/write to sys/fs/cgroup in your tests, you need to use the `--build-type cgroup` --------- Signed-off-by: Ibrahim Rabbani <[email protected]> Signed-off-by: Srinath Krishnamachari <[email protected]>
dentiny
left a comment
dentiny
left a comment
There was a problem hiding this comment.
Some post-merge comments (correct me if wrong):
- We need to have documentation on how cgroup tests could be enabled in CI, because it's different with normal C++ tests;
- It looks to me we need to filter cgroup tests out for normal C++ tests by tags, and enable cgroup tests by tags (using
--only-tags)
I could make updates if that sounds reasonable to you. |
Enable CI environment with writeable cgroupv2 in CI.
To use this environment for testing, you need to use the
--privileged-containerflag in your test definitions.To enable bazel read/write to sys/fs/cgroup in your tests, you need to use the
--build-type cgroup