NETOBSERV-909 fix servicemonitor & prom rule reconcile #290
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #290 +/- ##
==========================================
+ Coverage 47.57% 49.69% +2.11%
==========================================
Files 43 43
Lines 5019 4984 -35
==========================================
+ Hits 2388 2477 +89
+ Misses 2420 2305 -115
+ Partials 211 202 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jotak
force-pushed
the
fix-reconcile-on-upgrade
branch
2 times, most recently
from
3f819d6 to
3b6c395
Compare
KalmanMeth
previously approved these changes
Mar 9, 2023
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: ["quay.io/netobserv/network-observability-operator:a35d384"]. It will expire after two weeks. |
|
@jotak - I am seeing an error still this time with console-plugin reconcilation, if I tried procedure as described in ticket: |
jotak
force-pushed
the
fix-reconcile-on-upgrade
branch
from
3b6c395 to
e0e9d0d
Compare
github-actions
bot
removed
the
ok-to-test
|
@OlivierCazade can you take a second look please? @memodi pointed out an issue that turned out needing more changes, cf my last commit e0e9d0d |
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
/ok-to-test |
|
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-075326e
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
|
@memodi I retried and cannot reproduce the initial issue with this fix applied. Maybe we need to check if our reproduction scenario differ. What I do is:
|
|
Just retried using loki-operator instead of zero-click, to make sure it isn't related to that : still no issue |
We need to be careful about not tying lifecycle of different objects together, as this would be assuming a certain state, which creates "holes" in the reconcile logic - we must avoid as much as possible any assumption about the current state
Codecov fails too often to upload. Make it non mandatory Also increase the timeout for some tests
They are also flawed: we should not assume they are created once for all when flowcollector is created. E.g. of how it could go wrong: 1. an old version of an operator is installed, with a flowcollector 2. an upgrade is done, which introduces a new "static" resource => that new resource would not be created, since the FlowCollector object was already present hence did not trigger these "static init" things
jotak
force-pushed
the
fix-reconcile-on-upgrade
branch
from
e0e9d0d to
34f0fbb
Compare
github-actions
bot
removed
the
ok-to-test
jotak
added
the
ok-to-test
|
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-7a17fa1
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
|
@memodi the catalog issue should be fixed, I regenerated a new one |
|
/label qe-approved thanks @jotak - confirming both the issues - Reconciliation during upgrade and NOO image with pre-merge catalog image - are fixed with most recent changes. |
jpinsonneau
reviewed
Mar 16, 2023
| @@ -29,7 +29,7 @@ jobs: | |||
| with: | |||
| files: ./cover.out | |||
| flags: unittests | |||
| fail_ci_if_error: true | |||
| fail_ci_if_error: false | |||
There was a problem hiding this comment.
Is that the solution to flaky test coverage ? Will it skip if it fails to upload for example ?
There was a problem hiding this comment.
yeah well, IMO coverage upload is not something critical enough to make whole CI fail, given how unstable it is
jpinsonneau
approved these changes
Mar 16, 2023
|
/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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
bug tld;dr:
We need to be careful about not tying lifecycle of different objects together, as this would be assuming a certain state, which creates "holes" in the reconcile logic - we must avoid as much as possible any assumption about the current state