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.

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

ci: add conformance-ginkgo-race #27979

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

learnitall
Copy link
Contributor

@learnitall learnitall commented Sep 6, 2023

This commit introduces a new workflow for running the ginkgo test suite with race detection enabled. The goal is to start this effort by doing a copy and paste of the existing ginkgo workflow and modifying it as needed. In the future, effort will be put towards reducing this duplication.

Thank you to @markpash for getting this started.

Fixes: #26170

Supersedes #27797.

Here is the diff between conformance-ginkgo-race and conformance-ginkgo:

1c1
< name: Conformance Ginkgo (ci-ginkgo)
---
> name: Conformance Ginkgo Race Detection (ci-ginkgo-race)
22a23
>   pull_request: {}
171c172
<             until docker manifest inspect quay.io/${{ env.QUAY_ORGANIZATION_DEV }}/$image:${{ needs.setup-vars.outputs.SHA }} &> /dev/null; do sleep 45s; done
---
>             until docker manifest inspect quay.io/${{ env.QUAY_ORGANIZATION_DEV }}/$image:${{ needs.setup-vars.outputs.SHA }}-race &> /dev/null; do sleep 45s; done
385a387,389
>             # Race detection variables
>             export RACE=1
>             export LOCKDEBUG=1
393c397
<              -cilium.tag=${{ needs.setup-vars.outputs.SHA }}  \
---
>              -cilium.tag=${{ needs.setup-vars.outputs.SHA }}-race  \
395c399
<              -cilium.operator-tag=${{ needs.setup-vars.outputs.SHA }} \
---
>              -cilium.operator-tag=${{ needs.setup-vars.outputs.SHA }}-race \
397c401
<              -cilium.hubble-relay-tag=${{ needs.setup-vars.outputs.SHA }} \
---
>              -cilium.hubble-relay-tag=${{ needs.setup-vars.outputs.SHA }}-race \
409c413
<                -cilium.tag=${{ needs.setup-vars.outputs.SHA }}  \
---
>                -cilium.tag=${{ needs.setup-vars.outputs.SHA }}-race  \
411c415
<                -cilium.operator-tag=${{ needs.setup-vars.outputs.SHA }} \
---
>                -cilium.operator-tag=${{ needs.setup-vars.outputs.SHA }}-race \
413c417
<                -cilium.hubble-relay-tag=${{ needs.setup-vars.outputs.SHA }} \
---
>                -cilium.hubble-relay-tag=${{ needs.setup-vars.outputs.SHA }}-race \
Add CI workflow for running ginkgo with race detection

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 6, 2023
@learnitall learnitall force-pushed the pr/learnitall/ginkgo-race-workflow branch from cc72017 to 28ce2f8 Compare September 6, 2023 20:37
@learnitall learnitall added the dont-merge/preview-only Only for preview or testing, don't merge it. label Sep 6, 2023
@learnitall learnitall force-pushed the pr/learnitall/ginkgo-race-workflow branch from 28ce2f8 to 979dd0c Compare September 7, 2023 15:51
@learnitall learnitall added the release-note/ci This PR makes changes to the CI. label Sep 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 7, 2023
@learnitall learnitall force-pushed the pr/learnitall/ginkgo-race-workflow branch from 979dd0c to 87a6ac2 Compare September 7, 2023 15:51
@nbusseneau nbusseneau self-requested a review September 7, 2023 20:36
@nbusseneau
Copy link
Member

Copy/pasting comment from previous PR:

There is a special trick you need to do to get your new workflow to register when you open the PR: add a temporary pull_request: {} trigger to the workflow, just to register the workflow on the repository when the PR opens. After it has been registered, you will be able to run the workflow with the Ariane trigger no problem, and can remove the pull_request: {} trigger.

As for structure: for testing let's just do it copy/pasted for now, when we are happy with how it works I would suggest we look into merging the changes in the main Ginkgo workflow file, so that only one file needs to be maintained. One idea I have is to leverage the extra-args parameters that Ariane allows, so we could trigger race pipelines with a trigger like this: /ci-ginkgo race.

Let me know if you need help with anything :)

@learnitall learnitall force-pushed the pr/learnitall/ginkgo-race-workflow branch from 87a6ac2 to 4b427f5 Compare September 8, 2023 16:29
@learnitall learnitall marked this pull request as ready for review September 8, 2023 16:30
@learnitall learnitall requested review from a team as code owners September 8, 2023 16:30
@learnitall learnitall force-pushed the pr/learnitall/ginkgo-race-workflow branch from 4b427f5 to 9e4d120 Compare September 11, 2023 20:01
@nbusseneau nbusseneau marked this pull request as draft September 12, 2023 15:04
@nbusseneau
Copy link
Member

Keeping this in draft until we have passing workflows :)

@learnitall learnitall force-pushed the pr/learnitall/ginkgo-race-workflow branch 3 times, most recently from bec8701 to 60a7e0b Compare September 13, 2023 14:55
@learnitall
Copy link
Contributor Author

/ci-ginkgo-race

aanm pushed a commit that referenced this pull request Sep 29, 2023
[ upstream commit fb6bd90 ]

PR #25309 introduced a data race by sharing the sessionUDPFactory between the
DNS server instances for the different IP families (IPv4 & IPv6). This has been detected
by #27979.

This commit fixes the data race by using dedicated instances of the sessionUDPFactory.

Fixes: #28156

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
aanm pushed a commit that referenced this pull request Sep 29, 2023
[ upstream commit f73e1c5 ]

[ backporter's notes: switched cilium/dns import to miekg/dns, as v1.14
  was relying on a replace directive instead of pointing to the fork. ]

PR #25309 introduced a data race by sharing the sessionUDPFactory between
the DNS server instances for the different IP families (IPv4 & IPv6).
This has been detected by #27979.

This commit fixes the issue for the TCP servers too. It not set explicitly,
these are initialized with the default udp session factory to prevent nil
pointer exceptions. Therefore, an explicit noop udp session factory is set.

Fixes: #28156

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@learnitall
Copy link
Contributor Author

/ci-ginkgo-race

@learnitall learnitall force-pushed the pr/learnitall/ginkgo-race-workflow branch from 1506f83 to 6ebc9c9 Compare October 2, 2023 16:05
@learnitall
Copy link
Contributor Author

/ci-ginkgo-race

@aanm aanm force-pushed the pr/learnitall/ginkgo-race-workflow branch 3 times, most recently from bacd82b to 34a083b Compare October 5, 2023 09:31
@aanm
Copy link
Member

aanm commented Oct 5, 2023

/ci-ginkgo-race

@aanm aanm force-pushed the pr/learnitall/ginkgo-race-workflow branch from 34a083b to 4a490e7 Compare October 6, 2023 08:20
@aanm
Copy link
Member

aanm commented Oct 6, 2023

/ci-ginkgo-race

gandro pushed a commit to gandro/cilium that referenced this pull request Oct 10, 2023
[ upstream commit fb6bd90 ]

PR cilium#25309 introduced a data race by sharing the sessionUDPFactory between the
DNS server instances for the different IP families (IPv4 & IPv6). This has been detected
by cilium#27979.

This commit fixes the data race by using dedicated instances of the sessionUDPFactory.

Fixes: cilium#28156

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
gandro pushed a commit to gandro/cilium that referenced this pull request Oct 10, 2023
[ upstream commit f73e1c5 ]

[ backporter's notes: switched cilium/dns import to miekg/dns, as v1.14
  was relying on a replace directive instead of pointing to the fork. ]

PR cilium#25309 introduced a data race by sharing the sessionUDPFactory between
the DNS server instances for the different IP families (IPv4 & IPv6).
This has been detected by cilium#27979.

This commit fixes the issue for the TCP servers too. It not set explicitly,
these are initialized with the default udp session factory to prevent nil
pointer exceptions. Therefore, an explicit noop udp session factory is set.

Fixes: cilium#28156

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@aanm aanm force-pushed the pr/learnitall/ginkgo-race-workflow branch from 4a490e7 to 8603059 Compare October 12, 2023 13:29
@aanm
Copy link
Member

aanm commented Oct 12, 2023

/ci-ginkgo-race

1 similar comment
@aanm
Copy link
Member

aanm commented Nov 9, 2023

/ci-ginkgo-race

@aanm aanm force-pushed the pr/learnitall/ginkgo-race-workflow branch 3 times, most recently from ae1e57d to a62b295 Compare November 9, 2023 15:51
@learnitall learnitall force-pushed the pr/learnitall/ginkgo-race-workflow branch from a62b295 to d147936 Compare December 8, 2023 23:23
This commit introduces a new workflow for running the ginkgo test suite
with race detection enabled. The goal is to start this effort by doing a
copy and paste of the existing ginkgo workflow and modifying it as
needed. In the future, effort will be put towards reducing this
duplication.

Thank you to @markpash for getting this started.

Fixes: #26170

Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
@learnitall learnitall force-pushed the pr/learnitall/ginkgo-race-workflow branch from d147936 to d613507 Compare December 8, 2023 23:29
Copy link

github-actions bot commented Jan 8, 2024

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 8, 2024
@learnitall learnitall added pinned These issues are not marked stale by our issue bot. and removed stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. labels Jan 8, 2024
@learnitall learnitall self-assigned this Feb 21, 2024
@learnitall
Copy link
Contributor Author

/ci-ginkgo-race

@learnitall learnitall added the area/CI-improvement Topic or proposal to improve the Continuous Integration workflow label Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow dont-merge/preview-only Only for preview or testing, don't merge it. pinned These issues are not marked stale by our issue bot. release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add race-detector tests back to the CI pipeline
4 participants