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

[DPE-4173] Miscellaneous improvements to continue stabilizing exporter integration tests #254

Merged
merged 14 commits into from
Jun 28, 2024

Conversation

shayancanonical
Copy link
Contributor

@shayancanonical shayancanonical commented Jun 13, 2024

Issue

  1. We are using connection pools in our exporter integration tests. As these connection pools use connections with keep-alive, the guess is that the server has connections in TIME_WAIT for a period of time when it shuts down. Immediately starting the server (in consecutive tests) results in bind-address in use error
  2. We are running to 2 possibly flakey tests back to back (the first one unrelating and the second one shortly relating with COS right after)

Solution

  1. Use requests.get(stream=False) instead of using connection pools
  2. Split the 2 tests into separate files so that they are part of separate groups and run separately

Notes

Tests passed in 1 retry compared to usually higher retries

lib/charms/tempo_k8s/v1/charm_tracing.py Dismissed Show dismissed Hide dismissed
@shayancanonical shayancanonical changed the title [DPE-4173] Replace urllib3.PoolManager with single request to avoid open connections [DPE-4173] Miscallenous improvements to continue stabilizing exporter integration tests Jun 27, 2024
@shayancanonical shayancanonical changed the title [DPE-4173] Miscallenous improvements to continue stabilizing exporter integration tests [DPE-4173] Miscellaneous improvements to continue stabilizing exporter integration tests Jun 27, 2024
@shayancanonical shayancanonical marked this pull request as ready for review June 27, 2024 19:37
Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

Interesting approach!

@shayancanonical
Copy link
Contributor Author

It is by no means a call to surrender! Plan on continuing investigating the connection in TIME_WAIT with upstream: rluisr/mysqlrouter_exporter#67

src/rock.py Outdated Show resolved Hide resolved
tests/integration/test_exporter.py Outdated Show resolved Hide resolved
tests/integration/test_exporter.py Outdated Show resolved Hide resolved
tests/integration/test_exporter.py Outdated Show resolved Hide resolved
RETRY_TIMEOUT = 3 * 60

if juju_.is_3_or_higher:
TLS_APP_NAME = "self-signed-certificates"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you use lower_case since these technically aren't constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why are they not constants? i believe they are as their values are not changing

also, all such variables are constants in tests, so for now, for consistency, i would prefer to leave as is

Copy link
Contributor

Choose a reason for hiding this comment

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

the value changes based on if juju_.is_3_or_higher:, so by definition it's not constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think that this is open to interpretation. the value is assigned based on whether juju >=3, but does not change during the course of the variable's lifecycle. i dont believe, either way, that this would be a blocker for the PR - i am not adding the variable, just relocating it

Copy link
Collaborator

Choose a reason for hiding this comment

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

color of the buttons, as someone like to says. let's move forward

RETRY_TIMEOUT = 3 * 60

if juju_.is_3_or_higher:
TLS_APP_NAME = "self-signed-certificates"
Copy link
Collaborator

Choose a reason for hiding this comment

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

color of the buttons, as someone like to says. let's move forward

@shayancanonical shayancanonical merged commit ae928fc into main Jun 28, 2024
25 checks passed
@shayancanonical shayancanonical deleted the fix/exporter_integration_test branch June 28, 2024 20:04
shayancanonical added a commit to canonical/mysql-router-operator that referenced this pull request Jul 16, 2024
…ssues (#154)

Counterpart of
canonical/mysql-router-k8s-operator#254

## Summary
1. Use `requests.get(stream=False)` to avoid open connections on the
router exporter
2. Split the two exporter tests into different files so that they end up
in different integration test groups
3. Use a much larger timeout (7mins) that the TIME_WAIT timeout
(60s/1min) to see if the snap daemon (mysql-router-exporter) will
restart successfully
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.

4 participants