Skip to content

tests: Remove AZ add/remove host integration test #3120

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

tests: Remove AZ add/remove host integration test #3120

wants to merge 1 commit into from

Conversation

stephenfin
Copy link
Contributor

Go runs tests in parallel for different packages in parallel. The AZ add/remove host test was adding and removing hosts from an AZ and if this ran at the same time as another test that created a server then the host addition or removal would fail since Nova forbids it. There does not appear to be any way to resolve this. We could run all integration tests serially instead, but this would be a backwards step that will massively increase test run time. We could also rewrite the test to only add/remove an unused host, but that would require us to fence off this compute node in all other tests using non-default parameters to server create. Given the lack of better options, we simply remove the test and assume our testing to date on this method has been good enough and unit tests will be enough to carry us forward.

Signed-off-by: Stephen Finucane stephenfin@redhat.com

@stephenfin stephenfin mentioned this pull request Jul 17, 2024
Merged
@mandre
Copy link
Contributor

mandre commented Jul 17, 2024

I'm not at all convinced these issues are caused by parallelization. As far as I understand, we do not run the tests in parallel -- we don't use t.Parallel() anywhere. Have you seen this test cause issues?

@stephenfin
Copy link
Contributor Author

stephenfin commented Jul 17, 2024 •
edited
Loading

I'm not at all convinced these issues are caused by parallelization. As far as I understand, we do not run the tests in parallel -- we don't use t.Parallel() anywhere. Have you seen this test cause issues?

Yes, I saw it fail here. Reproducing the failure here:

    aggregates_test.go:81: Failure in aggregates_test.go, line 81: unexpected error "Expected HTTP response code [200] when accessing [POST http://10.1.0.62/compute/v2.1/os-aggregates/3/action], but got 409 instead: {\"conflictingRequest\": {\"code\": 409, \"message\": \"Cannot add host to aggregate 3. Reason: The host cannot be added to the aggregate as the availability zone of the host would change from 'None' to 'zone_jgzKH' but the host already has 1 instance(s). Changing the AZ of an existing instance is not supported by this action. Move the instances away from this host then try again. If you need to move the instances between AZs then you can use shelve_offload and unshelve to achieve this..\"}}"

So, as I'm reading that, a test in another file (meaning t.Parallel() is irrelevant, since that only lets you do parallelization in a given file: Go will run groups of tests from separates files in parallel by default) had created a server. Before that test was completed (i.e. while the server from that other test was still running), we started running this test and attempted to add the host to a new aggregate. That failed because Nova no longer lets you move a host between AZs when that host has a server https://review.opendev.org/c/openstack/nova/+/821423. This wouldn't happen if we ran all our tests serially but we don't.

@github-actions github-actions bot added the semver:patch No API change label Jul 17, 2024
@coveralls
Copy link

coveralls commented Jul 17, 2024 •
edited
Loading

Coverage Status

coverage: 63.718%. first build
when pulling 320542c on shiftstack:remove-az-modification-tests
into bbf1905 on gophercloud:main.

@stephenfin stephenfin force-pushed the remove-az-modification-tests branch from 73f7c95 to 25e93c1 Compare May 23, 2025 15:53
@stephenfin stephenfin requested a review from mandre May 23, 2025 15:54
@github-actions github-actions bot added the edit:compute This PR updates compute code label May 23, 2025
@stephenfin
tests: Remove AZ add/remove host integration test
320542c 
Go runs tests in parallel for different packages in parallel. The AZ
add/remove host test was adding and removing hosts from an AZ and if
this ran at the same time as another test that created a server
then the host addition or removal would fail since Nova forbids it.
There does not appear to be any way to resolve this. We could run all
integration tests serially instead, but this would be a backwards step
that will massively increase test run time. We could also rewrite the
test to only add/remove an unused host, but that would require us to
fence off this compute node in all other tests using non-default
parameters to server create. Given the lack of better options, we simply
remove the test and assume our testing to date on this method has been
good enough and unit tests will be enough to carry us forward.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
@stephenfin stephenfin force-pushed the remove-az-modification-tests branch from 25e93c1 to 320542c Compare July 21, 2025 15:42
@github-actions github-actions bot added the backport-v2 This PR will be backported to v2 label Jul 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mandre mandre Awaiting requested review from mandre

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
backport-v2 This PR will be backported to v2 edit:compute This PR updates compute code semver:patch No API change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stephenfin @mandre @coveralls