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

bugfix: separate images in acceptance tests #170

Closed
wants to merge 1 commit into from

Conversation

breml
Copy link
Collaborator

@breml breml commented Nov 20, 2024

Use separate images for instance and image acceptance tests, since they run concurrently and potentially lead to race conditions otherwise.

Quote from https://pkg.go.dev/cmd/go/internal/test:

Note that -parallel only applies within a single test binary.
The 'go test' command may run tests for different packages
in parallel as well, according to the setting of the -p flag
(see 'go help build').

This PR is an attempt to make the acceptance tests in CI more stable and reliable.

Use separate images for instance and image acceptance tests, since
they run concurrently and potentially lead to race conditions otherwise.

Signed-off-by: Lucas Bremgartner <[email protected]>
@breml
Copy link
Collaborator Author

breml commented Nov 20, 2024

acctest.TestImage is referenced in some more packages, so the same race condition can happen (e.g. https://github.com/lxc/terraform-provider-incus/actions/runs/11936968102/job/33271847644#step:8:231). Either we run all the tests sequentially or we have to select a different image for all the sub packages.

@breml
Copy link
Collaborator Author

breml commented Nov 21, 2024

If we decide to go with the "non-parallel" option for the acceptance tests, I think our Makefile needs to be updated for the testacc target like this (change to -parallel 1, add p 1):

	TF_LOG=$(TF_LOG) TF_ACC=1 $(GO) test -p 1 -v -race $(TESTARGS) -timeout 60m ./internal/...
  • -p 1: instructs the Go tool chain to only run 1 program, such as build commands or test binaries, which can be run in parallel. Defaults to GOMAXPROCS.
  • -parallel 1 instructs the test program to run up to 1 test cases concurrently, which are marked as "concurrency save" with t.Parallel() (or resource.ParallelTest in the case of Terraform acceptance tests). Currently, there is no such test case present in this code base, so all the test cases (for each package) are currently run in sequence. So the change of this instruction is not strictly necessary, but if we go with the -p 1, I feel, that it is only consistent to also change this to 1.

Copy link
Member

@maveonair maveonair left a comment

Choose a reason for hiding this comment

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

LGTM

@maveonair
Copy link
Member

@breml

If we decide to go with the "non-parallel" option for the acceptance tests, I think our Makefile needs to be updated for the testacc target like this (change to -parallel 1, add p 1):

	TF_LOG=$(TF_LOG) TF_ACC=1 $(GO) test -p 1 -v -race $(TESTARGS) -timeout 60m ./internal/...
  • -p 1: instructs the Go tool chain to only run 1 program, such as build commands or test binaries, which can be run in parallel. Defaults to GOMAXPROCS.
  • -parallel 1 instructs the test program to run up to 1 test cases concurrently, which are marked as "concurrency save" with t.Parallel() (or resource.ParallelTest in the case of Terraform acceptance tests). Currently, there is no such test case present in this code base, so all the test cases (for each package) are currently run in sequence. So the change of this instruction is not strictly necessary, but if we go with the -p 1, I feel, that it is only consistent to also change this to 1.

The acceptance test take anyway a certain amount of time. Fo if we can stabilize the flakiness with that approach then I am happy to try it out :)!

@stgraber
Copy link
Member

I don't think I want this fix as it's not a fix so much as a workaround.

Incus should be able to handle many instances and/or images copies happening at the same time, if this leads to errors, then we need to make sure that our image download locking logic is corrected there.

@stgraber
Copy link
Member

What would be good is to get a reliable reproducer for this against Incus so we can then track it down on the Incus side and fix it.

@stgraber
Copy link
Member

I managed to reproduce something similar to this issue

@stgraber
Copy link
Member

lxc/incus#1408

@stgraber stgraber closed this Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants