-
Notifications
You must be signed in to change notification settings - Fork 169
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
PUCM Maintenance Signals #3021
PUCM Maintenance Signals #3021
Conversation
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.
Suggestion for the test cases
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.
Looks great, just one adjustment suggested. Thanks!
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.
Small suggestion but otherwise looks good.
@niontive We're planning on moving away from the "PUCM" terminology once the automation is going. Could we change this to a more generic "maintenancepending"? |
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.
I think this is not the way it should be done, I believe we should emit the metrics directly when we are updating the cluster, rather than storing the state in the database and then polling it. This will increase the load on the monitor when we don't need to. We have access to the metrics emitter directly from the frontend (and most likely the backend too) and we should use that from there directly imho.
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.
Thank you for this. Great work so far!
I left some suggestions to simplify a bit a few things in case you find them useful.
Thanks! 😃
@facchettos I reviewed this design with @bennerv and @s-amann. Below is the justification for choosing using the cluster monitoring stack vs just the frontend:
|
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.
Hi @niontive I do see all remarks are answered, the design is thought through and the code is ok. Thanks for the PR.
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.
LGTM! 🚀 Thanks for applying the suggestions.
Which issue this PR addresses:
Implement maintenance signals for this: https://issues.redhat.com/browse/ARO-1933
What this PR does / why we need it:
Test plan for issue:
Is there any documentation that needs to be updated for this PR?