[stable/kong] Options to install Prometheus plugin with ServiceMonitor #14346

decayofmind · 2019-05-30T21:48:26Z

What this PR does / why we need it:

This PR provides an option to install kong-prometheus plugin as KongPlugin custom resource.
Prometheus-operator ServiceMonitor could be installed as well.

As a side-effect of this PR, Kong CRDs are not being deleted with the release anymore. It prevents from losing Kong declarative configuration while uninstalling Kong.

The proposed change was inspired by Prometheus-operator and Velero charts.

Checklist

DCO signed
Chart Version bumped
Variables are documented in the README.md
Title of the PR starts with chart name (e.g. [stable/chart])

k8s-ci-robot · 2019-05-30T21:48:40Z

Hi @decayofmind. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

decayofmind · 2019-05-30T21:49:21Z

/assign @hbagdi

decayofmind · 2019-06-03T09:44:54Z

/assign @shashiranjan84

decayofmind · 2019-06-11T22:46:37Z

@hbagdi any luck this will be useful?

Signed-off-by: Roman Komkov <[email protected]>

hbagdi · 2019-06-13T22:30:54Z

@decayofmind Sorry for the delayed responses on this. Have been very busy with multiple things flying around. I'll try to get to this soon.

hbagdi · 2019-06-18T19:02:37Z

We will keep this PR around for now.
There is a feature that needs to land in this Helm chart which exposes a metrics port on all Kong deployments, based on a ConfigMap, similar to how we do it here.

Once that get's in, this PR can be simplified quite a bit:

please do not over-complicate the logic of how installation is done for CRDs, let's keep it simple.
While automatically enabling Prometheus plugin when ServiceMonitor is enabled, it is going a bit too far IMHO of providing a magical button for everything. It is okay to document thing and ask users to enable the plugin by themselves, depending on how they are running Kong (with or without Ingress Controller, with or without DB)

hbagdi · 2019-07-03T18:43:28Z

Since #15079 is merged in. We can work on this PR.
Please see my comment above.

Also, it probably might be more accurate to provide serviceMonitor.enabled, rather than metrics.serviceMonitor.enabled as a config value. Please refer to other stable charts in this repository.

decayofmind · 2019-07-08T22:02:40Z

@hbagdi serviceMonitor.enabled is under metrics., because if metrics itself are disabled it has no sense. I can imagine a case, where someone will leave metrics.enabled: false, but serviceMonitor.enabled: true.

But, if you think it's better, I have no objections to change the variable name as you said :)

decayofmind · 2019-07-08T22:09:14Z

please do not over-complicate the logic of how installation is done for CRDs, let's keep it simple.

For installation of CRDs I've used a built-in Helm feature/hook (https://github.com/helm/helm/blob/master/docs/charts_hooks.md#the-available-hooks) to install CRDs before any other template. If something else is over-complicated, let me now, I'll remove it from PR.

While automatically enabling Prometheus plugin when ServiceMonitor is enabled, it is going a bit too far IMHO of providing a magical button for everything. It is okay to document thing and ask users to enable the plugin by themselves, depending on how they are running Kong (with or without Ingress Controller, with or without DB)

A ServiceMonitor will be installed only if metrics.serviceMonitor.create set to true.

stable/kong/templates/servicemonitor.yaml

hbagdi

Please rebase this PR on latest master.

stable/kong/templates/cleanup-crds.yaml

stable/kong/templates/prometheus-plugin.yaml

Signed-off-by: Roman Komkov <[email protected]>

decayofmind · 2019-08-06T19:06:21Z

@hbagdi I've done some changes you requested, could you please take a look at my PR one more time.

hbagdi · 2019-08-15T16:17:03Z

stable/kong/templates/prometheus-plugin.yaml

@@ -0,0 +1,13 @@
+{{- if and .Values.ingressController.enabled .Values.ingressController.metrics.enabled }}


As I said in a review before, let's remove this.
The user can create an additional KongPlugin resource if the need be after the Kong Ingress Controller is installed.

Do note that Kong will expose some basic observability stats even if this plugin is not enabled.

