[health-report ci] Put back the full start requirement. #17994
Conversation
… scenario requirement.
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
|
This pull request does not have a backport label. Could you fix it @mashhurs? 🙏
|
| # Read stdout and stderr in real-time | ||
| for stdout_line in iter(process.stdout.readline, ""): | ||
| print(stdout_line.strip()) | ||
| # we don't wait for Logstash fully start as we also test slow pipeline start scenarios |
There was a problem hiding this comment.
I think this comment is hinting at the fact that the control flow here has implications on polling etc around expectations for LS lifecycle. Perhaps we can be more explicit around what these break statements do?
There was a problem hiding this comment.
Something like "we poll stdout until a desired message is encountered, this has implications for how long the logstash process is observed".
When i originally reviewed this (coming from not knowing much about this) it was not clear to be that this has important implications for how long logstash is allowed to run while tests are polling for state.
There was a problem hiding this comment.
Ah I just saw it.
Right! The current expectation is to validate health observer to catch when pipeline slow start occurs. These break statements are generalized and suit for current scenarios/expectations we want to validate with.
During the slow start scenario, script doesn't show the LS instance logs after knowing it is starting the LS.
As an improvement I can see what we can do is continuous (maybe in a separate thread) showing what is happening with LS instance.
|
There was a problem hiding this comment.
This is passing the previously failing test https://buildkite.com/elastic/logstash-health-report-tests-pipeline/builds/381#0198afd4-d1f8-4cff-876b-bc9e3118308e In favor of merge to get back to green CI, we can discuss #17994 (comment) separately.
💚 Build Succeeded
|
3 participants
Release notes
[rn:skip]
What does this PR do?
Places back the full start requirement flag
full_start_requiredwhich can be controlled by a driver. For the scenarios such as slow start this can be not required as it needs to confirm pipeline start taking longer. And for other backpressure test cases it is required to check full pipeline startWhy is it important/What is the impact to the user?
N/A
Checklist
[ ] I have commented my code, particularly in hard-to-understand areas[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)[ ] I have added tests that prove my fix is effective or that my feature worksAuthor's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs