Don't use telemetry in standalone mode with ZEEKCTL_DISABLE_LISTEN, plus a bunch of cleanups
#79
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Aka, how I spent yesterday afternoon. :-)
ckreibich
force-pushed
the
topic/christian/fix-unreliable-tests
branch
2 times, most recently
from
b5696b5 to
203a786
Compare
ckreibich
changed the title
ZEEKCTL_DISABLE_LISTEN, plus a bunch of cleanups
ckreibich
added a commit
to zeek/zeek
that referenced
this pull request
Feb 21, 2025
This adds zeek/zeekctl#79 for CI exposure.
ckreibich
added a commit
to zeek/zeek
that referenced
this pull request
Feb 21, 2025
This adds zeek/zeekctl#79 for CI exposure.
…ISTEN This could cause transient port availability problems, and thus test failures, since while Broker didn't end up listening in those scenarios, Zeek opened up the Prometheus port regardless. I was tempted to guard the whole thing via the env var, but in case something verifies ports it seems safer to focus the guard on the telemetry port. Resolves #72
When we added this I changed the docstring for this in the generated sources since I had no idea zeekctl has automated docs extraction. Gnnn.
ckreibich
force-pushed
the
topic/christian/fix-unreliable-tests
branch
from
203a786 to
a2e6697
Compare
ckreibich
added a commit
to zeek/zeek
that referenced
this pull request
Feb 21, 2025
This adds zeek/zeekctl#79 for CI exposure.
awelzel
approved these changes
Feb 24, 2025
| @@ -178,7 +178,10 @@ def use_port(self, node): | |||
| # This is the port that standalone nodes listen on for remote | |||
| # control by default. | |||
| ostr += f"redef Broker::default_port = {zeekport.use_port(manager)}/tcp;\n" | |||
| ostr += '@if ( getenv("ZEEKCTL_DISABLE_LISTEN") == "" )\n' | |||
There was a problem hiding this comment.
We have the following over in telemetry/Manager.cc
But, I think this is better here as you have it., maybe redef to 0/tcp to explicitly disable in the else path? No strong opinion.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've not been able to definitively show how the port collision in #72 arises, but since this is not one of the tests that btest serializes (no
@TEST-SERIALIZE: listen), I'm pretty sure that's the culprit. My fix here is not to just serialize the test, but to make the use of telemetry in standalone mode depend on our use ofZEEKCTL_DISABLE_LISTEN, which was previously missing. I suspect this also fixes #75, but we'll have to see.I'm also adding an explanation of how these tests work to the README, and I'm dusting off some stuff that lets us regenerate the docs.
The way these tests separate their install tree from the test folders makes them pretty suboptimal for use in CI. For now I don't see a need to invest lots of time here, but the cluster testsuite would be a better place for all of this. I'm adding a btest environment
debugso they're not auto-cleaned.