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

✨ (go/v4): add race flag for go test command at the makefile #4526

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PG2000
Copy link
Contributor

@PG2000 PG2000 commented Jan 25, 2025

In order to detect data races early it would make sense to use the race flag
for the most common generated go commands.

In grown projects built on kubebuilder i observed that with the timing and growing codebases
some data races occured in tests and then its hard to find the causing commit or code chnage.

Maybe this change can help to find them earlier.

In order to detect data races early it would make sense
to use the race flag for the most common generated go commands
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 25, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: PG2000
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 25, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @PG2000. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Jan 26, 2025

/ok-to-test

IMHO, this is a great thought!

However, please note that whenever we update the scaffold, we need to run make generate to regenerate all samples under testdata and in the docs/book.

Additionally, I hope you don’t mind, but I updated the title of this PR to include an emoji so it aligns with our standards and helps us create the Release Notes. Please refer to the CONTRIBUTING.md guide for more details.

Could you please:

Run make generate locally, commit the changes, and squash them to ensure this PR has only one commit? That way, we can proceed to merge it once the CI checks pass.

Thank you! 🙌

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 26, 2025
@camilamacedo86 camilamacedo86 changed the title chore: add race flag for go commands ✨ chore: add race flag for go test command at the makefile Jan 26, 2025
@camilamacedo86 camilamacedo86 changed the title ✨ chore: add race flag for go test command at the makefile ✨ (go/v4): add race flag for go test command at the makefile Jan 26, 2025
@@ -61,7 +61,7 @@ COPY internal/ internal/
# was called. For example, if we call make docker-build in a local env which has the Apple Silicon M1 SO
Copy link
Member

Choose a reason for hiding this comment

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

Also, see that the changes here broke the tests

      #15 [builder 9/9] RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -race -a -o manager cmd/main.go
      #15 0.430 go: -race requires cgo; enable cgo by setting CGO_ENABLED=1
      #15 ERROR: process "/bin/sh -c CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -race -a -o manager cmd/main.go" did not complete successfully: exit code: 2
      ------

It does not seems to work.
So, can you please check out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will check this. Seems like CGO must be enabled for go build. Will try to leave this part out and start on the other problem where a data race was detected.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jan 26, 2025

@PG2000: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubebuilder-e2e-k8s-1-32-0 51115f7 link true /test pull-kubebuilder-e2e-k8s-1-32-0
pull-kubebuilder-e2e-k8s-1-30-8 51115f7 link true /test pull-kubebuilder-e2e-k8s-1-30-8

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@@ -155,7 +155,7 @@ test-e2e: manifests generate fmt vet ## Run the e2e tests. Expected an isolated
echo "No Kind cluster is running. Please start a Kind cluster before running the e2e tests."; \
exit 1; \
}
go test ./test/e2e/ -v -ginkgo.v
go test ./test/e2e/ -race -v -ginkgo.v
Copy link
Contributor

@dmvolod dmvolod Jan 29, 2025

Choose a reason for hiding this comment

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

This slow-down all process for several times and required cgo to be enabled, and on non-Darwin systems requires an installed C compiler. Not sure, it should be enabled by default.

Probably it can be an option or flag and customer should decide if it enabled or not.
For example, we are using it for long running CI processes at night, but not in regular pipelines for each PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to add some extra flexibility, we can introduce a field that will be empty by default with the ability to change it, and which we will add as arguments when calling tests and/or build.
We can add two different fields for build and tests.
But it is very dangerous to forcefully add the -race flag by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants