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

Fix NodeMonitoring breaks target status #786

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Fix NodeMonitoring breaks target status #786

merged 1 commit into from
Jan 30, 2024

Conversation

TheSpiritXIII
Copy link
Member

@TheSpiritXIII TheSpiritXIII commented Jan 26, 2024 •
edited
Loading

After NodeMonitoring was added, when it is configured, target status stops working completely. Long story short, the problem is early exit on an unknown scrape pool format.

This change includes some refactoring originally part of the ProbeMonitoring PR (#766) but also adds in the possibility of supporting a NodeMonitoring target status if we choose to add it in the future, by at the very least detecting and validating the scrape pool format.

Changes include:

  • Ensures we still patch everything on error (with new unit test case).
  • Show all errors, not just the last one we found.
  • Ensures we parse NodeMonitoring scrape pool formats (with new unit test case).
  • Validate expected errors in the test cases.

Thanks @bwplotka for finding this issue!

@github-actions github-actions bot requested a review from pintohutch January 26, 2024 20:46
pintohutch
pintohutch requested changes Jan 26, 2024
View reviewed changes
Copy link
Collaborator

@pintohutch pintohutch left a comment

Choose a reason for hiding this comment

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

Thanks!

Can you also, either:

  1. Create an issue describing the bug, or
  2. Add the issue description in the PR

I think I understand the problem, but had to do a bit of reverse-engineering here. I may still not have the full picture.

pkg/operator/target_status.go Show resolved Hide resolved
maxamins
maxamins requested changes Jan 26, 2024
View reviewed changes
Copy link
Collaborator

@maxamins maxamins left a comment

Choose a reason for hiding this comment

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

Thanks for quickly coding this! I have minor comments and I think it should be good to go after.

pkg/operator/endpoint_status_builder.go Outdated Show resolved Hide resolved
pkg/operator/endpoint_status_builder.go Outdated Show resolved Hide resolved
pkg/operator/endpoint_status_builder.go Outdated Show resolved Hide resolved
pkg/operator/endpoint_status_builder.go Show resolved Hide resolved
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/node-monitoring-target-fix branch 2 times, most recently from af81bef to 88f495b Compare January 28, 2024 20:20
@TheSpiritXIII TheSpiritXIII mentioned this pull request Jan 29, 2024
Merged
bwplotka
bwplotka approved these changes Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks 👍🏽 LGTM

pkg/operator/target_status.go Show resolved Hide resolved
pintohutch
pintohutch reviewed Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@pintohutch pintohutch left a comment

Choose a reason for hiding this comment

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

Round 2

pkg/operator/endpoint_status_builder.go Outdated Show resolved Hide resolved
pkg/operator/endpoint_status_builder.go Show resolved Hide resolved
pkg/operator/target_status.go Show resolved Hide resolved
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/node-monitoring-target-fix branch from 88f495b to d3f905e Compare January 29, 2024 13:50
pintohutch
pintohutch approved these changes Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@pintohutch pintohutch left a comment

Choose a reason for hiding this comment

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

:shipit:

@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/node-monitoring-target-fix branch from d3f905e to a688dce Compare January 29, 2024 20:47
maxamins
maxamins requested changes Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@maxamins maxamins left a comment

Choose a reason for hiding this comment

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

Thanks!

pkg/operator/endpoint_status_builder.go Outdated Show resolved Hide resolved
pkg/operator/endpoint_status_builder.go Outdated Show resolved Hide resolved
pkg/operator/endpoint_status_builder.go Show resolved Hide resolved
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/node-monitoring-target-fix branch from a688dce to 0932746 Compare January 29, 2024 22:46
maxamins
maxamins approved these changes Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@maxamins maxamins left a comment

Choose a reason for hiding this comment

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

Amazing 💪

@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/node-monitoring-target-fix branch 2 times, most recently from b9842be to a2df2c4 Compare January 30, 2024 03:55
@TheSpiritXIII
fix: resolve enabling NodeMonitoring breaks target status
4f75293 
After NodeMonitoring was added, when it is configured, target status stops working completely. Long story short, the problem is early exit on an unknown scrape pool format.

This change includes some refactoring originally part of the ProbeMonitoring PR (#766) but also adds in the possibility of supporting a NodeMonitoring target status if we choose to add it in the future, by at the very least detecting and validating the scrape pool format.

Changes include:

- Ensures we still patch everything on error (with new unit test case).
- Show all errors, not just the last one we found.
- Ensures we parse NodeMonitoring scrape pool formats (with new unit test case).
- Validate expected errors in the test cases.

Thanks @bwplotka for finding this issue!
@TheSpiritXIII TheSpiritXIII force-pushed the TheSpiritXIII/node-monitoring-target-fix branch from a2df2c4 to 4f75293 Compare January 30, 2024 15:24
@TheSpiritXIII TheSpiritXIII merged commit a9cf882 into main Jan 30, 2024
12 checks passed
@TheSpiritXIII TheSpiritXIII deleted the TheSpiritXIII/node-monitoring-target-fix branch January 30, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pintohutch pintohutch pintohutch approved these changes

@maxamins maxamins maxamins approved these changes

@bwplotka bwplotka Awaiting requested review from bwplotka

Assignees

@pintohutch pintohutch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TheSpiritXIII @pintohutch @bwplotka @maxamins