-
Notifications
You must be signed in to change notification settings - Fork 26
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-1342: new FlowMetric API (dev preview) #508
Conversation
@jotak: This pull request references NETOBSERV-1342 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.15.0" version, but no target version was set. 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 kubernetes/test-infra repository. |
@jotak: This pull request references NETOBSERV-1342 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.15.0" version, but no target version was set. 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 kubernetes/test-infra repository. |
@jotak: This pull request references NETOBSERV-1342 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.15.0" version, but no target version was set. 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 kubernetes/test-infra repository. |
Here's an example that creates a metric to track pods talking with cluster-external IPs (or supposed to be ... not 100% accurate) : apiVersion: flows.netobserv.io/v1alpha1
kind: FlowMetric
metadata:
name: flowmetric-pods-talking-outside
spec:
metricName: pods_talking_outside
type: Counter
valueField: Bytes
labels: [SrcK8S_Name,SrcK8S_Namespace,SrcK8S_OwnerName,SrcK8S_OwnerType,DstAddr]
filters:
- field: DstK8S_OwnerType
matchType: Absence
- field: SrcK8S_Type
value: Pod
- field: Duplicate
value: "false"
- field: FlowDirection
value: "1|2"
matchType: Regex
|
@jotak: This pull request references NETOBSERV-1342 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.15.0" version, but no target version was set. 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 kubernetes/test-infra repository. |
@jotak: This pull request references NETOBSERV-1342 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.16.0" version, but no target version was set. 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 kubernetes/test-infra repository. |
@jotak: This pull request references NETOBSERV-1342 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.16.0" version, but no target version was set. 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 kubernetes/test-infra repository. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #508 +/- ##
===========================================
- Coverage 66.72% 56.18% -10.55%
===========================================
Files 63 69 +6
Lines 7526 9074 +1548
===========================================
+ Hits 5022 5098 +76
- Misses 2185 3647 +1462
- Partials 319 329 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pkg/test/envtest.go
Outdated
@@ -25,6 +25,7 @@ import ( | |||
"sigs.k8s.io/controller-runtime/pkg/log/zap" | |||
"sigs.k8s.io/controller-runtime/pkg/metrics/server" | |||
|
|||
flowsv1alpha1 "github.com/netobserv/network-observability-operator/api/v1alpha1" |
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.
We could actually decorrelate flowcollector
and flowmetric
by renaming api folder to apis
with a subfolders for each of these. Here is an example with loki operator:
https://github.com/grafana/loki/tree/b18078827ea320bf6d9494f2eef8b315074934c6/operator/apis
Then you can add to scheme independently even if the packages names are the same (v1 in this example):
https://github.com/grafana/loki/blob/b18078827ea320bf6d9494f2eef8b315074934c6/operator/main.go#L41-L43
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.
good idea, done
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 ! Would it make sense to keep deprecation on FlowCollector
v1alpha1 then ?
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.
api/v1alpha1/flowmetric_types.go
Outdated
type FlowMetricStatus struct { | ||
// INSERT ADDITIONAL STATUS FIELD - define observed state of cluster | ||
// Important: Run "make" to regenerate code after modifying this file | ||
} |
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.
It would be great to have a validation result here in case something went wrong.
At first maybe just report if flowMetricToFLP
returned an error ?
In the future we could improve to check valueField
, labels
and filters
are correct for example.
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.
This looks like something to do in a validation webhook, no?
I am creating a story for it: https://issues.redhat.com/browse/NETOBSERV-1447
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
@jotak: This pull request references NETOBSERV-1342 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.16.0" version, but no target version was set. 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. |
/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:602d080 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-602d080 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-602d080
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
@jotak - I regression tested this all of the backend tests except upgrade test and all passed: I just have couple of concerns around noting it as dev preview in CRD, I have noted them in slack thread here |
- operator-sdk code scaffolding to create a new API called FlowMetrics - initial implementation of the api type - remove v1alpha1 deprecation since this is also a v1alpha1 sharing same package - shrinking the deprecated flowcollector v1alpha1 alm sample to simplify CSV - since v1alpha1 is imported in business code again, there was an import cycle with helper package=> to fix it, moving some code out of "helper" Implement converting new API to FLP config Provide some examples Add watch, add tests, some syntax sugar Ad omitempty
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:0b19c17 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-0b19c17 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-0b19c17
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
/lgtm |
/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 |
Description
package
CSV
import cycle with helper package=> to fix it, moving some code out of
"helper"
Follow-ups:
Dependencies
Based on #510
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.