NETOBSERV-1076 Fix service change detection #360
Conversation
use DeepDerivative on labels & annotations only, rather than full ObjectMeta
jotak
added
the
ok-to-test
Codecov Report
@@ Coverage Diff @@
## main #360 +/- ##
==========================================
+ Coverage 53.37% 53.53% +0.15%
==========================================
Files 45 45
Lines 5328 5329 +1
==========================================
+ Hits 2844 2853 +9
+ Misses 2281 2273 -8
Partials 203 203
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
New images:
They will expire after two weeks. Catalog source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-e50247e
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
|
/label qe-approved |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak 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 |
|
/lgtm |
| @@ -75,7 +75,8 @@ func probeChanged(old, new *corev1.Probe) bool { | |||
| } | |||
|
|
|||
| func ServiceChanged(old, new *corev1.Service, report *ChangeReport) bool { | |||
| return report.Check("Service meta changed", !equality.Semantic.DeepDerivative(new.ObjectMeta, old.ObjectMeta)) || | |||
| return report.Check("Service annotations changed", !equality.Semantic.DeepDerivative(new.Annotations, old.Annotations)) || | |||
| report.Check("Service labels changed", !equality.Semantic.DeepDerivative(new.Labels, old.Labels)) || | |||
There was a problem hiding this comment.
What part of corev1.Service changed that triggered this to return true, or does it not matter because it's clear that only Annotations and Labels are relevant? Why was this not happening before?
There was a problem hiding this comment.
Many parts of the ObjectMeta will differ when comparing an existing object with a newly built object; e.g. are the uuid, revision number, etc. It's safe to rely only on labels & annotations because that's at the moment the only things in ObjectMeta that we partly derive from configuration*. You can see an example here of a service that we create: https://github.com/netobserv/network-observability-operator/blob/main/controllers/consoleplugin/consoleplugin_objects.go#L300-L322
*: with the exception of the namespace, but a change of namespace triggers a whole different reconciliation process.
Why it happens only now? I wasn't sure... but now I remember something: in #326, during the refactoring I fixed something related to the DidChange / IsInProgress stuff: previously it wasn't fully working in every situation because these were fields of a structure that happened to be sometimes copied as argument - which means that the "didChange" field of the parent was never updated when the one from the copy was. I fixed it by making didChange/isInProgress variables now only created and owned from the parent, and updated via lambdas.
So, tl;dr: it was happening before, but without the same impact on the "Updating" CR status.
There was a problem hiding this comment.
Thanks for the detailed explanation.
use DeepDerivative on labels & annotations only, rather than full ObjectMeta