MOD-7543 purge readies from sanitizer flow by DvirDukhan · Pull Request #4726 · RediSearch/RediSearch

DvirDukhan · 2024-06-09T19:08:11Z

Describe the changes in the pull request
This PR purges Readies from the sanitizer flow, allowing for an easier, less intrusive workflow by using the standard docker and dependency toolchain.
This PR unifies the flows for all test flows (test, coverage, sanitizer) to use task-test.yml.
This PR moves the llvm and rust installations to our common .install folder, where they should be, and removes them from build-rejson script

As a temporary workaround, this PR also disables sanitizer with JSON tests, until a fix is submitted to RedisJSON repo

Also, we link statically to libstdc++ when we run sanitizer, since otherwise we crash in geometry tests otherwise. This is fine as long as we test sanitizer against redis 7.4. When we move to test against redis 8, we should link libstdc++ dynamically (as we do from now on).

Which issues this PR fixes
MOD-7453

Main objects this PR modified
task-test.yml
build-rejson

This PR introduces API changes
This PR introduces serialization changes

codecov · 2024-08-08T12:34:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.50%. Comparing base (ab3628d) to head (535acda).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4726      +/-   ##
==========================================
- Coverage   86.50%   86.50%   -0.01%     
==========================================
  Files         195      195              
  Lines       34818    34818              
==========================================
- Hits        30121    30120       -1     
- Misses       4697     4698       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

GuyAv46 · 2024-08-08T19:54:34Z

        type: string
        required: true
+      get-redis:
+        type: string


This new parameter is not passed to the next workflow

removed flow

GuyAv46 · 2024-08-08T20:04:50Z

+if [[ $OS_TYPE == Darwin ]]; then
+    brew install llvm@$VERSION
+else
+    $MODE apt install lsb-release wget software-properties-common gnupg


If we use apt is it working on all Linux based OSs?

yes, let's start here, with the common OSs to devs, and see if we need to add support to other OSs as well

GuyAv46 · 2024-08-08T20:15:38Z

IMO CI infrastructure should be aligned and up-to-date across all maintained branches (master and versions). This makes maintaining the CI much simpler as we practically have a single infra, and if something breaks we only need a single fix (with CPs)

raz-mon · 2024-08-11T13:34:59Z

-      san: addr
+      san: address
+      env: ubuntu-latest
+      rejson: 0


Still needed? json sanitizer hiccup solved

GuyAv46 · 2024-12-17T08:35:04Z

+        run: |
+          ${{ steps.mode.outputs.mode }} SANITIZER=${{ inputs.san }} BUILD_TLS=yes make install


Suggested change

run: |

${{ steps.mode.outputs.mode }} SANITIZER=${{ inputs.san }} BUILD_TLS=yes make install

env:

BUILD_TLS: yes

SANITIZER: ${{ input.san }}

run: ${{ steps.mode.outputs.mode }} make install

redisearch-backport-pull-request · 2024-12-17T09:50:11Z

Backport failed for 2.8, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.8
git worktree add -d .worktree/backport-4726-to-2.8 origin/2.8
cd .worktree/backport-4726-to-2.8
git switch --create backport-4726-to-2.8
git cherry-pick -x 29d0b784048d5f8c00a6b7792664a8d5c87ba840

redisearch-backport-pull-request · 2024-12-17T09:50:12Z

Backport failed for 2.6, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.6
git worktree add -d .worktree/backport-4726-to-2.6 origin/2.6
cd .worktree/backport-4726-to-2.6
git switch --create backport-4726-to-2.6
git cherry-pick -x 29d0b784048d5f8c00a6b7792664a8d5c87ba840

redisearch-backport-pull-request · 2024-12-17T09:50:13Z

Backport failed for 2.10, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.10
git worktree add -d .worktree/backport-4726-to-2.10 origin/2.10
cd .worktree/backport-4726-to-2.10
git switch --create backport-4726-to-2.10
git cherry-pick -x 29d0b784048d5f8c00a6b7792664a8d5c87ba840

redisearch-backport-pull-request · 2024-12-17T09:50:14Z

Backport failed for 8.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 8.0
git worktree add -d .worktree/backport-4726-to-8.0 origin/8.0
cd .worktree/backport-4726-to-8.0
git switch --create backport-4726-to-8.0
git cherry-pick -x 29d0b784048d5f8c00a6b7792664a8d5c87ba840

* wip * wip * wip * wip * delete * added get-redis input to sanitzier * removed no draft for sanitizer * added mode for llvm install * restored build redisjson * wip * after rebase * fixed typo * llvm_version * restored install boost * fixed? * set clang and llvm version * take llvm version from env * fixed build redisjson * don't run json on sanitizer * fixed task-test * removed flow-sanitizer * moved rust and llvm to test_deps. fixed llvm params order * fixed PR comments * remove unused * upload logs if canceled * tmp add test * also on coord * try explicitly set flags * MOD-7596: Add mirroring support (#4937) * * initial commit * * Use RediSearch branch mirroring fork * * Code Review - Round #1 * MOD-7695: Decide Ubuntu Runner Using Variable (#4975) * * initial commit * * code review * added LD flag * trying the whole tests * delete event mirror * try a clean container * add -y * add curl to jammy * single tests for logs * add SAN to cmake defs * shared linker flags * fixed a typo * build redis with clang * Add cflags to redis for sanitizer * remove = * skip json in new json test, split phases in build redis (install after) * Switch position Build_tls and sanitizer * typo in sanitizer * run full test suite * Set sanitizer to run against latest redis with static libstdc++ * Run tests with json as well * removed jammy from PR event * bring back jammy * added jammy to merge queue and nightly * added test_config to merge queue * install LLVM only on sanitizer * added working directory * curl * remove curl from amazon linux 2023 * bring back install llvm * put rust in bashrc * add rest to path - another attempt * move rust to common installations --------- Co-authored-by: kei-nan <jonathan.keinan@redis.com> Co-authored-by: alon-reshef <alon.reshef@redis.com> Co-authored-by: alonre24 <alonreshef24@gmail.com>

DvirDukhan force-pushed the dvirdu_santizer branch from 83344a3 to 4eed00c Compare July 22, 2024 07:06

DvirDukhan force-pushed the dvirdu_santizer branch from 6f1e992 to 5176130 Compare August 8, 2024 08:55

DvirDukhan marked this pull request as ready for review August 8, 2024 08:58

DvirDukhan changed the title ~~Dvirdu santizer~~ MOD-7543 purge readies from sanitizer flow Aug 8, 2024

DvirDukhan requested review from GuyAv46, alonre24 and oshadmi August 8, 2024 19:05

GuyAv46 reviewed Aug 8, 2024

View reviewed changes

alonre24 reviewed Aug 8, 2024

View reviewed changes

Comment thread .github/workflows/event-pull_request.yml Outdated

Comment thread .github/workflows/task-test.yml Outdated

Comment thread .github/workflows/task-test.yml

Comment thread tests/pytests/runtests.sh Outdated

raz-mon reviewed Aug 11, 2024

View reviewed changes

raz-mon mentioned this pull request Sep 3, 2024

MOD-7465 MOD-7466: Add ACL support for RediSearch, phase 1 #4957

Merged

2 tasks

kei-nan force-pushed the master branch from 6adbeea to c1e17d9 Compare September 8, 2024 11:16

DvirDukhan added 16 commits September 16, 2024 19:09

wip

71ac6c2

wip

c5ce45e

wip

4f03983

wip

a749299

delete

901bb0e

added get-redis input to sanitzier

48e9e37

removed no draft for sanitizer

9c8725e

added mode for llvm install

224c12d

restored build redisjson

f823997

wip

8579ce3

after rebase

28788b1

fixed typo

44a78c1

llvm_version

278e865

restored install boost

283fdc1

fixed?

254cfff

set clang and llvm version

c8898ab

alonre24 previously approved these changes Nov 17, 2024

View reviewed changes

DvirDukhan added this pull request to the merge queue Nov 17, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 17, 2024

remove curl from amazon linux 2023

c1bbaf4

DvirDukhan dismissed alonre24’s stale review via c1bbaf4 November 17, 2024 19:43

DvirDukhan enabled auto-merge November 18, 2024 07:39

alonre24 added 5 commits December 16, 2024 12:20

rebase with master + add rust to path

1ebd07b

bring back install llvm

3a9a801

put rust in bashrc

bdb85b3

add rest to path - another attempt

081208a

move rust to common installations

535acda

alonre24 approved these changes Dec 17, 2024

View reviewed changes

alonre24 removed the request for review from oshadmi December 17, 2024 08:01

alonre24 disabled auto-merge December 17, 2024 08:02

DvirDukhan enabled auto-merge December 17, 2024 08:03

alonre24 requested a review from GuyAv46 December 17, 2024 08:14

GuyAv46 approved these changes Dec 17, 2024

View reviewed changes

DvirDukhan added this pull request to the merge queue Dec 17, 2024

Merged via the queue into master with commit 29d0b78 Dec 17, 2024

DvirDukhan deleted the dvirdu_santizer branch December 17, 2024 09:49

This was referenced Dec 17, 2024

[MOD-7543] [8.0] purge readies from sanitizer flow #5359

Merged

[MOD-7543] purge readies from sanitizer flow [2.10] #5360

Merged

[MOD-7543] purge readies from sanitizer flow [2.8] #5385

Merged

[2.6] Purge readies from sanitizer flow [MOD-7543] #5395

Merged

		run: \|
		${{ steps.mode.outputs.mode }} SANITIZER=${{ inputs.san }} BUILD_TLS=yes make install

-        run: |
-          ${{ steps.mode.outputs.mode }} SANITIZER=${{ inputs.san }} BUILD_TLS=yes make install
+        env:
+          BUILD_TLS: yes
+          SANITIZER: ${{ input.san }}
+        run: ${{ steps.mode.outputs.mode }} make install

Conversation

DvirDukhan commented Jun 9, 2024 • edited by alonre24 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

GuyAv46 Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

DvirDukhan Aug 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuyAv46 Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

DvirDukhan Aug 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GuyAv46 commented Aug 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raz-mon Aug 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GuyAv46 Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

redisearch-backport-pull-request Bot commented Dec 17, 2024

Uh oh!

redisearch-backport-pull-request Bot commented Dec 17, 2024

Uh oh!

redisearch-backport-pull-request Bot commented Dec 17, 2024

Uh oh!

redisearch-backport-pull-request Bot commented Dec 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DvirDukhan commented Jun 9, 2024 •

edited by alonre24

Loading

codecov Bot commented Aug 8, 2024 •

edited

Loading