-
Notifications
You must be signed in to change notification settings - Fork 357
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
OCPBUGS-17113: Lazy updates for Prometheus #2395
base: master
Are you sure you want to change the base?
Conversation
@rexagod: This pull request references Jira Issue OCPBUGS-17113, 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rexagod The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/jira refresh |
@rexagod: This pull request references Jira Issue OCPBUGS-17113, which is valid. 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. |
484f320
to
d394434
Compare
/retest-required |
d394434
to
29846c5
Compare
/retest-required |
1 similar comment
/retest-required |
Incorporate openshift/library-go#1575 downstream. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
29846c5
to
564d684
Compare
/test e2e-aws-ovn-techpreview |
/label qe-approved |
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.
Thanks!
I left some comments.
pkg/client/client.go
Outdated
_, err = c.dclient. | ||
Resource(prometheusGVR). | ||
Namespace(structuredRequiredPrometheus.GetNamespace()). | ||
Create(ctx, unstructuredRequiredPrometheus, metav1.CreateOptions{}) |
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 can delegate the creation to ApplyUnstructuredResourceImproved
as it already does that.
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 see. I tried to preserve the earlier behavior, albeit at the cost of no cache updates during creation, but this makes sense. Done!
pkg/client/client.go
Outdated
return &didUpdate, nil | ||
} | ||
|
||
func prometheusDefaultingFunc(unstructuredPrometheus *unstructured.Unstructured) { |
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 thought that after our discussion here openshift/library-go#1575 (comment), we "agreed" to give the "cache approach" a try for now, as it's simpler (less code/less maintenance), and with the cache, IIUC, I don't think we need to care about the defaults (if the wanted config is the one stored in the cache which mean the one deployed, skip the update).
Any reasons behind the change?
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.
Referring to the same thread: openshift/library-go#1575 (comment), we do this in addition to the cache approach since that doesn't cache spec
fields, in case of a metadata cache miss (same as what Trevor's patch targets in ef5f348#diff-2895f30116602d8fe6b0545c186d4f81247fa4eaab049dab174c6c91c0036e08R639-R667).
c := &Client{} | ||
c.dclient = dynamicFake.NewSimpleDynamicClient(runtime.NewScheme(), initialPrometheus.DeepCopy()) | ||
c.eventRecorder = events.NewInMemoryRecorder("create-or-update-prometheus-test") | ||
c.resourceCache = resourceapply.NewResourceCache() |
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.
Note that the code seems to be using the "comparison approach" (I don't see any cache initialized there), but here we're testing the "cache approach", am I missing sth?
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.
Both of these are in use (PTAL my comment https://github.com/openshift/cluster-monitoring-operator/pull/2395/files#r1696698968). Here, we initialize the cache, and down below we use the defaulting approach to reject default spec
updates (shouldUpdate
is false
for this case).
} | ||
unstructuredRequiredPrometheusMetadataLabels := unstructuredRequiredPrometheus.GetLabels() |
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.
these may be worth a comment.
Just to be sure I understand, is this done to avoid
#### NOTE: |
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.
That's done to preserve the past behavior: PTAL rexagod@529b005#diff-2895f30116602d8fe6b0545c186d4f81247fa4eaab049dab174c6c91c0036e08R301.
pkg/client/client.go
Outdated
func (c *Client) CreateOrUpdatePrometheus(ctx context.Context, p *monv1.Prometheus) error { | ||
pclient := c.mclient.MonitoringV1().Prometheuses(p.GetNamespace()) | ||
existing, err := pclient.Get(ctx, p.GetName(), metav1.GetOptions{}) | ||
func (c *Client) CreateOrUpdatePrometheus(ctx context.Context, structuredRequiredPrometheus *monv1.Prometheus) (*bool, error) { |
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.
why do we need to return a pointer instead of just a bool here?
I don't think tests should rely on this, I think we should retrieve whether the object was updated from the API/client.
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 looked at the usages and there doesn't seem to be a need for three states. Fixed! 👍🏼
5b0d83b
to
4e75b4a
Compare
@rexagod: all tests passed! 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. |
shouldUpdate: true, | ||
}, | ||
{ | ||
description: "apply no value to the existing prometheus", | ||
prometheus: &monv1.Prometheus{ | ||
TypeMeta: initialPrometheus.TypeMeta, | ||
ObjectMeta: initialPrometheus.ObjectMeta, | ||
}, | ||
shouldUpdate: true, |
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.
With the no defaulting approach, there won't be a difference between default and non-default applications, as both will trigger updates as compared to just the latter earlier, at the benefit of not maintaining hardcoded default values in CMO. LMK if I should revert this or if this sounds good.
Incorporate openshift/library-go#1575 downstream.