helm: Add example dashboards as chart options #23794

jcpunk · 2023-02-15T20:53:08Z

With this patch, helm users can enable reporting on their clusters with example grafana dashboards.

These dashboards were copied from examples/kubernetes/addons/prometheus/files/grafana-dashboards/

Fixes: #21921

Integration of sample dashboards with Helm chart

gandro

Thanks for the PR! I'll ping some folks more familiar with the Grafana integration to also take a look at this.

Since you copied the Dashboards from examples - I think it would make sense to remove them there and update the docs to the new Helm chart options instead. Otherwise we would have to maintain two copies of the dashboards in the tree.

lambdanis

Since you copied the Dashboards from examples - I think it would make sense to remove them there and update the docs to the new Helm chart options instead. Otherwise we would have to maintain two copies of the dashboards in the tree.

+1, I don't how files in examples are used (are there any scripts or docs depending on them?), but maintaining two copies of each dashboard should be avoidable.

I'm not sure what's the quality of these "example" dashboards, @tommyp1ckles might have more input on that. If a dashboard is included in a Helm chart, I would expect it to be well-thought with prod usage in mind, not just a dump of metrics. I know such dashboards for Cilium exist, but I'm not sure if that's the case for these examples.

Also a housekeeping note: I left a longer comment in the referenced issue (#21921), but this PR seems to be more directly relevant for another one: #20354

install/kubernetes/cilium/values.yaml

install/kubernetes/cilium/templates/cilium-agent/dashboards-configmap.yaml

lambdanis

Thanks! The change looks good to me.

The quality of these dashboards is an open question, but if better dashboards exist they can be commited separately. Same about the dashboards duplication (it seems that hubble-l7-http-metrics-by-workload.json is already in both places, so this PR is not introducing duplication; but it should be resolved, especially if the examples are actually used my some scripts or docs).

jcpunk · 2023-02-16T16:15:38Z

Would dropping soft links in the examples directory work for now? That would preserve the paths for evaluation by folks who use them.

PR updated with links

lambdanis · 2023-02-16T17:47:05Z

@jcpunk One more thing, when you update the Helm values you need to update the Helm values docs too:

make -C Documentation update-helm-values
make -C install/kubernetes docs

I think that would be it, can you commit the generated docs changes?

I hope switching to symlinks in examples won't break anything, either way this is the right direction IMO.

jcpunk · 2023-02-16T17:55:21Z

.... I've got to be doing something wrong here:

[riehecky@leibniz cilium]$ make -C Documentation update-helm-values
make: Entering directory '/tmp/foo/cilium/Documentation'
docker container run --rm --workdir /src/Documentation --volume /tmp/foo/cilium/Documentation/..:/src --env READTHEDOCS_VERSION= --env SKIP_LINT= --user "1000:1000" "quay.io/cilium/helm-toolbox:"v1.0.1"@sha256:"a1483fc036b15c290326ba7ed36b8f0b714e185913b72a6074c04225d74e034c"" helm-docs -d -c /src/install/kubernetes -t /src/Documentation/helm-values.tmp.tmpl > helm-values.tmp
Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
time="2023-02-16T17:54:44Z" level=fatal msg="error finding chart directories: open /src/install/kubernetes: permission denied"
make: *** [Makefile:108: helm-values.rst] Error 1
make: Leaving directory '/tmp/foo/cilium/Documentation'
[riehecky@leibniz cilium]$ make -C install/kubernetes doc
make: Entering directory '/tmp/foo/cilium/install/kubernetes'
make: *** No rule to make target 'doc'.  Stop.
make: Leaving directory '/tmp/foo/cilium/install/kubernetes'

lambdanis · 2023-02-16T18:03:31Z

.... I've got to be doing something wrong here:

nope, my typo, docs with an s

jcpunk · 2023-02-16T18:05:39Z

Any guesses on:

time="2023-02-16T18:05:07Z" level=fatal msg="error finding chart directories: open /src/install/kubernetes: permission denied"

I've given you merge rights to my repo if my local system is just too wonky....

lambdanis · 2023-02-16T18:17:45Z

@jcpunk I don't know, make dev-doctor might have some hints or someone else will know for sure.

jcpunk · 2023-02-16T20:57:45Z

I think I got it to run... the files changed at least.

lambdanis

Looks good to me now.
There are some more dashboards in a Grafana mixin btw: https://github.com/grafana/jsonnet-libs/tree/master/cilium-enterprise-mixin/dashboards The dashboards here should be updated too at some point, but not necessarily in this PR.

lambdanis · 2023-02-16T21:43:11Z

/test

gandro

Thanks! I've left some more feedback

install/kubernetes/cilium/values.yaml.tmpl

install/kubernetes/cilium/files/hubble/dashboards/hubble-dashboard.json

lambdanis · 2023-02-20T13:02:37Z

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathVerifier Runs the kernel verifier against Cilium's BPF datapath

Failure Output

FAIL: terminating containers are not deleted after timeout

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

lambdanis · 2023-02-27T14:19:23Z

/test

gandro

Thanks! One last typo

install/kubernetes/cilium/values.yaml.tmpl

jcpunk · 2023-02-27T16:08:57Z

Updated and rebased

gandro

Thanks! Some more documentation nits, and then this should be good to go I think!

install/kubernetes/cilium/values.yaml

jcpunk · 2023-02-28T15:02:52Z

Done

gandro

Thank you!

gandro · 2023-02-28T15:43:50Z

/test

gandro · 2023-03-01T11:22:42Z

Seems like there are still some issues with the generated code:

https://github.com/cilium/cilium/actions/runs/4294347264/jobs/7484211056

+++ /home/runner/work/cilium/cilium/install/kubernetes/cilium/values.yaml	2023-02-28 15:44:31.598361649 +0000
@@ -1696,7 +1696,7 @@
   enabled: false
   label: grafana_dashboard
   namespace: ~
-  labelValue: "1"
+  labelValue: "1" 
   annotations: {}
 
 # -- Configure Istio proxy options.
@@ -2057,7 +2057,6 @@
       relabelings: ~
       # -- Metrics relabeling configs for the ServiceMonitor cilium-operator
       metricRelabelings: ~
-
   # -- Grafana dashboards for cilium-operator
   # grafana can import dashboards based on the label and value
   # ref: https://github.com/grafana/helm-charts/tree/main/charts/grafana#sidecar-for-dashboards
@@ -2065,7 +2064,7 @@
     enabled: false
     label: grafana_dashboard
     namespace: ~
-    labelValue: "1" 
+    labelValue: "1"
     annotations: {}
 
   # -- Skip CRDs creation for cilium-operator

make: *** [Makefile:49: check-values-yaml] Error 1
error: cilium/values.yaml has been modified
Make sure to apply your changes to cilium/values.yaml.tmpl and run 'make -C install/kubernetes cilium/values.yaml' to regenerate.

jcpunk · 2023-03-02T15:39:45Z

I think I've got it sorted...

gandro · 2023-03-02T16:24:11Z

/test

gandro · 2023-03-02T16:30:34Z

The issue seems to persist unfortunately.

Locally, I can fix it via rm install/kubernetes/cilium/values.yaml; make -C install/kubernetes cilium/values.yaml

With this patch, helm users can enable reporting on their clusters with example grafana dashboards. Fixes: #21921 Fixes: #20354 Signed-off-by: Pat Riehecky <riehecky@fnal.gov>

jcpunk · 2023-03-02T16:54:25Z

That is strange... I've run that by hand and hopefully that helps.

gandro · 2023-03-02T17:03:06Z

Looks good now!

gandro · 2023-03-02T17:03:13Z

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sAgentPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath

Failure Output

FAIL: connectivity-check pods are not ready after timeout

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

gandro · 2023-03-06T16:26:48Z

@tommyp1ckles Ping for review

tommyp1ckles · 2023-03-09T05:49:41Z

@lambdanis

I'm not sure what's the quality of these "example" dashboards, @tommyp1ckles might have more input on that. If a dashboard is included in a Helm chart, I would expect it to be well-thought with prod usage in mind, not just a dump of metrics. I know such dashboards for Cilium exist, but I'm not sure if that's the case for these examples.

They appear similar to the other Isovalent dashboards here: https://grafana.com/orgs/isovalent/dashboards. Not 100% sure what the relationship is.

Overall they're ok, for basic use cases but some obvious flaws jump out (same issues as the linked ones):

Grouping by pods scales poorly for large clusters, or setups with multiple clusters.
More things should be parameterized, such as cluster, Agent Pod, etc...
Lots of arbitrary looking interval values, should probably be dynamic or $__interval?

Anyway, changes LGTM 🙏

gandro · 2023-03-09T09:39:45Z

I'm marking this ready to merge. There are a newer test suites which weren't part of the branch when this PR was opened, so those can be ignored. The other test failures are unrelated:

k8s-1.16-kernel-4.19 https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/296/ hit known flake CI: K8sAgentPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath #22749
IntegrationTests https://github.com/cilium/cilium/actions/runs/4316321020/jobs/7532127000 hit go version mismatch which was fixed by go.mod, golangci-lint: update base Go version to 1.20 #24113 (but not included in this branch)
ConformanceIngress https://github.com/cilium/cilium/actions/runs/4316321018/jobs/7532047424 hit a new? but unrelated flake here CI: ConformanceIngress - Run Sanity check (internal to NodePort) - Immediate connect fail for ::1 and 127.0.0.1 #24257

sathieu · 2023-04-04T09:23:36Z

@jcpunk I think there is a bug in this PR.

There is {{- $files := .Files.Glob "files/cilium-dashboards/*.json" }}:

cilium/install/kubernetes/cilium/templates/cilium-agent/dashboards-configmap.yaml

Line 2 in 89f9525

While the dashboard are in files/cilium-agent/dashboards.

Could you fix this?

jcpunk · 2023-04-04T14:46:57Z

#24733 opened

jcpunk requested review from a team as code owners February 15, 2023 20:53

jcpunk requested review from tommyp1ckles and gandro February 15, 2023 20:53

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 15, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 15, 2023

gandro reviewed Feb 16, 2023

View reviewed changes

gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/metrics Impacts statistics / metrics gathering, eg via Prometheus. area/helm Impacts helm charts and user deployment experience labels Feb 16, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 16, 2023

gandro requested a review from lambdanis February 16, 2023 12:49

lambdanis reviewed Feb 16, 2023

View reviewed changes

install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved

lambdanis reviewed Feb 16, 2023

View reviewed changes

install/kubernetes/cilium/templates/cilium-agent/dashboards-configmap.yaml Outdated Show resolved Hide resolved

lambdanis reviewed Feb 16, 2023

View reviewed changes

lambdanis approved these changes Feb 16, 2023

View reviewed changes

gandro reviewed Feb 17, 2023

View reviewed changes

install/kubernetes/cilium/values.yaml.tmpl Show resolved Hide resolved

install/kubernetes/cilium/files/hubble/dashboards/hubble-dashboard.json Show resolved Hide resolved

gandro reviewed Feb 27, 2023

View reviewed changes

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved

gandro reviewed Feb 28, 2023

View reviewed changes

install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved

install/kubernetes/cilium/values.yaml Show resolved Hide resolved

gandro reviewed Feb 28, 2023

View reviewed changes

install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved

gandro approved these changes Feb 28, 2023

View reviewed changes

helm: Add example dashboards as chart options

2742edf

With this patch, helm users can enable reporting on their clusters with example grafana dashboards. Fixes: #21921 Fixes: #20354 Signed-off-by: Pat Riehecky <riehecky@fnal.gov>

tommyp1ckles approved these changes Mar 9, 2023

View reviewed changes

gandro mentioned this pull request Mar 9, 2023

CI: ConformanceIngress - Run Sanity check (internal to NodePort) - Immediate connect fail for ::1 and 127.0.0.1 #24257

Closed

gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 9, 2023

jrajahalme merged commit bdbb129 into cilium:master Mar 13, 2023

jcpunk deleted the dashboards branch March 13, 2023 13:29

helm: Add example dashboards as chart options #23794

helm: Add example dashboards as chart options #23794

Conversation

jcpunk commented Feb 15, 2023

gandro left a comment

Choose a reason for hiding this comment

lambdanis left a comment • edited

Choose a reason for hiding this comment

lambdanis left a comment

Choose a reason for hiding this comment

jcpunk commented Feb 16, 2023 • edited

lambdanis commented Feb 16, 2023 • edited

jcpunk commented Feb 16, 2023

lambdanis commented Feb 16, 2023

jcpunk commented Feb 16, 2023 • edited

lambdanis commented Feb 16, 2023 • edited

jcpunk commented Feb 16, 2023

lambdanis left a comment

Choose a reason for hiding this comment

lambdanis commented Feb 16, 2023

gandro left a comment

Choose a reason for hiding this comment

lambdanis commented Feb 20, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

lambdanis commented Feb 27, 2023

gandro left a comment

Choose a reason for hiding this comment

jcpunk commented Feb 27, 2023

gandro left a comment

Choose a reason for hiding this comment

jcpunk commented Feb 28, 2023

gandro left a comment

Choose a reason for hiding this comment

gandro commented Feb 28, 2023

gandro commented Mar 1, 2023

jcpunk commented Mar 2, 2023

gandro commented Mar 2, 2023

gandro commented Mar 2, 2023

jcpunk commented Mar 2, 2023

gandro commented Mar 2, 2023

gandro commented Mar 2, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

gandro commented Mar 6, 2023

tommyp1ckles commented Mar 9, 2023 • edited

gandro commented Mar 9, 2023

sathieu commented Apr 4, 2023

jcpunk commented Apr 4, 2023

lambdanis left a comment •

edited

jcpunk commented Feb 16, 2023 •

edited

lambdanis commented Feb 16, 2023 •

edited

jcpunk commented Feb 16, 2023 •

edited

lambdanis commented Feb 16, 2023 •

edited

lambdanis commented Feb 20, 2023 •

edited by maintainer-s-little-helper bot

gandro commented Mar 2, 2023 •

edited by maintainer-s-little-helper bot

tommyp1ckles commented Mar 9, 2023 •

edited