when we moved to github actions we deleted memory sanitise tests

[donbowman]

Bug Report

In .github/workflows/unit-tests.yaml we do not have -DFLB_SANITIZE_MEMORY=On nor -DFLB_SANITIZE_THREAD=On

We now have code which is committed that fails and causes SIGSEGV that these tests correctly identify as failed. (see e.g. #3585, #3588)

In our CI, which does have these enabled, master no longer passes, showing things like:

58/64 Test #49: flb-it-input_chunk ...............***Failed   14.39 sec
Test input_chunk_exceed_limit...                free(): double free detected in tcache 2
  Test interrupted by SIGABRT.

In these 3 commits:

7781234f450dce7cbe851ca6d6907f31e0a26c8b ci: switch travis UT to 'xenial', add matrix of clang/gcc for sanitizers (#885)
a6a985aab0f662f1ec8365341850b56e9e583b51 cmake: add sanitizers-cmake
69eee3fe11b46fa5ebfbe9a8c9278588cd3ec12b build: add address/thread/memory sanitizers as option (#792)

I had added support. This was removed for github workflow, please reinstate.

https://github.com/fluent/fluent-bit/pulls?q=is%3Apr+is%3Aclosed

shows that we still merge pull requests even when the existing tests fail. IMHO this is bad practice, bad governance.

We also have direct commits to master, w/ no PR, that fail the CI
e.g.
03f3339

suggest that:

a: we block master for PUSH for 100% of users. All code should go through PR, and pass UT
b: we reinstate the memory tests, we currently have crashes in released code that the tests are finding
c: if it doesn't pass the test, it doesn't merge.

[donbowman]

Note: the current memory corruption is in one of the commits on June 4
03f3339
687b08f
3dd43ea
744cf85
e152556
14f00ca

its hard to bisect those tho since some don't compile (e.g. 744cf85)

[nokute78]

The points are

1. Reinstate -DFLB_SANITIZE_MEMORY=On and -DFLB_SANITIZE_THREAD=On
2. We block master for PUSH for 100% of users. All code should go through PR, and pass UT

I agree these points.

@niedbalski Could you check the sanitizing configuration?

@edsiper How about blocking direct push to master ?
I believe it is better for this project to go to next step.

[donbowman]

i guess no comments

[nokute78]

a: we block master for PUSH for 100% of users. All code should go through PR, and pass UT

From my point of view, recent changes are gone through PR and passed CI.
e.g. #3725 #3724 #3721

[donbowman]

most of the recent commits were directly to master w/o PR.
some don't have passing tests
https://github.com/fluent/fluent-bit/commits/master
shows.

5 of the most recent 25 PR don't have passing tests, but those that are passing, the memory tests were commented out in the workflow, so its not really running all of them (which was the original intent of this issue).

so i guess, is there a govenance model for this project? https://github.com/fluent/fluent-bit/blob/master/GOVERNANCE.md doesn't have anthing to say on best practices like:

• lock and don't commit to master [including maintainers]
• all code goes through PR process, unit tests pass as a gate before accepting merge

i did the original work to get the unit tests online, and then did that again after they were disregarded and broke, so i'm loathe to step in again if its just not important to the maintainers. My last 3 rebase from the upstream of this project have all resulted in broken code (in 1 case it did not even compile).

[edsiper]

most of the recent commits were directly to master w/o PR.
some don't have passing tests
https://github.com/fluent/fluent-bit/commits/master
shows.

actually, all of them have a PR that was rebased and merged after CI passed (except one config file that added a comment).

https://github.com/fluent/fluent-bit/pulls?q=is%3Apr+is%3Aclosed+

[edsiper]

I agree with the points to be added to GOVERNANCE and be more strict about that.

[edsiper]

@niedbalski

can you take a look at the missing sanitizers mentioned above ?

[niedbalski]

The points are

1. Reinstate -DFLB_SANITIZE_MEMORY=On and -DFLB_SANITIZE_THREAD=On
2. We block master for PUSH for 100% of users. All code should go through PR, and pass UT

I agree these points.

@niedbalski Could you check the sanitizing configuration?

@edsiper How about blocking direct push to master ?
I believe it is better for this project to go to next step.

1. I will fix this
2. We can add a branch rule that disallows push for master (including repository admins) without review/pr.

Let me know your thoughts on this.

[edsiper]

Yeah, let's move forward with that

…

On Wed, Jul 7, 2021, 10:09 Jorge Niedbalski ***@***.***> wrote: The points are 1. Reinstate -DFLB_SANITIZE_MEMORY=On and -DFLB_SANITIZE_THREAD=On 2. We block master for PUSH for 100% of users. All code should go through PR, and pass UT I agree these points. @niedbalski <https://github.com/niedbalski> Could you check the sanitizing configuration? @edsiper <https://github.com/edsiper> How about blocking direct push to master ? I believe it is better for this project to go to next step. 1. I will fix this 2. We can add a branch rule that disallows push for master (including repository admins) without review/pr. Let me know your thoughts on this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3609 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC2INXTVJ2YYBXGGNO7FQLTWR32JANCNFSM46KBVYGQ> .

[niedbalski]

@nokute78 @edsiper I made the following changes:

1. Re-enabled both flags (-DFLB_SANITIZE_MEMORY=On and -DFLB_SANITIZE_THREAD=On) on the unit tests.
2. Enforced branch protection rules for admins / enabled checks required before merging (github: enforce repository rules to admins. fluent-bit-infra#9)

That should fix this requirement

[nokute78]

@niedbalski Thank you for updating.

By the way, is "Require branches to be up to date before merging" enabled?
I think it is strict but it is hard to merge pull request for both contributor and maintainer.

In my understanding, if one pull request is merged, any other pull requests can't be merged since they are out-of-date branch.
Other pull request owners have to merge the latest change over and over until it will be merged.