-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
update default image versions #3007
update default image versions #3007
Conversation
I already opened a similar PR #2901. But the CI always fails. |
Ah sorry, I didn't see that one! |
@yeya24 I think its because of the ordering of your vars. If you don't mind closing yours, as this one fixes a few more things. Hope thats okay? :) Thanks! |
@lilic I don't think it's the ordering, since if that was the issue it would probably fail early during build, wouldn't it? |
@lilic Sure, I will close mine. |
Yes, its a global var not inline defined, strange. 🤷♀ |
Looks like the same test failure we saw before, I'll try to investigate further. |
I tracked the issue down to prometheus v2.8.0 and higher, and seems to be caused by a change in the error handling in the scrape client (prometheus/prometheus#5182). The issue is that the certificates are not available in the pod when the test is running, and in Prometheus 2.7.2 and lower, the invalid cert actually causes Prometheus to crash with nil pointer.
And this allows the test to succeed for some reason. Prometheus 2.8.0 and later prints the error but keeps running, but I guess it's not able to scrape and the test just times out. Anyway, this seems to be a problem in our tests and not an issue with Prometheus. |
Prometheus image should default to latest version in compatibility matrix. Also updates Alertmanager and Thanos to latest stable versions.
The allowed-tls-file and denied-tls-file tests of service monitor config were not able to access tls certificate files on the local filesystem. This issue was only exposed when updating the default Prometheus version above 2.8.0 because this version introduced validation of the TLS config. This changes the config for these two tests to mount a secret containing these two files.
13fab85
to
869f7b7
Compare
Could we double check whether an empty file would have the same result? I think it’s just the existence of a file that is referenced. |
@brancz Looks like it needs a real certificate, the test fails with a timeout when using an empty file, and the Prometheus logs print an error.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! lgtm
Prometheus image should default to latest version in compatibility
matrix. Also updates Alertmanager and Thanos to latest stable versions.