Skip to content
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-567 Enable connection tracking in NOO #252

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Jan 24, 2023

To allow working / testing https://issues.redhat.com/browse/NETOBSERV-568 I started working on enabling connection tracking in netobserv operator.

The only configurable options are connectionUpdateInterval and ConnectionEndTimeout for consistency in console plugin.

@openshift-ci
Copy link

openshift-ci bot commented Jan 24, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ronensc
ronensc previously approved these changes Feb 2, 2023
@jpinsonneau
Copy link
Contributor Author

Holding this PR as we need console plugin merged at the same time

/hold

api/v1alpha1/flowcollector_types.go Outdated Show resolved Hide resolved
OlivierCazade
OlivierCazade previously approved these changes Feb 10, 2023
Copy link
Contributor

@OlivierCazade OlivierCazade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

api/v1alpha1/flowcollector_types.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm label Feb 10, 2023
KalmanMeth pushed a commit to KalmanMeth/network-observability-operator that referenced this pull request Feb 13, 2023
@openshift-ci openshift-ci bot removed the lgtm label Feb 14, 2023
@jpinsonneau
Copy link
Contributor Author

/hold for #258 to be merge

@@ -358,6 +358,16 @@ type FlowCollectorFLP struct {
// kafkaConsumerBatchSize indicates to the broker the maximum batch size, in bytes, that the consumer will accept. Ignored when not using Kafka. Default: 10MB.
KafkaConsumerBatchSize int `json:"kafkaConsumerBatchSize"`

//+kubebuilder:default:="30s"
// +optional
// connection update interval is the duration of time to wait between update reports of a connection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. There are no min/max. Also the default values were set arbitrarily.

api/v1beta1/flowcollector_types.go Outdated Show resolved Hide resolved
//+kubebuilder:default:={"flowLog"}
// outputRecordTypes is a list of record types to export from ["flowLog","newConnection","heartbeat","endConnection"]
// +optional
OutputRecordTypes *[]string `json:"outputRecordTypes,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Providing an empty array here could be an option to say "only export prometheus metrics". I probably need to update a bit the pipeline for that.

IIUC, the proposal is that an empty array will be equivilant to an array with a single value "flowLog" which indicates disabling connection tracking?

@jpinsonneau
Copy link
Contributor Author

Addressed feedback on ConnectionHeartbeatInterval renaming + OutputRecordTypes enum

@Amoghrd
Copy link
Contributor

Amoghrd commented Feb 27, 2023

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Feb 27, 2023
@github-actions
Copy link

New image: ["quay.io/netobserv/network-observability-operator:8680eee"]. It will expire after two weeks.

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Feb 28, 2023
@jpinsonneau
Copy link
Contributor Author

@msherif1234 splitted commits as requested

@Amoghrd also fixed the sample yaml

I have updated to flp v0.1.8 and rebased as there was conflicts in go dependencies

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Feb 28, 2023
@github-actions
Copy link

New image: ["quay.io/netobserv/network-observability-operator:76733a1"]. It will expire after two weeks.

@msherif1234
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 28, 2023
@Amoghrd
Copy link
Contributor

Amoghrd commented Mar 1, 2023

/qe-approved

@Amoghrd
Copy link
Contributor

Amoghrd commented Mar 1, 2023

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved QE has approved this pull request label Mar 1, 2023
@jpinsonneau
Copy link
Contributor Author

/approve

@openshift-ci
Copy link

openshift-ci bot commented Mar 2, 2023

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Mar 2, 2023
@openshift-merge-robot openshift-merge-robot merged commit 795b18c into netobserv:main Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved dependencies Pull requests that update a dependency file lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. qe-approved QE has approved this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants