-
Notifications
You must be signed in to change notification settings - Fork 24
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
NETOBSERV-1224 Flowcollector does not report status != Ready in OCP Console #403
Conversation
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:801a9e0 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-801a9e0 Or as a 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-801a9e0
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #403 +/- ##
==========================================
+ Coverage 55.49% 55.69% +0.19%
==========================================
Files 45 45
Lines 5874 5884 +10
==========================================
+ Hits 3260 3277 +17
+ Misses 2393 2387 -6
+ Partials 221 220 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
cond.Status = metav1.ConditionTrue | ||
meta.SetStatusCondition(&fc.Status.Conditions, *cond) | ||
sortConditions(fc) | ||
} |
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.
while you are here lets add ut for condition package
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.
Sure I'll do 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've added some tests but these seems flaky 🤔
fda8f2a
I'm digging to find why:
-> seems to be related to console plugin reconciliation 🤕
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.
@jotak can you please explain the following action ?
network-observability-operator/controllers/flowcollector_controller_test.go
Lines 378 to 381 in 725e26f
// Do a dummy change that will trigger reconcile, and make sure SM is created again | |
UpdateCR(crKey, func(fc *flowslatest.FlowCollector) { | |
fc.Spec.Processor.LogLevel = "info" | |
}) |
Why do we need to trigger the loop ? Is it a behavior related to the test bench ?
As far as I understand, ginkgo run all of these Context
in parallel; that's why I get flaky tests checking the status in the middle of the test bench.
If I check for Ready in Cleanup
it works but I also need to trigger the reconcile loop which doesn't make sense to me
a0dc9ae
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 cannot recall now, maybe this UpdateCR isn't necessary, but I guess if I added that it's because the kube test env didn't trigger the reconcile as I expected (the goal here being to make sure the ServiceMonitor is recreated - I remember I added this test after a bug where SM wasn't reconciled.
I remember that a while ago I removed some condition-checking code in tests because iirc it wasn't working as expected with kube testenv, and led to flaky tests ... but I don't remember all the details
/ok-to-test |
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:117d5bb make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-117d5bb Or as a 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-117d5bb
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
/label qe-approved |
Shoould this be tested again? @jpinsonneau |
No need @Amoghrd, only unit tests were added. I'm just waiting for devs LGTM 😄 |
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.
LGTM
@jpinsonneau I checked " Does this PR require a product release notes entry? ", I guess it's needed here? If so can you add it to the JIRA? |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpinsonneau 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 |
Description
Improve CR status update:
condition.Status
field. Only the active one should showTrue
Ready
/Pending
/Failed
messages with only one of theseTrue
Status
fieldScreencast.from.2023-08-23.11-15-55.webm
Related issues:
https://issues.redhat.com/browse/NETOBSERV-1224
https://issues.redhat.com/browse/NETOBSERV-610
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.