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

More reliable system tests #1086

Merged
merged 2 commits into from
Jul 7, 2022
Merged

More reliable system tests #1086

merged 2 commits into from
Jul 7, 2022

Conversation

coro
Copy link
Contributor

@coro coro commented Jul 6, 2022

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

  • Dynamically check for PVC expansion support in tests
  • Be more explicit about the starting capacity of the cluster used in the PVC expansion test
  • Dynamically look up RMQ version to see if the clustering issue is present

Additional Context

Previously, it was the responsibility of whoever was running the system tests to decide if PVC expansion was supported or not.
In addition, the tests created a storageClass with the gce-pd Provider, in order to ensure there was a storageClass available that supported expansion. This Provider is deprecated, and in modern gke versions supports expansion anyway, removing the need for a bespoke class.

This change now dynamically queries the k8s API for the default storage class, and checks if it supports expansion or not. If it does not, the expansion test is skipped as before (e.g. in kind). If it does, the test proceeds, using this default class.
This also means the tests should now be able to be run in non-GKE environments with storage drivers that support PVC expansion.

This should hopefully alleviate some race conditions we see in our CI where a storageClass is not yet available before a PVC is provisioned using that Class.

In addition, this PR adds a dynamic lookup for the version of RabbitMQ being tested against in CI, to see if it is in the version range affected by #662, rather than just 3.8.8 (which is what we happen to test in our CI as the earliest supported version).

Local Testing

Please ensure you run the unit, integration and system tests before approving the PR.

To run the unit and integration tests:

$ make unit-tests integration-tests

You will need to target a k8s cluster and have the operator deployed for running the system tests.

For example, for a Kubernetes context named dev-bunny:

$ kubectx dev-bunny
$ make destroy deploy-dev
# wait for operator to be deployed
$ make system-tests

coro added 2 commits July 6, 2022 16:43
Previously, it was the responsibility of whoever was running the system tests to decide if PVC expansion was supported or not.
In addition, the tests created a storageClass with the gce-pd Provider, in order to ensure there was a storageClass available that supported expansion. This Provider is deprecated, and in modern gke versions supports expansion anyway, removing the need for a bespoke class.

This change now dynamically queries the k8s API for the default storage class, and checks if it supports expansion or not. If it does not, the expansion test is skipped as before (e.g. in kind). If it does, the test proceeds, using this default class.
This also means the tests should now be able to be run in non-GKE environments with storage drivers that support PVC expansion.
Copy link
Collaborator

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

Looking good 👌 Perhaps we could expect with offset in the helper functions? I'll leave it up to you 🙂

system_tests/utils.go Show resolved Hide resolved
@coro
Copy link
Contributor Author

coro commented Jul 7, 2022

Looking good 👌 Perhaps we could expect with offset in the helper functions? I'll leave it up to you 🙂

@Zerpet I won't in this case, as the helper functions are just returning information about the system for the test to use, and if the Expects fail, then that's a test environment failure rather than a test failure (e.g., I'd always expect there to be a default storageClass). I'd use ExpectWithOffset if the helper functions were something like expectExpansionToBeSupported(), in which case I'd want Gomega to point to the line calling that helper function as the failure, rather than any Expects in that function.

@coro coro merged commit 6a580b8 into main Jul 7, 2022
@coro coro deleted the more-reliable-system-tests branch July 7, 2022 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants