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-256: Kafka config #107

Merged

Conversation

OlivierCazade
Copy link
Contributor

In this PR:

  • configuration of FLP ingester and transformer when kafka is enabled
  • Makefile targets to deploy kafka
  • example of configuration

What you need to do to deploy NOO with Kafka:

  • deploy kafka using the Makefile
  • enable Kafka in the FlowCollector example, other parameters default values should be fine.

Comment on lines +29 to +31
kubectl create -f "https://strimzi.io/install/latest?namespace="$(NAMESPACE) -n $(NAMESPACE)
kubectl create -f "https://raw.githubusercontent.com/netobserv/documents/main/examples/kafka-cluster.yaml" -n $(NAMESPACE)
kubectl create -f "https://raw.githubusercontent.com/netobserv/documents/main/examples/kafka-topic.yaml" -n $(NAMESPACE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we mix both strimzi.io / netobserv/document sources, we should fix the version here instead of using latest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strimzi only provide this endpoint for its latest version.

The other option is to track this file in one of our repository, but this file has to be changed depending of the targeted namespace.

Since we only use this for now as quick setup for development and since this file is quite big (13k lines) would you be fine to keep using latest for know? If at one point we experience issues it will still be possible to freeze the version there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well if there is no easy solution let's keep this as is. Thanks

controllers/flowlogspipeline/flp_objects.go Outdated Show resolved Hide resolved
kubectl create -f "https://raw.githubusercontent.com/netobserv/documents/main/examples/kafka-topic.yaml" -n $(NAMESPACE)

.PHONY: undeploy-kafka
undeploy-kafka: ## Undeploy loki.
Copy link
Contributor

Choose a reason for hiding this comment

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

change comment from Undeploy loki to undeploy-kafka

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

Copy link
Contributor

@eranra eranra left a comment

Choose a reason for hiding this comment

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

I miss somewhere in the Makefile usage of deploy-kafka and undeploy-kafka ???

@OlivierCazade
Copy link
Contributor Author

I miss somewhere in the Makefile usage of deploy-kafka and undeploy-kafka ???

Kafka for now is not enabled by default, since kafka cluster create many differents pods I did not add deploy-kafka to the deploy-all command.

Do you think we should deploy kafka in the deploy-all command?

kafka:
enable: false
address: "kafka-cluster-kafka-bootstrap"
topic: "flp-topic"
Copy link
Member

Choose a reason for hiding this comment

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

as this CR will define the default values in OLM, I'd use a topic name that better describes what it is for. Maybe "netflows" ?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, network-flows would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

@jotak
Copy link
Member

jotak commented May 30, 2022

Do you think we should deploy kafka in the deploy-all command?

I think we can continue to keep it turned off by default, at least for the moment

@jotak
Copy link
Member

jotak commented May 30, 2022

Looks good at first sight 😍

Capture d’écran de 2022-05-30 10-22-07

@jotak
Copy link
Member

jotak commented May 30, 2022

LGTM (code review + tested), except we should rename the default topic.

@OlivierCazade also, the doc should be updated: in README and in the clusterserviceversion file. You can create another story for doc if you prefer.

@OlivierCazade
Copy link
Contributor Author

the doc should be updated: in README and in the clusterserviceversion file. You can create another story for doc if you prefer.

New story created:
https://issues.redhat.com/browse/NETOBSERV-378

Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

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

/lgtm

@OlivierCazade
Copy link
Contributor Author

/approve

@openshift-ci
Copy link

openshift-ci bot commented May 31, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: OlivierCazade

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-merge-robot openshift-merge-robot merged commit 0994ae6 into netobserv:main May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants