-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add a metric of AppWrappers counts per state #675
base: main
Are you sure you want to change the base?
Conversation
Add dependency: sigs.k8s.io/controller-runtime v0.14.6 go get sigs.k8s.io/[email protected] go mod tidy
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
awList, err := controller.appWrapperLister.List(labels.Everything()) | ||
|
||
if err != nil { | ||
klog.Errorf("[updateQueue] Unable to obtain the list of AppWrappers: %+v", err) |
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.
In case of error, should we update the counters to 0 or leave them as is?
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 we should leave them as is and retry to update with new value.
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.
This update is done periodically. So instead of retrying, we can simply wait for the next update on the next tick.
I dug deeper into the implementation of controller.appWrapperLister.List(labels.Everything())
, and it seems that since labels.Everything().Empty()
returns true, there is no way that controller.appWrapperLister.List()
will return an error. So the discussion seems to be purely theoretical.
Issue link
Closes #674
What changes have been made
This PR adds a custom metric to expose how many AppWrappers are per state.
It is built on top of #651. To review only the delta, skip the first 3 commits in this PR.
Verification steps
Same as mentioned in #651, I checked that the metrics are available in the codeflare-operator operator by running it:
and in a new shell:
To see the counts change, I created a sample AppWrapper:
Checks