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

test: Block HubbleObserveFollow until ready #25090

Merged
merged 1 commit into from Apr 26, 2023

Conversation

pchaigno
Copy link
Member

One of our flaky tests has been failing because we can't find the expected flow event in the hubble observe output. Looking at the logs revealed that the expected flow event was emitted before our hubble observe command running in the background was ready.

This happened because we start hubble observe in the background and directly make the test request without waiting [1]. This pull request changes that to ensure HubbleObserveFollow (which starts hubble observe in the background) waits for Hubble to be ready before returning.

To that end, debug mode is enabled. In that mode, hubble observe will the following debug message once connected to the server:

time="2023-04-21T16:29:45Z" level=debug msg="Sending GetFlows request" request="follow:true whitelist:{event_type:{type:129}}"

We can wait until we see that message to exit from HubbleObserveFollow. After that we'll simply call WaitUntilMatchFilterLineTimeout as usual, but we'll be sure Hubble was ready when we made the test query.

This works because the debug message is sent over stderr, whereas WaitUntilMatchFilterLineTimeout reads from stdout. WaitUntilMatchTimeout, the helper function we use to block in HubbleObserveFollow, searches for a match in both stdout and stderr.

1 - In that particular flaky test, we were actually waiting 2s, but that wasn't always enough.
Fixes: #11232.
Fixes: #22056.

@pchaigno pchaigno added the release-note/ci This PR makes changes to the CI. label Apr 24, 2023
@pchaigno pchaigno force-pushed the test-fix-hubble-observe-use branch from f553126 to d34b99f Compare April 24, 2023 16:58
@pchaigno
Copy link
Member Author

/test-vagrant

@pchaigno pchaigno marked this pull request as ready for review April 25, 2023 09:03
@pchaigno pchaigno requested review from a team as code owners April 25, 2023 09:03
Copy link
Contributor

@viktor-kurchenko viktor-kurchenko left a comment

Choose a reason for hiding this comment

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

LGTM.

test/helpers/kubectl.go Outdated Show resolved Hide resolved
One of our flaky tests has been failing because we can't find the
expected flow event in the hubble observe output. Looking at the logs
revealed that the expected flow event was emitted before our 'hubble
observe' command running in the background was ready.

This happened because we start 'hubble observe' in the background and
directly make the test request without waiting [1]. This commit changes
that to ensure HubbleObserveFollow (which starts 'hubble observe' in the
background) waits for Hubble to be ready before returning.

To that end, debug mode is enabled. In that mode, 'hubble observe' will
the following debug message once connected to the server:

    time="2023-04-21T16:29:45Z" level=debug msg="Sending GetFlows request" request="follow:true whitelist:{event_type:{type:129}}"

We can wait until we see that message to exit from HubbleObserveFollow.
After that we'll simply call WaitUntilMatchFilterLineTimeout as usual,
but we'll be sure Hubble was ready when we made the test query.

This works because the debug message is sent over stderr, whereas
WaitUntilMatchFilterLineTimeout reads from stdout.
WaitUntilMatchTimeout, the helper function we use to block in
HubbleObserveFollow, searches for a match in both stdout and stderr.

1 - In that particular flaky test, we were actually waiting 2s, but
    that wasn't always enough.
Fixes: 782919d ("ci/helpers: Add Hubble CLI helpers")
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the test-fix-hubble-observe-use branch from d34b99f to 18ed9ed Compare April 26, 2023 11:45
@pchaigno
Copy link
Member Author

I fixed both nits without rebasing in the last force push. The end-to-end ginkgo tests were green before that and I don't think we need to rerun them given I only touched an error string and a comment. Merging once linters are passing.

@pchaigno pchaigno merged commit f3c8190 into cilium:main Apr 26, 2023
34 of 35 checks passed
@pchaigno pchaigno deleted the test-fix-hubble-observe-use branch April 26, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: K8sAgentPolicyTest Basic Test Traffic redirections to proxy Tests HTTP proxy visibility without policy
4 participants