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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix panic when collecting metrics from Prometheus Agent controller #5511

Merged

Conversation

ArthurSens
Copy link
Member

Description

Fixes #5508

Just waiting for confirmation in case the mentioned new label is really needed 馃

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

Fix panic when collecting metrics from agent controller

@simonpasquier
Copy link
Contributor

lgtm. Have you checked that it fixes the issue? I guess we also need an e2e test that scrapes metrics from the operator :)
Can you submit the PR against the release-0.64 branch? I'll cut a 0.64.1 release.

@philipgough
Copy link
Contributor

Looks good but would also like to see an e2e test

@slashpai
Copy link
Contributor

slashpai commented Apr 18, 2023

Can we add #5476 also in 0.64.1 to avoid alerting when crd not installed? I am working on changes

@ArthurSens
Copy link
Member Author

Agreed that an e2e would be great here, but I'm struggling to do so to be honest 馃槗 #5471

@ArthurSens
Copy link
Member Author

ArthurSens commented Apr 18, 2023

Manual tests worked tho

image

@simonpasquier
Copy link
Contributor

@slashpai I'm afraid that the change won't apply cleanly on the release-0.64 branch because of #5472

@simonpasquier
Copy link
Contributor

I'm fine with the e2e test in a follow-up PR.

@simonpasquier
Copy link
Contributor

Manual tests worked tho

image

I assume that you've checked prometheus_operator_spec_replicas too?

@ArthurSens
Copy link
Member Author

Manual tests worked tho
image

I assume that you've checked prometheus_operator_spec_replicas too?

I did not, just one sec 馃槢

@ArthurSens
Copy link
Member Author

Yeah, all good!

image

Tested with prometheus and prometheus agent running at the same time

@simonpasquier simonpasquier changed the base branch from main to release-0.64 April 18, 2023 20:25
@simonpasquier simonpasquier changed the base branch from release-0.64 to main April 18, 2023 20:26
Copy link
Contributor

@JoaoBraveCoding JoaoBraveCoding left a comment

Choose a reason for hiding this comment

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

lgtm

@simonpasquier
Copy link
Contributor

@ArthurSens any chance you can rebase your change on top of the release-0.64 branch?

Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
(cherry picked from commit 88a0435)
@ArthurSens
Copy link
Member Author

Rebased on top of release-0.64, I'm not sure I have the permissions to change the PR target branch though

@simonpasquier simonpasquier changed the base branch from main to release-0.64 April 21, 2023 13:56
@simonpasquier
Copy link
Contributor

simonpasquier commented Apr 21, 2023

I'm not sure I have the permissions to change the PR target branch though

As the author, you should see the Edit button at the top of the page.

@ArthurSens
Copy link
Member Author

I'm not sure I have the permissions to change the PR target branch though

As the author, you should see the Edit button at the top of the page.

TIL!

@simonpasquier simonpasquier merged commit af55071 into prometheus-operator:release-0.64 Apr 24, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operator v0.64.0 got panic after creating the CRD PrometheusAgent
5 participants