OK, let it be as you're suggesting, I've got the point :)

hbagdi · 2019-08-15T16:17:30Z

stable/kong/templates/deployment.yaml

@@ -251,5 +251,5 @@ spec:
      volumes:
        - name: custom-nginx-template-volume
          configMap:
-            name: {{ .Release.Name }}-kong-default-custom-server-blocks
+            name: {{ template "kong.fullname" . }}-default-custom-server-blocks


Why is this changed?

It may not be the matter of this PR, but in general, all objects in a chart should be addressing {{ template "<chartname.fullname>" }} in its names.

It allows a user to override objects naming for the whole installation, by taking into account .Values.nameOverride and .Values.fullNameOverride (not yet in Kong chart) variables passed to the chart.

hbagdi · 2019-08-15T16:18:05Z

stable/kong/templates/crd-kongconsumer.yaml

@@ -8,6 +8,9 @@ metadata:
    chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
    release: "{{ .Release.Name }}"
    heritage: "{{ .Release.Service }}"
+  annotations:
+    "helm.sh/hook": crd-install
+    "helm.sh/hook-delete-policy": "before-hook-creation"


I believe these changes can go away once KongPlugin resource is removed.

Signed-off-by: Roman Komkov <[email protected]>

hbagdi · 2019-08-16T16:22:04Z

stable/kong/templates/crd-kongplugins.yaml

@@ -8,6 +8,9 @@ metadata:
    chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
    release: "{{ .Release.Name }}"
    heritage: "{{ .Release.Service }}"
+  annotations:
+    "helm.sh/hook": crd-install
+    "helm.sh/hook-delete-policy": "before-hook-creation"


I think these changes to CRDs can be removed since the Prometheus plugin is not being automatically created.

Another problem that I see here:
If multiple Kong Ingress Controllers are being installed one after the other, then this will incorrectly delete the CRD first, and then re-install the CRD. This will lead to deletion of all the custom resources that were present in the cluster before the installation.

@decayofmind Could you address this comment?
Thanks!

hbagdi · 2019-08-16T16:23:29Z

@decayofmind Left one comment and other than that, this PR looks good.
#16293 will be merged before this so please rebase again.

hbagdi · 2019-08-16T16:23:36Z

/ok-to-test

hbagdi · 2019-08-19T17:35:34Z

Please rebase on master. This is real close and almost ready to merge.

hbagdi · 2019-08-19T19:41:44Z

@decayofmind Please address the one outstanding comment and fix the lint failure.

Once that's done, we are good to merge.

Signed-off-by: Roman Komkov <[email protected]>

decayofmind · 2019-08-19T22:18:01Z

@hbagdi It seems like everything is finally fixed

hbagdi · 2019-08-19T22:54:06Z

/lgtm

k8s-ci-robot · 2019-08-19T22:54:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: decayofmind, hbagdi

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:

~~stable/kong/OWNERS~~ [hbagdi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hbagdi · 2019-08-19T22:56:06Z

@decayofmind Thank you for your contribution!

helm#14346) * Options to install Prometheus plugin with ServiceMonitor Signed-off-by: Roman Komkov <[email protected]> * Changes after code review Signed-off-by: Roman Komkov <[email protected]> * Removed ingressController and plugin dependency. Signed-off-by: Roman Komkov <[email protected]> * Version bumped Signed-off-by: Roman Komkov <[email protected]> * Remove helm-hook annotations Signed-off-by: Roman Komkov <[email protected]> * Remove trailing whitespace Signed-off-by: Roman Komkov <[email protected]>

helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label May 30, 2019

k8s-ci-robot requested review from hbagdi and shashiranjan84 May 30, 2019 21:48

helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 30, 2019

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 30, 2019

k8s-ci-robot assigned hbagdi May 30, 2019

k8s-ci-robot assigned shashiranjan84 Jun 3, 2019

Options to install Prometheus plugin with ServiceMonitor

72a7347

Signed-off-by: Roman Komkov <[email protected]>

hbagdi reviewed Jul 29, 2019

View reviewed changes

stable/kong/templates/servicemonitor.yaml Outdated Show resolved Hide resolved

hbagdi suggested changes Jul 29, 2019

View reviewed changes

stable/kong/templates/cleanup-crds.yaml Outdated Show resolved Hide resolved

stable/kong/templates/prometheus-plugin.yaml Outdated Show resolved Hide resolved

decayofmind and others added 2 commits July 30, 2019 19:37

Merge branch 'master' into kong/prometheus-integration

c0ef24e

Changes after code review

a40837d

Signed-off-by: Roman Komkov <[email protected]>

hbagdi reviewed Aug 15, 2019

View reviewed changes

decayofmind and others added 2 commits August 16, 2019 13:58

Merge branch 'master' into kong/prometheus-integration

0e989c6

Removed ingressController and plugin dependency.

0e99b73

Signed-off-by: Roman Komkov <[email protected]>

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 16, 2019

Version bumped

a45e32f

Signed-off-by: Roman Komkov <[email protected]>

hbagdi reviewed Aug 16, 2019

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 16, 2019

Merge branch 'master' into kong/prometheus-integration

8b48986

Roman Komkov and others added 3 commits August 19, 2019 23:58

Remove helm-hook annotations

06afe5e

Signed-off-by: Roman Komkov <[email protected]>

Merge branch 'master' into kong/prometheus-integration

ff2ea39

Remove trailing whitespace

477c8d5

Signed-off-by: Roman Komkov <[email protected]>

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2019

k8s-ci-robot merged commit 75c2526 into helm:master Aug 19, 2019

decayofmind deleted the kong/prometheus-integration branch August 20, 2019 00:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stable/kong] Options to install Prometheus plugin with ServiceMonitor #14346

[stable/kong] Options to install Prometheus plugin with ServiceMonitor #14346

decayofmind commented May 30, 2019

k8s-ci-robot commented May 30, 2019

decayofmind commented May 30, 2019

decayofmind commented Jun 3, 2019

decayofmind commented Jun 11, 2019

hbagdi commented Jun 13, 2019

hbagdi commented Jun 18, 2019

hbagdi commented Jul 3, 2019

decayofmind commented Jul 8, 2019

decayofmind commented Jul 8, 2019

hbagdi left a comment

decayofmind commented Aug 6, 2019

hbagdi Aug 15, 2019

hbagdi Aug 15, 2019

decayofmind Aug 16, 2019

hbagdi Aug 15, 2019

decayofmind Aug 16, 2019

hbagdi Aug 15, 2019

hbagdi Aug 16, 2019

hbagdi Aug 19, 2019

hbagdi commented Aug 16, 2019

hbagdi commented Aug 16, 2019

hbagdi commented Aug 19, 2019

hbagdi commented Aug 19, 2019

decayofmind commented Aug 19, 2019

hbagdi commented Aug 19, 2019

k8s-ci-robot commented Aug 19, 2019

hbagdi commented Aug 19, 2019

		@@ -0,0 +1,13 @@
		{{- if and .Values.ingressController.enabled .Values.ingressController.metrics.enabled }}

[stable/kong] Options to install Prometheus plugin with ServiceMonitor #14346

[stable/kong] Options to install Prometheus plugin with ServiceMonitor #14346

Conversation

decayofmind commented May 30, 2019

What this PR does / why we need it:

Checklist

k8s-ci-robot commented May 30, 2019

decayofmind commented May 30, 2019

decayofmind commented Jun 3, 2019

decayofmind commented Jun 11, 2019

hbagdi commented Jun 13, 2019

hbagdi commented Jun 18, 2019

hbagdi commented Jul 3, 2019

decayofmind commented Jul 8, 2019

decayofmind commented Jul 8, 2019

hbagdi left a comment

Choose a reason for hiding this comment

decayofmind commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Aug 16, 2019

hbagdi commented Aug 16, 2019

hbagdi commented Aug 19, 2019

hbagdi commented Aug 19, 2019

decayofmind commented Aug 19, 2019

hbagdi commented Aug 19, 2019

k8s-ci-robot commented Aug 19, 2019

hbagdi commented Aug 19, 2019