OCPBUGS-62493: Only fire OSImageURLOverridden and set metrics on changes #5319
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@sdodson: This pull request references Jira Issue OCPBUGS-62493, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| } else { | ||
| // Reset metric when OSImageURL has not been overridden | ||
| ctrlcommon.OSImageURLOverride.WithLabelValues(pool.Name).Set(0) | ||
| } |
There was a problem hiding this comment.
Maybe the metrics bits should be moved out of this portion of code so that if the controller is restarted they're always accurate?
There was a problem hiding this comment.
That could work, probably check if the existing metric is already at 0/1 and only re-event if that changes? (Although I don't think that's the right way to do event metrics, as far as I understand it)
| if err != nil { | ||
| return err | ||
| } | ||
| // Emit event and collect metric when OSImageURL was overridden and we're going to apply a new config |
There was a problem hiding this comment.
| // Emit event and collect metric when OSImageURL was overridden and we're going to apply a new config | |
| // Emit event and collect metric when OSImageURL was overridden and we're going to create a new config |
maybe?
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sdodson 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sdodson: This pull request references Jira Issue OCPBUGS-62493, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| ctrl.eventRecorder.Eventf(generated, corev1.EventTypeNormal, "OSImageURLOverridden", "OSImageURL was overridden via machineconfig in %s (was: %s is: %s)", generated.Name, cc.Spec.OSImageURL, generated.Spec.OSImageURL) | ||
| } else { | ||
| // Reset metric when OSImageURL has not been overridden | ||
| ctrlcommon.OSImageURLOverride.WithLabelValues(pool.Name).Set(0) |
There was a problem hiding this comment.
I think the reset here should probably live outside of this block, since the metric will stay at (1) if you delete the config in this workflow (and revert back to the old config)
There was a problem hiding this comment.
Where's that happening? I'm not seeing an obvious place where we do that :(
There was a problem hiding this comment.
Rather I think it probably makes more sense to have the metrics and eventing be separate processes, so the event only gets fired on changes, but we can keep the metric setting outside
There was a problem hiding this comment.
I think I've updated it to achieve that now.
| } else { | ||
| // Reset metric when OSImageURL has not been overridden | ||
| ctrlcommon.OSImageURLOverride.WithLabelValues(pool.Name).Set(0) | ||
| } |
There was a problem hiding this comment.
That could work, probably check if the existing metric is already at 0/1 and only re-event if that changes? (Although I don't think that's the right way to do event metrics, as far as I understand it)
Prior to this we're emitting the event everytime the config is built even though we're not applying it.
|
@sdodson: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
Prior to this we're emitting the event everytime the config is built even though we're not applying it.
Fixes: OCPBUGS-62493
- What I did
Moved the event and metrics update code to the bit of code that creates the new config if it's missing.
- How to verify it
set osImageURL and see if events are emitted each time the config is reconciled
- Description for the changelog
Emit OSImageURLOverridden only when creating a new config