use default k8s cluster plugin from rabbitmq #4591

vtuson · 2018-04-02T10:37:47Z

What this PR does / why we need it:
This PR modifies the existing chart to be able to be horizontally scaled by adding additional replicas of rabbitmq. It uses the default http://www.rabbitmq.com/cluster-formation.html#peer-discovery-k8s which is prepackage in 3.7+ versions of rabbitmq.

In order to achieve this I have moved from a deployment to a statefulselt for a stable network configuration.

This PR also provides rbac Service account, role and role-binding since the rabbitmq plugin configures the cluster by monitoring endpoint on a given service.

Additionally this PR provides an ingress resource (optional) for the management interface of rabbitmq.

I have also provided an additional values yaml file for high availability configuration.

This PR adds additional configurability for the chart, including the option of using a private registry, control over probe settings and a few other things.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
It is an improvement pr - addresses some of the concern in this issue #3075

Special notes for your reviewer:

@tompizmor @sameersbn @prydonius followed the discuss approach to provide both default setup and also production set up.

vtuson · 2018-04-02T10:39:13Z

/assign @tompizmor

same than before but with a matching email

tompizmor · 2018-04-02T16:06:48Z

stable/rabbitmq/values-production.yaml

+  pullPolicy: IfNotPresent
+
+  ## set to true if you would like to see extra information on logs
+  ## it turns BASH and NAMI debuggin in minideb


typo: debuggin -> debugging

thanks, will fix

tompizmor · 2018-04-02T16:07:16Z

stable/rabbitmq/values.yaml

+  tag: 3.7.4-r1
+
+  ## set to true if you would like to see extra information on logs
+  ## it turns BASH and NAMI debuggin in minideb


tompizmor · 2018-04-02T16:07:50Z

stable/rabbitmq/values.yaml

-    cpu: 100m
+resources: {}
+
+## Replica count, set to 3 to provide a default available cluster


Comment and below value mismatch

oops, copy paste :)

tompizmor · 2018-04-02T16:09:06Z

stable/rabbitmq/templates/configuration.yaml

+      [rabbitmq_management,rabbitmq_peer_discovery_k8s].
+
+  rabbitmq.conf: |
+      ## Clustering


I think this configuration should come from the values.yaml.
Example: https://github.com/kubernetes/charts/blob/master/stable/mariadb/templates/configmap.yaml

ok, I have moved the main body of both config files, but left the user and password configuration as that is configure via other variables. you cant reference other variables via values.yaml

tompizmor · 2018-04-02T16:12:13Z

stable/rabbitmq/templates/ingress.yaml

+        - path: {{ default "/" .path }}
+          backend:
+            serviceName: {{ template "rabbitmq.fullname" . }}
+            servicePort: {{ default "15672" .Values.rabbitmq.managerPort }}


Use "default" in the template is not recommended. We should rely on the default value set in the values.yaml file

good to know , thanks

tompizmor · 2018-04-02T16:14:25Z

stable/rabbitmq/templates/ingress.yaml

+    - secretName: {{  .Values.ingress.tlsSecret }}
+    {{- end }}
+{{- end }}
+---


I would say you don't need this dashes. I have seen it in some other charts but they were creating several ingress resources. For example https://github.com/kubernetes/charts/blob/master/stable/wordpress/templates/ingress.yaml

tompizmor · 2018-04-02T16:15:25Z

stable/rabbitmq/templates/statefulset.yaml

+      {{- if .Values.image.pullSecrets }}
+      imagePullSecrets:
+      {{- range .Values.image.pullSecrets }}
+        - name: {{.}}


{{.}} -> {{ . }}

coreypobrien · 2018-04-02T19:34:43Z

A lot of this looks like the beginning of merging this into the rabbitmq-ha chart maybe? The rabbitmq-ha chart already is using a statefulset. What would this PR accomplish that isn't already handled by the rabbitmq-ha chart?

