NETOBSERV-2534: Have a way to pause Network Observability functions#2362
NETOBSERV-2534: Have a way to pause Network Observability functions#2362jpinsonneau wants to merge 3 commits intonetobserv:mainfrom
Conversation
jpinsonneau
changed the title
jpinsonneau
added
the
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:93e5ad9 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-93e5ad9Or 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-sha-93e5ad9
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2362 +/- ##
==========================================
- Coverage 72.59% 72.41% -0.18%
==========================================
Files 104 104
Lines 10615 10644 +29
==========================================
+ Hits 7706 7708 +2
- Misses 2432 2462 +30
+ Partials 477 474 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
github-actions
bot
removed
the
ok-to-test
internal/pkg/cleanup/cleanup.go
Outdated
| if err := deleteResourcesByType(ctx, cl, listObj, labelSelector); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
(nit) Is there any reason to separate it into two lists?
There was a problem hiding this comment.
That was for clarity but there is no technical reason here.
Would you prefer all in one keeping the comments to separate namespaced resources and cluster ones ?
internal/pkg/cleanup/cleanup_test.go
Outdated
| }).WithTimeout(timeout).WithPolling(interval).Should(BeTrue()) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Wow, that's a lot of testing code for something that's not likely to break. This is more a comment than something to do.
The most likely issue in the future is that a new resource is used and the list of resources to delete from isn't updated.
There was a problem hiding this comment.
I agree it's a defensive approach and we can simplify it. The goal is to cover the most important resources types and ensure it doesn't delete the ones not managed by the operator.
If we prefer, we can reduce that test to the strict minimal and check the list of types in another test.
There was a problem hiding this comment.
Thinking more about this, we could even parse the RBAC to ensure the list of kinds we delete is in sync with the roles 🤔
But the test file will be even bigger 😆
|
/ok-to-test |
openshift-ci
bot
added
the
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:461b187 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-461b187Or 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-sha-461b187
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
|
Hi @jpinsonneau , Tested and working fine Only one point i want to confirm, When we put Flowcollector on Hold, these components not removed. Is It expected? |
|
jotak
changed the title
|
@jpinsonneau: This pull request references NETOBSERV-2534 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
|
@kapjain-rh yes it's expected that the operator itself, and the static plugin, remain active: they are not part of the flow collection process. Edit: actually, looking at the code, it seems there was the intent to disable the static plugin too. |
| Namespace string `json:"namespace,omitempty"` | ||
|
|
||
| // `onHold` indicates whether the operator is in hold mode. When enabled, the operator deletes all managed | ||
| // resources (except CRDs and namespaces) while preserving FlowCollector, FlowCollectorSlice, and FlowMetric |
There was a problem hiding this comment.
| // resources (except CRDs and namespaces) while preserving FlowCollector, FlowCollectorSlice, and FlowMetric | |
| // resources (except CRs and namespaces) while preserving FlowCollector, FlowCollectorSlice, and FlowMetric |
I think we're talking about the custom resources here and not the custom resource definitions (CRDs are not operator-managed anyway)
| // In hold mode, disable static plugin to trigger cleanup | ||
| enableStaticPlugin := !r.mgr.Config.Hold | ||
| if r.mgr.Config.Hold { | ||
| clog.Info("Hold mode enabled: disabling Static console plugin") | ||
| } | ||
|
|
||
| if r.mgr.ClusterInfo.HasConsolePlugin() { |
There was a problem hiding this comment.
If I understand correctly, we want the static plugin to stay active, in order to display the banner. So it seems we should revert those changes.
PS I'll take care of it during Julien's absence
There was a problem hiding this comment.
Oks, I think than it is working good.
you think some changes required based on discussion in syncup or we can close this for now
There was a problem hiding this comment.
I will bring some changes as discussed
|
/hold |
|
New changes are detected. LGTM label has been removed. |
github-actions
bot
removed
the
ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
(rebased) |
jotak
added
ok-to-test
|
New images: quay.io/netobserv/network-observability-operator:bd407e4
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-bd407e4
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-bd407e4
They will expire in two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:bd407e4
make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-bd407e4
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-sha-bd407e4
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
5 participants
Description
HOLDflag in CSV to hold controller reconciliation and delete all resources appart from user CRs and NamespacesAlso polished the plugin for clarity: netobserv/netobserv-web-console#1224
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.