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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DoNotMerge] Ip Reconciler Alerting #233

Conversation

nicklesimba
Copy link
Collaborator

Preface: this is a dupe of #218 created to better separate changes into commits for easier code review.

WIP pull request for ip-reconciler alerting. This PR introduces 2 changes:

  1. enables the ip-reconciler to be invoked via the ip-control-loop, deprecating the ip-reconciler binary. This is done simply be reorganizing the files and removing a line in the build script that creates the reconciler binary.
  2. sends metrics to prometheus when the ip-reconciler succeeds, so that cluster-network-operator can read the metrics and send alerts accordingly. (as a result, whereabouts code will interact with CNO yaml when everything is working correctly)

Aside from simple code cleanup, this is mostly a finished PR. Once functionality with alerting is confirmed to work with CNO, the code here in whereabouts should be ready to go.

Note: These changes are listed under the same pull request because it doesn't make sense to merge one without the other.

@lgtm-com
Copy link

lgtm-com bot commented May 30, 2022

This pull request introduces 1 alert when merging 9176123 into 2789b2e - view on LGTM.com

new alerts:

  • 1 for Unreachable statement

@lgtm-com
Copy link

lgtm-com bot commented May 30, 2022

This pull request introduces 1 alert when merging 41266e1 into 2789b2e - view on LGTM.com

new alerts:

  • 1 for Unreachable statement

@maiqueb
Copy link
Collaborator

maiqueb commented Jun 8, 2022

First of, I don't understand why you abandoned #218 and started off another PR. The history of the reviews will be gone unless someone is really into digging through the chain of PRs you've created.

Secondly, as I've stated in #218 (comment), this PR is doing 2 different things. As such, there should be 2 different PRs:

  • 1 for removing the reconciler binary, moving that logic into the main loop of IP reconciliation
  • another for adding metrics

That allows the reviewers to focus on the problem at hand - and again, there are two.

I don't buy the argument that "it doesn't make sense to merge one without the other." It makes sense to merge one without the other.

Do we want to get rid of the cronjob ? Yes. Why should we hold on that until we get the metrics right ? No logical reason I can think of.

Furthermore, adding metrics introduces the prometheus dependencies which are adding +- 150 files to this repo. If everything related to metrics goes in a separate PR reviewers can focus on the refactor you're doing, thus increasing the odds the refactor is correctly introduced.

@nicklesimba nicklesimba closed this Jun 9, 2022
@nicklesimba
Copy link
Collaborator Author

Closed in favor of #238 and #239

@nicklesimba nicklesimba deleted the ip-reconciler-alerting branch June 9, 2022 19:07
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.

2 participants