vtuson · 2018-04-03T10:38:04Z

@coreypobrien I am not sure why rabbitmq-ha was created to start with, seems like it should have been itself a PR to this chart. My prefer option would be to merge this PR once the comments are addressed and see how we want to proceed.

tompizmor · 2018-04-03T13:28:50Z

stable/rabbitmq/templates/configuration.yaml

@@ -0,0 +1,14 @@
+apiVersion: v1


you are missing labels in this file:

labels: app: {{ template "rabbitmq.name" . }} chart: {{ template "rabbitmq.chart". }} release: "{{ .Release.Name }}" heritage: "{{ .Release.Service }}"

I noticed that PVC has this also missing so I fixed both

tompizmor · 2018-04-03T13:29:56Z

stable/rabbitmq/templates/ingress.yaml

+metadata:
+  name: "{{ template "rabbitmq.fullname" . }}"
+  labels:
+    app: {{ template "rabbitmq.fullname" . }}


As stated in the best-practices the app labels should use the {{ template "rabbitmq.name" . }} and not {{ template "rabbitmq.fullname" . }}

This change in the labels applies to all files

tompizmor · 2018-04-03T13:32:46Z

stable/rabbitmq/templates/ingress.yaml

+  name: "{{ template "rabbitmq.fullname" . }}"
+  labels:
+    app: {{ template "rabbitmq.fullname" . }}
+    chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"


instead of using "{{ .Chart.Name }}-{{ .Chart.Version }}" you should create the following template and use it here:

chart: {{ template "rabbitmq.chart" . }}

Template:

