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

add rabbitmq support into helm chart #54

Merged
merged 16 commits into from
Nov 7, 2023
Merged

add rabbitmq support into helm chart #54

merged 16 commits into from
Nov 7, 2023

Conversation

edevosc2c
Copy link
Member

@edevosc2c edevosc2c commented Sep 19, 2023

Description

Since georchestra/improvement-proposals#6 was accepted and the support for rabbitmq was recently merged into georchestra/docker: https://github.com/georchestra/docker.

This pull request adds the support for rabbitmq. It uses bitnami rabbitmq well maintained helm chart, same author of the helm chart that we are using for postgresql.

Almost all the logic was copied from the postgresql component, same ability to set a custom secret and same structure for the YAML.

Technical notes about the changes in this PR

  • This upgrades the postgresql helm chart version, I had to update it because there was a conflict with the rabbitmq helm chart.
  • This separate each helm helpers for more readability.
  • This add the support of rabbitmq through helm chart of bitnami organization.

Notes - to discuss

The issue, to discuss, is that the georchestra gateway hasn't been set by default, so rabbitmq is not mandatory yet for georchestra.

This means one can turn off rabbitmq (rabbitmq.builtin: false) but there will still be a need to fill something in the rabbitmq.auth section because the helm chart consider that if you turn off the builtin rabbitmq then you must provide an external rabbitmq server then.

One solution to this is to add a new parameter called rabbitmq.enabled which if "false" stop provisioning anything related to rabbitmq into the chart. But the name could confuse some people with the parameter builtin: true

I'm not sure how to tackle this issue. @jeanmi151 @jeanpommier any ideas?

@edevosc2c
Copy link
Member Author

One solution to this is to add a new parameter called rabbitmq.enabled which if "false" stop provisioning anything related to rabbitmq into the chart. But the name could confuse some people with the parameter builtin: true

@jeanmi151 @jeanpommier what do you think of this solution for the issue mentioned above?

@jeanpommier
Copy link
Member

AFAIK, Gateway won't be optional for long, I believe SP is expected for deprecation in Spring 2024.

So, we just need a short-term solution. I would say that the way you handle it doesn't really matter as long as

@edevosc2c edevosc2c added the enhancement New feature or request label Nov 6, 2023
@jeanpommier
Copy link
Member

  • works with SP enabled, gateway disabled and rabbitmq disabled
  • works with SP enabled, gateway disabled and rabbitmq enabled
    Though it is apparently not supported to perform an upgrade (from rabbitmq disabled to rabbitmq enabled): getting
Error: UPGRADE FAILED: template: georchestra/charts/rabbitmq/templates/statefulset.yaml:39:28: executing "georchestra/charts/rabbitmq/templates/statefulset.yaml" at <include (print $.Template.BasePath "/secrets.yaml") .>: error calling include: template: georchestra/charts/rabbitmq/templates/secrets.yaml:27:29: executing "georchestra/charts/rabbitmq/templates/secrets.yaml" at <include "common.secrets.passwords.manage" (dict "secret" (include "common.names.fullname" .) "key" "rabbitmq-erlang-cookie" "length" 32 "providedValues" (list "auth.erlangCookie") "context" $)>: error calling include: template: georchestra/charts/database/charts/common/templates/_secrets.tpl:117:6: executing "common.secrets.passwords.manage" at <include "common.errors.upgrade.passwords.empty" (dict "validationErrors" $passwordValidationErrors "context" $.context)>: error calling include: template: georchestra/charts/database/charts/common/templates/_errors.tpl:26:48: executing "common.errors.upgrade.passwords.empty" at <fail>: error calling fail: 
PASSWORDS ERROR: You must provide your current passwords when upgrading the release.
                 Note that even after reinstallation, old credentials may be needed as they may be kept in persistent volume claims.
                 Further information can be obtained at https://docs.bitnami.com/general/how-to/troubleshoot-helm-chart-issues/#credential-errors-while-upgrading-chart-releases
  • works with SP disabled, gateway enabled and rabbitmq enabled -> can't say since gateway currently seems broken
  • works with SP disabled, gateway enabled and rabbitmq disabled -> same here. Not sure this is even a relevant use-case

I only tested with builtin rabbitmq

@edevosc2c
Copy link
Member Author

  Though it is apparently not supported to perform an upgrade (from rabbitmq disabled to rabbitmq enabled): getting
Error: UPGRADE FAILED: template: georchestra/charts/rabbitmq/templates/statefulset.yaml:39:28: executing "georchestra/charts/rabbitmq/templates/statefulset.yaml" at <include (print $.Template.BasePath "/secrets.yaml") .>: error calling include: template: georchestra/charts/rabbitmq/templates/secrets.yaml:27:29: executing "georchestra/charts/rabbitmq/templates/secrets.yaml" at <include "common.secrets.passwords.manage" (dict "secret" (include "common.names.fullname" .) "key" "rabbitmq-erlang-cookie" "length" 32 "providedValues" (list "auth.erlangCookie") "context" $)>: error calling include: template: georchestra/charts/database/charts/common/templates/_secrets.tpl:117:6: executing "common.secrets.passwords.manage" at <include "common.errors.upgrade.passwords.empty" (dict "validationErrors" $passwordValidationErrors "context" $.context)>: error calling include: template: georchestra/charts/database/charts/common/templates/_errors.tpl:26:48: executing "common.errors.upgrade.passwords.empty" at <fail>: error calling fail: 
PASSWORDS ERROR: You must provide your current passwords when upgrading the release.
                 Note that even after reinstallation, old credentials may be needed as they may be kept in persistent volume claims.
                 Further information can be obtained at https://docs.bitnami.com/general/how-to/troubleshoot-helm-chart-issues/#credential-errors-while-upgrading-chart-releases

Fixed in aae415f

Related to issue bitnami/charts#10609 (comment)

Copy link
Member

@jeanpommier jeanpommier left a comment

Choose a reason for hiding this comment

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

Looks good to me. But couldn't really proof-test it, since there is something that prevents gateway from starting (seems unrelated to this PR though)

@edevosc2c edevosc2c merged commit f4e765f into main Nov 7, 2023
1 check passed
@edevosc2c edevosc2c deleted the add-rabbitmq branch November 7, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants