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

connectivity: check disrupt client deployment if enabled. #2688

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

Conversation

tommyp1ckles
Copy link
Contributor

@tommyp1ckles tommyp1ckles commented Jul 15, 2024

The disrupt test client Pods establish connections to the disrupt server clients. Only after the connections are established does the readiness probe become healthy.

Currently, we do not check if the clients have become healthy before proceeding.

This can lead to confusing CI failures, even if you do an early sanity check test (which asserts no pod restarts), there may not be any restarts to report right after doing setup.

To improve this, this commit will now cause setup to wait for the deployment to become ready. Initial failures to connect will cause the setup step to fail fast rather than proceeding with subsequent upgrade tests.

This should also help differentiate disrupt connection issues that aren't related to any upgrade steps.

@tommyp1ckles tommyp1ckles marked this pull request as ready for review July 15, 2024 22:48
@tommyp1ckles tommyp1ckles requested a review from a team as a code owner July 15, 2024 22:48
@tommyp1ckles tommyp1ckles requested review from christarazi and asauber and removed request for christarazi July 15, 2024 22:48
The disrupt test client Pods establish connections to the disrupt server clients.
Only after the connections are established does the readiness probe
become healthy.

Currently, we do not check if the clients have become healthy before
proceeding.

This can lead to confusing CI failures, even if you do an early sanity
check, as there may not be any restarts to report right after doing
setup.

To improve this, this commit will now cause setup to wait for the
deployment to become ready. Initial failures to connect will cause
the setup step to fail.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@tommyp1ckles
Copy link
Contributor Author

/test

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

This change needs to be made in the github.com/cilium/cilium repo first and then vendored into this one. I assume you're going to open a corresponding change in cilium/cilium?

@tommyp1ckles
Copy link
Contributor Author

@tklauser Oh right, forgot about that change, I believe this was opened prior to that.

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