{{/* Create chart name and version as used by the chart label. */}} {{- define "rabbitmq.chart" -}} {{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" -}} {{- end -}}

This change in the labels applies to all files

mmm,seems over kill - is there a guideline about when to do this?

tompizmor · 2018-04-03T13:36:28Z

stable/rabbitmq/templates/statefulset.yaml

+        - name: epmd
+          containerPort: 4369
+        - name: amqp
+          containerPort: {{ default "5672" .Values.rabbitmq.nodePort }}


there are several default values in this file. can you remove all of them?

tompizmor · 2018-04-03T13:36:55Z

stable/rabbitmq/templates/svc.yaml

@@ -14,13 +14,13 @@ spec:
    port: 4369
    targetPort: epmd
  - name: amqp
-    port: {{ default "5672" .Values.rabbitmqNodePort }}
+    port: {{ default "5672" .Values.rabbitmq.nodePort }}


more defaults

in my defence, they were there before :) - changed

tompizmor · 2018-04-03T13:38:34Z

stable/rabbitmq/values-production.yaml

+image:
+  registry: docker.io
+  repository: bitnami/rabbitmq
+  tag: 3.7.4-r1


can you update this tag to 3.7.4? The tag already exists.

ok, I didnt know if that was the case. will do.

tompizmor · 2018-04-03T14:24:20Z

/ok-to-test

tompizmor · 2018-04-03T14:24:44Z

/lgtm

… when starting the pod

tompizmor · 2018-04-03T14:56:54Z

/lgtm

k8s-ci-robot · 2018-04-03T14:57:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tompizmor, vtuson

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/rabbitmq/OWNERS~~ [tompizmor]

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

macropin · 2018-04-04T08:47:03Z

Why is this madness being merged into stable?

redbaron · 2018-04-04T19:25:48Z

Seems that new rabbitmq.diskFreeLimit is not used anywhere and totally ignored

* use default k8s cluster plugin from rabbitmq * changes to reflect pr review comments * remove trailing spaces * futher changes from PR review * doing sed in the copy as it fails in gce due to filesystem permisions when starting the pod

prydonius · 2018-04-10T12:49:15Z

@vtuson @tompizmor for future reference, this should have been a major version bump.

QuingKhaos · 2018-04-17T11:00:09Z

stable/rabbitmq/templates/statefulset.yaml

@@ -0,0 +1,150 @@
+apiVersion: apps/v1


Works only with Kubernetes 1.9+, before this was a beta API 😭

#4909 should fix it

* use default k8s cluster plugin from rabbitmq * changes to reflect pr review comments * remove trailing spaces * futher changes from PR review * doing sed in the copy as it fails in gce due to filesystem permisions when starting the pod

* use default k8s cluster plugin from rabbitmq * changes to reflect pr review comments * remove trailing spaces * futher changes from PR review * doing sed in the copy as it fails in gce due to filesystem permisions when starting the pod Signed-off-by: voron <[email protected]>

* use default k8s cluster plugin from rabbitmq * changes to reflect pr review comments * remove trailing spaces * futher changes from PR review * doing sed in the copy as it fails in gce due to filesystem permisions when starting the pod

vtuson added 2 commits April 2, 2018 12:34

use default k8s cluster plugin from rabbitmq

6ceff32

Merge remote-tracking branch 'upstream/master' into rabbitmq_clean

1dbd127

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 2, 2018

k8s-ci-robot requested review from sameersbn and tompizmor April 2, 2018 10:37

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 2, 2018

k8s-ci-robot assigned tompizmor Apr 2, 2018

tompizmor suggested changes Apr 2, 2018

View reviewed changes

timstoop mentioned this pull request Apr 2, 2018

[stable/rabbitmq-ha] Clustering fixes #4610

Merged

vtuson added 3 commits April 3, 2018 14:13

changes to reflect pr review comments

963ad63

Merge remote-tracking branch 'upstream/master' into rabbitmq_clean

b383269

remove trailing spaces

7e06cf9

tompizmor suggested changes Apr 3, 2018

View reviewed changes

vtuson added 2 commits April 3, 2018 16:04

futher changes from PR review

4549ff8

Merge remote-tracking branch 'upstream/master' into rabbitmq_clean

d6103cf

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 3, 2018

k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 3, 2018

doing sed in the copy as it fails in gce due to filesystem permisions…

6e30d64

… when starting the pod

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2018

k8s-ci-robot merged commit d13f1f1 into helm:master Apr 3, 2018

coreypobrien mentioned this pull request Apr 6, 2018

[stable/rabbitmq] [stable/rabbitmq-ha] Merge charts discussion #4748

Closed

QuingKhaos reviewed Apr 17, 2018

View reviewed changes

This was referenced May 2, 2018

RabbitMQ high CPU usage on idle VM #3855

Closed

[stable/rabbitmq] no longer works on k8s 1.5--1.7 because apps/v1beta2 StatefulSet is not available #5388

Closed

[stable/rabbitmq] add back rabbitmq.vhost value #5394

Closed

tompizmor mentioned this pull request May 28, 2018

[stable/mongodb] - Refactor MongoDB chart to support replica set configurations #5578

Merged

mattfarina unassigned tompizmor Jul 19, 2018

This was referenced Oct 30, 2018

[stable/rabbitmq] rabbitmq.diskFreeLimit ignored #8901

Closed

[stable/rabbitmq] re-sync values-production.yaml with values.yaml #8929

Merged

thomas-riccardi mentioned this pull request Apr 16, 2019

[stable/rabbitmq] feature request: automatic configuration for schedulers threads using resources requests #13050

Closed

use default k8s cluster plugin from rabbitmq #4591

use default k8s cluster plugin from rabbitmq #4591

Conversation

vtuson commented Apr 2, 2018

vtuson commented Apr 2, 2018

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

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

coreypobrien commented Apr 2, 2018

vtuson commented Apr 3, 2018

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

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

tompizmor commented Apr 3, 2018

tompizmor commented Apr 3, 2018

tompizmor commented Apr 3, 2018

k8s-ci-robot commented Apr 3, 2018

macropin commented Apr 4, 2018

redbaron commented Apr 4, 2018

prydonius commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment