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.

Sign up for GitHub

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

Jump to bottom

Backport of PR #11541 to 7.7 #11641

Merged
merged 2 commits into from
Mar 13, 2020
Merged

Backport of PR #11541 to 7.7 #11641

merged 2 commits into from
Mar 13, 2020

Conversation

andsel
Copy link
Contributor

@andsel andsel commented Feb 28, 2020

Backport of PR #11541 to 7.7

Adaptations to internal collector to send data directly to monitoring cluster Close 11573

Fixes #11541

@elasticsearch-bot elasticsearch-bot self-assigned this Feb 28, 2020
@robbavey robbavey self-requested a review February 28, 2020 16:39
@robbavey
Copy link
Member

Jenkins test this please

2 similar comments
@andsel
Copy link
Contributor Author

andsel commented Mar 2, 2020

Jenkins test this please

@andsel
Copy link
Contributor Author

andsel commented Mar 2, 2020

Jenkins test this please

@elasticsearch-bot elasticsearch-bot self-assigned this Mar 4, 2020
@andsel
Copy link
Contributor Author

andsel commented Mar 4, 2020

@robbavey I've added a commit to this backport that fixed the flaky test. It was due to time format, and this explained the intermittent errors

@robbavey
Copy link
Member

robbavey commented Mar 5, 2020

Jenkins test this please

2 similar comments
@andsel
Copy link
Contributor Author

andsel commented Mar 6, 2020

Jenkins test this please

@andsel
Copy link
Contributor Author

andsel commented Mar 6, 2020

Jenkins test this please

@andsel
Copy link
Contributor Author

andsel commented Mar 6, 2020

@robbavey I don't why keep failing on Failure/Error: "http_address" => stats.get_shallow(:http_address).value, but that code was already present, before the PR. Do you have any hint why it started failing?
https://logstash-ci.elastic.co/job/elastic+logstash+pull-request+multijob-x-pack-unit/1077/consoleFull

@robbavey
Copy link
Member

robbavey commented Mar 6, 2020

@andsel The http_address metric is only populated when the webserver running inside Logstash starts. I suspect there is a race between the webserver starting up and the test being run which expects the http_address metric to be present

@andsel
Copy link
Contributor Author

andsel commented Mar 9, 2020

Jenkins test this please

2 similar comments
@andsel
Copy link
Contributor Author

andsel commented Mar 9, 2020

Jenkins test this please

@andsel
Copy link
Contributor Author

andsel commented Mar 10, 2020

Jenkins test this please

@andsel
Copy link
Contributor Author

andsel commented Mar 10, 2020

@robbavey I simply added a more long waiting (with timeout) condition on the availability of :http_address, is not so nice, but it's on the same model used in the lines above

@robbavey
Copy link
Member

Jenkins test this please

@robbavey
Copy link
Member

@andsel Just want to run this through CI another time to make sure we are stable, and not introducing a new flaky test

@andsel
Copy link
Contributor Author

andsel commented Mar 11, 2020

Jenkins test this please

1 similar comment
@andsel
Copy link
Contributor Author

andsel commented Mar 11, 2020

Jenkins test this please

robbavey
robbavey approved these changes Mar 11, 2020
View reviewed changes
Copy link
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM - nice work tracking down that flaky test. Do we need to port that change to master too?

@andsel
Copy link
Contributor Author

andsel commented Mar 11, 2020

@robbavey I changed the wait condition to look at not throwing exception

@andsel
Copy link
Contributor Author

andsel commented Mar 11, 2020

Yes, we need to port that change also to master, but other PR on master after I've integrated this one on 7.7 :-)

@andsel
Adaptations to internal collector to send data directly to monitoring…
32f8725 
… cluster Close 11573

Added check on HTTP server before asking for monitoring data in unit test
Fixes elastic#11541
andsel added a commit to andsel/logstash that referenced this pull request Mar 11, 2020
@andsel
Fixed flaky test, as fixed in packport PR elastic#11641
613ab44
@andsel andsel mentioned this pull request Mar 11, 2020
Closed
@andsel
Copy link
Contributor Author

andsel commented Mar 11, 2020

Jenkins test this please

@andsel
Copy link
Contributor Author

andsel commented Mar 12, 2020

Jenkins test this please

elasticsearch-bot pushed a commit that referenced this pull request Mar 12, 2020
@andsel
Fixed flaky test, as fixed in packport PR #11641
dfdc6f2 
Fixes #11679
@elasticsearch-bot
Copy link

Andrea Selva merged this into the following branches!

Branch Commits
7.x d901616

elasticsearch-bot pushed a commit that referenced this pull request Mar 12, 2020
@andsel
Adaptations to internal collector to send data directly to monitoring…
d901616 
… cluster Close 11573 Added check on HTTP server before asking for monitoring data in unit test Fixes #11541

Fixes #11641
@roaksoax roaksoax merged commit 7116d39 into elastic:7.x Mar 13, 2020
roaksoax added a commit that referenced this pull request Mar 13, 2020
@roaksoax
Revert "Adaptations to internal collector to send data directly to mo…
ce56da1 
…nitoring cluster Close 11573 (#11641)"

This reverts commit 7116d39.
@roaksoax roaksoax mentioned this pull request Mar 13, 2020
Merged
roaksoax added a commit that referenced this pull request Mar 13, 2020
@roaksoax
Revert "Adaptations to internal collector to send data directly to mo…
89aaebb 
…nitoring cluster Close 11573 (#11641)" (#11687)

This reverts commit 7116d39.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robbavey robbavey robbavey approved these changes

Assignees
No one assigned
Labels
v7.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andsel @robbavey @elasticsearch-bot @roaksoax