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

chore: add sanitizers to ci workflow #4462

Merged
merged 18 commits into from
Jan 22, 2025
Merged

chore: add sanitizers to ci workflow #4462

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5eb421a
chore: add sanitizers to ci workflow
kostasrim Jan 15, 2025
d44a95e
add missing run
kostasrim Jan 15, 2025
3ca3d41
try qith single quote
kostasrim Jan 15, 2025
5dd131f
add missing double quotes
kostasrim Jan 15, 2025
7648050
try
kostasrim Jan 15, 2025
079d1af
small fix
kostasrim Jan 15, 2025
b3d657e
comments
kostasrim Jan 16, 2025
9c29cf7
clean up
kostasrim Jan 16, 2025
5e409e3
try
kostasrim Jan 16, 2025
9753d98
remove exclude
kostasrim Jan 16, 2025
f10bd94
comment out sscache
kostasrim Jan 16, 2025
41e2fb4
comment out alpine
kostasrim Jan 16, 2025
536478c
comment out alpine
kostasrim Jan 16, 2025
b6f346b
fix
kostasrim Jan 16, 2025
0d98ac2
alpine
kostasrim Jan 17, 2025
a9a90b1
Merge branch 'main' into kpr6
kostasrim Jan 20, 2025
0678d5c
conflicts
kostasrim Jan 20, 2025
0a8f415
Merge branch 'main' into kpr6
kostasrim Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 51 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,25 @@ jobs:
strategy:
matrix:
# Test of these containers
container: ["ubuntu-dev:20", "alpine-dev:latest"]
container: ["ubuntu-dev:20", "alpine-dev:latest", "ubuntu-dev:24"]
build-type: [Debug, Release]
compiler: [{ cxx: g++, c: gcc }]
cxx_flags: ["-Werror"]
sanitizers: ["Plain"]
romange marked this conversation as resolved.
Show resolved Hide resolved
include:
- container: "alpine-dev:latest"
build-type: Debug
compiler: { cxx: clang++, c: clang }
cxx_flags: ""
sanitizers: "Plain"
- container: "ubuntu-dev:24"
build-type: Debug
compiler: { cxx: clang++, c: clang }
cxx_flags: ""
sanitizers: "Sanitizers"
exclude:
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you add it to the matrix.containers list if you exclude it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we would generate a bunch more job. exclude is filter on the matrix level. GH action syntax is missleading here :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You basically "exclude" all the jobs that run on ubuntu-24 container and are not sanitizers. Why? Because otherwise we would have a release ubuntu-24, debug ubuntu-24, etc. That way you get only one job sanitizers + ubuntu 24 with clang

Copy link
Contributor Author

Choose a reason for hiding this comment

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

include above is the opposite. it acts as a "decorator" overwritting the matrix value

Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I understand include adds and exclude removes. here you remove all the ubuntu-dev:24 defined in the matrix above therefor I think that if you remove it from the matrix it will be enough we will just run them from the include

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try

- container: "ubuntu-dev:24"
sanitizers: "Plain"

runs-on: ubuntu-latest
env:
Expand Down Expand Up @@ -99,7 +109,8 @@ jobs:
core.exportVariable('ACTIONS_CACHE_URL', process.env.ACTIONS_CACHE_URL || '');
core.exportVariable('ACTIONS_RUNTIME_TOKEN', process.env.ACTIONS_RUNTIME_TOKEN || '')

- name: Configure CMake
- name: Configure CMake for plain build
if: matrix.sanitizers == 'Plain'
# Configure CMake in a 'build' subdirectory. `CMAKE_BUILD_TYPE` is only required if you are using a single-configuration generator such as make.
# See https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html?highlight=cmake_build_type
run: |
Expand All @@ -116,6 +127,35 @@ jobs:
cd ${GITHUB_WORKSPACE}/build && pwd
du -hcs _deps/

- name: Configure CMake for sanitizers build
if: matrix.sanitizers == 'Sanitizers'
run: |
apt -y update
apt -y upgrade
apt install -y clang
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need this step for sanitizers but not for alpine build that uses clang?

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets make this a separate step to just install clang

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because alpine comes prebundled with clang

lets make this a separate step to just install clang

IMO this is bad as well. It should be prebundled and should go under container foundtry and not as part of this workflow file. See: https://github.com/romange/container-foundry/blob/main/u24.04-dev.Dockerfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

romange/container-foundry#14

Copy link
Collaborator

Choose a reason for hiding this comment

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

please follow up on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will, let it be for now

which clang
echo "ulimit is"
ulimit -s
echo "-----------------------------"
echo "disk space is:"
df -h
echo "-----------------------------"
mkdir -p $GITHUB_WORKSPACE/build
cd $GITHUB_WORKSPACE/build
cmake .. \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we can make only one build step for sanitizers and non sanitizers and control the configure all the flags in under the matrix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another downside of this: [build (alpine-dev:latest, Release, g++, gcc, -Werror, NoSanitizers, OFF, OFF)](https://github.com/dragonflydb/dragonfly/actions/runs/12804933055/job/35700274090?pr=4462#logs)

See https://github.com/dragonflydb/dragonfly/actions/runs/12804933055/job/35700274090?pr=4462

(I will figure out something 😄 -- need to expiriment )

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok I suggest use https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/running-variations-of-jobs-in-a-workflow to understand what is the correct why to do this

-DCMAKE_BUILD_TYPE=Debug \
Copy link
Collaborator

Choose a reason for hiding this comment

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

matrix.build-type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice!

-GNinja \
-DCMAKE_C_COMPILER="${{matrix.compiler.c}}" \
-DCMAKE_CXX_COMPILER="${{matrix.compiler.cxx}}" \
-DCMAKE_C_COMPILER_LAUNCHER=sccache \
-DCMAKE_CXX_COMPILER_LAUNCHER=sccache \
-DCMAKE_CXX_FLAGS="${{matrix.cxx_flags}}" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

this line appears twice

-DWITH_ASAN=ON \
-DWITH_USAN=ON \
-DCMAKE_C_FLAGS=-Wno-error=unused-command-line-argument \
-DCMAKE_CXX_FLAGS=-Wno-error=unused-command-line-argument
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be passed from matrix.cxx_flags

# https://maskray.me/blog/2023-08-25-clang-wunused-command-line-argument (search for compiler-rt)

- name: Build
run: |
cd ${GITHUB_WORKSPACE}/build
Expand All @@ -130,7 +170,14 @@ jobs:
echo "disk space is:"
df -h

- name: C++ Unit Tests
- name: C++ Unit Tests sanitizers
if: matrix.sanitizers == 'Sanitizers'
run: |
cd ${GITHUB_WORKSPACE}/build
ctest -V -L DFLY
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the reason for different unit test run for sanitizers and the "plain"?

Copy link
Contributor Author

@kostasrim kostasrim Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We run unit tests with epoll and without and also some more other manually (for the non sanitizers build) 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

you are stating what we are doing. I can see that and therefor I ask what is the reason for this?
Why not run the same for both?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that if you unite the step and run the same tests for both you will not need the matrix.sanitizers at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Helio is a third party lib, not part of dragonfly. We are using sanitizers to test dragonfly and not helio's proactor (iouring or epoll). Furthermore, you are trying to to bring everything under the same umbrella. I don't think this is a good thing. The end goal would be to have separate actions, one for normal builds and one for sanitizers. That way, as we change things, workflows are separate and self contained (similar to what we do with regression tests, that we have action which we parametarize).

Anyway, these are not important for now. I will unify them for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that unifying the steps fro different builds (similarly to code quadruplication )

  1. removes rows and simplifies
  2. if we need to do changes we dont need to remember to fix both

Unlsess there is a reason to not unify them I dont see why not.
We want the sanitizers to run on different flows, there is a reason we have in the build running tests with different flags for example cluster emulated / lock on hash tag. If we run the sanitizers on this flows as well we might catch different failures.

Similar to unifying the bulild step, you see that we added some params to the build f.e HELIO_STACK_CHECK that we forgot to add to the sanitizers bulid. once we unify this there is nothing to remember


- name: C++ Unit Tests plain
if: matrix.sanitizers == 'Plain'
run: |
cd ${GITHUB_WORKSPACE}/build
echo Run ctest -V -L DFLY
Expand Down Expand Up @@ -161,6 +208,7 @@ jobs:
timeout 5m ./dragonfly_test
timeout 5m ./json_family_test --jsonpathv2=false
timeout 5m ./tiered_storage_test --vmodule=db_slice=2 --logtostderr

- name: Upload unit logs on failure
if: failure()
uses: actions/upload-artifact@v4
Expand Down
107 changes: 0 additions & 107 deletions .github/workflows/daily-sanitizers.yml
View file
Open in desktop

This file was deleted.

Loading