-
Notifications
You must be signed in to change notification settings - Fork 364
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
Refactor Grafana and ClickHouse deployment configuration instructions #3525
Conversation
docs/network-flow-visibility.md
Outdated
ClickHouse credentials are also specified in [flow-aggregator.yml][flow_aggregator_manifest_yaml] | ||
as a resource of kind: a Secret. Please also make the corresponding changes. | ||
ClickHouse credentials are also specified in `flow-aggregator.yml` as a resource | ||
of name: `clickhouse-secret`. Please also make the corresponding changes. |
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 will be nice to show these lines of code to help users locate where to change these secrets in flow-aggregator.yml
https://github.com/antrea-io/antrea/blob/main/build/yamls/flow-aggregator.yml#L240-L251
docs/network-flow-visibility.md
Outdated
Grafana login credentials are specified in [grafana.yml][grafana_manifest_yaml] as | ||
resource of kind: a Secret. | ||
Grafana login credentials are specified in `flow-visibility.yml` as a resource of | ||
name: `grafana-secret`. |
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.
Same suggestion for grafana-secret
.
Changed grafana Service type from |
Codecov Report
@@ Coverage Diff @@
## main #3525 +/- ##
==========================================
- Coverage 65.46% 63.93% -1.53%
==========================================
Files 278 278
Lines 27771 27825 +54
==========================================
- Hits 18179 17789 -390
- Misses 7666 8124 +458
+ Partials 1926 1912 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -193,4 +193,4 @@ spec: | |||
selector: | |||
app: grafana | |||
sessionAffinity: None | |||
type: LoadBalancer |
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 agree with this change, but I think we should also mention in the documentation that users can change this Service type to LoadBalancer in the downloaded YAML, if their K8s cluster supports LoadBalancer Services
When we provide a Helm chart, this can be parameterized
docs/network-flow-visibility.md
Outdated
@@ -659,8 +659,22 @@ kubectl delete -f https://raw.githubusercontent.com/Altinity/clickhouse-operator | |||
|
|||
##### Credentials Configuration | |||
|
|||
ClickHouse credentials are specified in [clickhouse.yml][clickhouse_manifest_yaml] as | |||
a resource of kind: a Secret. If the username `clickhouse_operator` has changed, please | |||
ClickHouse credentials are specified in `flow-visibility.yml` as a resource of name: |
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.
as a Secret named clickhouse-secret
same below
docs/network-flow-visibility.md
Outdated
- If you have changed the HTTP port, please update the `url` of a resource of name `grafana-datasource-provider` in `flow-visibility.yml`. | ||
|
||
- If you have changed the TCP port, please update the `databaseURL` following [Flow Aggregator Configuration](#configuration-1), | ||
and also update the `jsonData.port` of the `grafana-datasource-provider` resource. |
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.
line wrapping
docs/network-flow-visibility.md
Outdated
for dashboard JSON files export and manual import. | ||
|
||
To generate a deployment manifest with the changes, please follow the following steps: |
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.
maybe emphasize that there are 2 ways of adding dashboards:
- in the UI or by importing a JSON at runtime (I don't know how these are persisted to Grafana, they may be lost in case of a Pod restart?)
- by generating a new manifest and (re)deploying, as described below
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.
yes, the changes will be lost after a Pod restart. They won't be automatically saved to the source.
I have added the 2 ways, could you help check if the wordings make sense to you? Thanks
docs/network-flow-visibility.md
Outdated
specified in [clickhouse.yml][clickhouse_manifest_yaml] as `serviceTemplates`. | ||
To use other ports, please update the following section accordingly. | ||
specified in `flow-visibility.yml` as `serviceTemplates` of a resource of kind: | ||
a `ClickHouseInstallation`. To use other ports, please update the following section. |
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.
kind: a ClickHouseInstallation
-> kind: ClickHouseInstallation
?
docs/network-flow-visibility.md
Outdated
and `databaseURL` in the [Flow Aggregator Configuration](#configuration-1). | ||
This service is used by the Flow Aggregator and Grafana. | ||
|
||
- If you have changed the HTTP port, please update the `url` of a resource of name `grafana-datasource-provider` in `flow-visibility.yml`. |
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.
Maybe we need to keep consistency in whether we need a colon after "a resource of name"?
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 will also join the chorus and ask to change 'resource of name' to 'ConfigMap named' at lines 750 and 753
docs/network-flow-visibility.md
Outdated
|
||
The ClickHouse throughput depends on two factors - the storage size of the ClickHouse | ||
and the time interval between the batch commits to the ClickHouse. Larger storage | ||
size and longer commit interval provide higher throughput. | ||
|
||
Grafana flow collector supports the ClickHouse in-memory deployment with limited | ||
storage size. This is specified in [clickhouse.yml][clickhouse_manifest_yaml]. | ||
The default value of storage size for the ClickHouse server is 8 GiB. Users | ||
storage size. This is specified in `flow-visibility.yml` under the `ClickHouseInstallation` |
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.
line wrap
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 a bit pedant, but maybe you can consider writing:
[...] under the clickhouse
resource of kind ClickHouseInstallation
. The default value [...]
docs/network-flow-visibility.md
Outdated
storage size. This is specified in [clickhouse.yml][clickhouse_manifest_yaml]. | ||
The default value of storage size for the ClickHouse server is 8 GiB. Users | ||
storage size. This is specified in `flow-visibility.yml` under the `ClickHouseInstallation` | ||
resource. The default value of storage size for the ClickHouse server is 8 GiB. Users | ||
can expect a linear growth in the ClickHouse throughput when they enlarge the | ||
storage size. For development or testing environment, you can decrease the storage | ||
size to 2GB. To deploy the ClickHouse with a different storage size, please |
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.
size to 2GB. To deploy the ClickHouse with a different storage size, please | |
size to 2GiB. To deploy the ClickHouse with a different storage size, please |
docs/network-flow-visibility.md
Outdated
@@ -602,6 +602,27 @@ use the checked-in [deployment yaml](/build/yamls/flow-visibility.yml): | |||
kubectl apply -f https://raw.githubusercontent.com/antrea-io/antrea/main/build/yamls/flow-visibility.yml | |||
``` | |||
|
|||
To be noticed, Grafana is exposed as a NodePort Service by default in `flow-visibility.yml`. |
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 think you mean "To be accessible"
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 guess just remove "To be noticed"
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.
exposed through a NodePort Service
docs/network-flow-visibility.md
Outdated
@@ -602,6 +602,27 @@ use the checked-in [deployment yaml](/build/yamls/flow-visibility.yml): | |||
kubectl apply -f https://raw.githubusercontent.com/antrea-io/antrea/main/build/yamls/flow-visibility.yml | |||
``` | |||
|
|||
To be noticed, Grafana is exposed as a NodePort Service by default in `flow-visibility.yml`. | |||
If the given K8s cluster support LoadBalancer Services, Grafana can be changed to a |
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.
s/support/supports
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.
"Grafana can be exposed through a LoadBalancer Service..."?
docs/network-flow-visibility.md
Outdated
a new dashboard, please follow this [doc](https://grafana.com/docs/grafana/latest/dashboards/) | ||
on how to build a dashboard. | ||
|
||
By saving dashboards in the Grafana UI, these changes will be persisted in Grafana |
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.
Maybe "By configuring dashboards"?
docs/network-flow-visibility.md
Outdated
a new dashboard, please follow this [doc](https://grafana.com/docs/grafana/latest/dashboards/) | ||
on how to build a dashboard. | ||
|
||
By saving dashboards in the Grafana UI, these changes will be persisted in Grafana |
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.
in the Grafana database
docs/network-flow-visibility.md
Outdated
|
||
By saving dashboards in the Grafana UI, these changes will be persisted in Grafana | ||
database at runtime, but they will be lost after restarting the Grafana deployment. | ||
To keep those changes for a restart, as the first step, you will need to export the |
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.
To restore those changes after a restart
docs/network-flow-visibility.md
Outdated
By saving dashboards in the Grafana UI, these changes will be persisted in Grafana | ||
database at runtime, but they will be lost after restarting the Grafana deployment. | ||
To keep those changes for a restart, as the first step, you will need to export the | ||
dashboard JSON file following the [doc](https://grafana.com/docs/grafana/latest/dashboards/export-import/), then there are two ways to import the dashboard depending on your needs. |
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.
line wrap
also end the sentence with a :
and not a .
docs/network-flow-visibility.md
Outdated
dashboard JSON file following the [doc](https://grafana.com/docs/grafana/latest/dashboards/export-import/), then there are two ways to import the dashboard depending on your needs. | ||
|
||
- In the running Grafana UI, manually import the dashboard JSON files. | ||
- If you want the changed dashboards to be automatically provisioned to Grafana |
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.
s/provisioned to Grafana/provisioned in Grafana
docs/network-flow-visibility.md
Outdated
- In the running Grafana UI, manually import the dashboard JSON files. | ||
- If you want the changed dashboards to be automatically provisioned to Grafana | ||
like our pre-built dashboards, generate a deployment manifest with the changes by | ||
following the steps as below: |
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.
the steps below
docs/network-flow-visibility.md
Outdated
@@ -602,6 +602,27 @@ use the checked-in [deployment yaml](/build/yamls/flow-visibility.yml): | |||
kubectl apply -f https://raw.githubusercontent.com/antrea-io/antrea/main/build/yamls/flow-visibility.yml | |||
``` | |||
|
|||
To be noticed, Grafana is exposed as a NodePort Service by default in `flow-visibility.yml`. |
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 guess just remove "To be noticed"
docs/network-flow-visibility.md
Outdated
@@ -602,6 +602,27 @@ use the checked-in [deployment yaml](/build/yamls/flow-visibility.yml): | |||
kubectl apply -f https://raw.githubusercontent.com/antrea-io/antrea/main/build/yamls/flow-visibility.yml | |||
``` | |||
|
|||
To be noticed, Grafana is exposed as a NodePort Service by default in `flow-visibility.yml`. | |||
If the given K8s cluster support LoadBalancer Services, Grafana can be changed to a |
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.
"Grafana can be exposed through a LoadBalancer Service..."?
docs/network-flow-visibility.md
Outdated
@@ -602,6 +602,27 @@ use the checked-in [deployment yaml](/build/yamls/flow-visibility.yml): | |||
kubectl apply -f https://raw.githubusercontent.com/antrea-io/antrea/main/build/yamls/flow-visibility.yml | |||
``` | |||
|
|||
To be noticed, Grafana is exposed as a NodePort Service by default in `flow-visibility.yml`. |
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.
exposed through a NodePort Service
docs/network-flow-visibility.md
Outdated
``` | ||
|
||
We recommend changing all the credentials above if you are going to run the Flow | ||
Collector in production. | ||
|
||
##### ClickHouse Configuration | ||
|
||
The ClickHouse database can be accessed through the service `clickhouse-clickhouse`. | ||
The pod exposes HTTP port at 8123 and TCP port at 9000 by default. The ports are |
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.
pod -> Pod
docs/network-flow-visibility.md
Outdated
``` | ||
|
||
We recommend changing all the credentials above if you are going to run the Flow | ||
Collector in production. | ||
|
||
##### ClickHouse Configuration | ||
|
||
The ClickHouse database can be accessed through the service `clickhouse-clickhouse`. |
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.
"service" refers to K8 Service? If so, use "Service" and change other occurrences.
docs/network-flow-visibility.md
Outdated
and `databaseURL` in the [Flow Aggregator Configuration](#configuration-1). | ||
This service is used by the Flow Aggregator and Grafana. | ||
|
||
- If you have changed the HTTP port, please update the `url` of a resource of name `grafana-datasource-provider` in `flow-visibility.yml`. |
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 will also join the chorus and ask to change 'resource of name' to 'ConfigMap named' at lines 750 and 753
docs/network-flow-visibility.md
Outdated
|
||
The ClickHouse throughput depends on two factors - the storage size of the ClickHouse | ||
and the time interval between the batch commits to the ClickHouse. Larger storage | ||
size and longer commit interval provide higher throughput. | ||
|
||
Grafana flow collector supports the ClickHouse in-memory deployment with limited | ||
storage size. This is specified in [clickhouse.yml][clickhouse_manifest_yaml]. | ||
The default value of storage size for the ClickHouse server is 8 GiB. Users | ||
storage size. This is specified in `flow-visibility.yml` under the `ClickHouseInstallation` |
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 a bit pedant, but maybe you can consider writing:
[...] under the clickhouse
resource of kind ClickHouseInstallation
. The default value [...]
Resolved all the comments and squashed the the commits. May I ask for another round of review? @antoninbas @jianjuns @salv-orlando |
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.
Just a nit.
docs/network-flow-visibility.md
Outdated
To use other ports, please update the following section accordingly. | ||
The ClickHouse database can be accessed through the Service `clickhouse-clickhouse`. | ||
The Pod exposes HTTP port at 8123 and TCP port at 9000 by default. The ports are | ||
specified in `flow-visibility.yml` as `serviceTemplates` of a resource of kind: |
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 could not understand this sentence. I apologize if it is that I missed something obvious.
Do you mean "as serviceTemplates
of a ClickHouseInstallation
resource"?
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.
yes, that's what I was trying to say. Rephrased as suggested. Thanks.
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, pending feedback from @jianjuns
Signed-off-by: heanlan <[email protected]> Change grafana Service to NodePort Signed-off-by: heanlan <[email protected]>
/skip-all |
@antoninbas shall we merge it now? |
Signed-off-by: heanlan [email protected]