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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃尡 addr.Suggest should lock a file instead of memory #1563

Merged
merged 1 commit into from Jun 23, 2021

Conversation

vincepri
Copy link
Member

Envtest is often running in parallel when using go test, which spins up
multiple indipendent go test processes that cannot talk to each other.
The address suggestion code, mostly used to find an open port, can
cause port collisions and a race condition between different envtests
running at the same time.

This change switches the internal memory to use a file based system that
creates a file.

Signed-off-by: Vince Prignano vincepri@vmware.com

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 22, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 22, 2021
@vincepri vincepri force-pushed the addr-file-based branch 2 times, most recently from de49419 to a31ea38 Compare June 22, 2021 03:19
@coderanger
Copy link
Contributor

This would help specifically for parallel testing of one package but not multiple packages, right? Since those each get their own entrypoint binary so would be independent tempdirs?

@vincepri
Copy link
Member Author

Are you referring to darwin?

@alvaroaleman
Copy link
Member

Envtest is often running in parallel when using go test, which spins up
multiple indipendent go test processes

TILd

pkg/internal/testing/addr/manager.go Outdated Show resolved Hide resolved
pkg/internal/testing/addr/manager.go Outdated Show resolved Hide resolved
pkg/internal/testing/addr/manager.go Outdated Show resolved Hide resolved
@coderanger
Copy link
Contributor

Are you referring to darwin?

Looks like you took care of it with UserCacheDir, nice :)

@christopherhein
Copy link
Member

Does this stop one test process from stopping envtest on test completion causing flakes across multiple packages?

@vincepri
Copy link
Member Author

Does this stop one test process from stopping envtest on test completion causing flakes across multiple packages?

This stops a race condition that could happen in the event that the same port is allocated and closed quickly between two processes, where one end up not starting because either etcd or api-server is reusing a port.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 23, 2021
@vincepri vincepri force-pushed the addr-file-based branch 3 times, most recently from d1ff9ab to d182594 Compare June 23, 2021 17:59
Envtest is often running in parallel when using go test, which spins up
multiple indipendent go test processes that cannot talk to each other.
The address suggestion code, mostly used to find an open port, can
cause port collisions and a race condition between different envtests
running at the same time.

This change switches the internal memory to use a file based system that
creates a file.

Signed-off-by: Vince Prignano <vincepri@vmware.com>
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, vincepri

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [alvaroaleman,vincepri]

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 merged commit 985e819 into kubernetes-sigs:master Jun 23, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.9.x milestone Jun 23, 2021
@Porges
Copy link

Porges commented Jul 21, 2021

@vincepri We just updated to controller-runtime@v0.9.2 and it looks like this code is causing problems for us. We run envtest in parallel and are getting test timeouts with goroutines sitting in unix.Flock; I am wondering if the acquire call should be made with LOCK_NB|LOCK_EX to ensure it doesn鈥檛 block?

@vincepri
Copy link
Member Author

vincepri commented Jul 21, 2021

@Porges Thanks for the report, running some tests locally with the exclusive and non-blocking flags. Can you open an issue and give some more information on your environment, how many envtests are you running in parallel, and the information you found?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants