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

Prosody is always restart #83

Open
wrenix opened this issue Aug 1, 2023 · 13 comments · Fixed by #84
Open

Prosody is always restart #83

wrenix opened this issue Aug 1, 2023 · 13 comments · Fixed by #84
Labels
bug Something isn't working enhancement New feature or request

Comments

@wrenix
Copy link
Contributor

wrenix commented Aug 1, 2023

Prosody is always restart on helm-chart run, even no values or chart-version changes.

the anti-pattern is here:
https://github.com/jitsi-contrib/jitsi-helm/blob/3744d68b90c0786d2b458d5af7357fc85ed562c3/charts/prosody/templates/statefulset.yaml#L24C73-L25C5


Therefore the helm-chart should use an hash from the values (instatt of an timestamp):

https://helm.sh/docs/chart_template_guide/function_list/#cryptographic-and-security-functions

e.g. (or an subpart of Values and multiple hash):

labels:
hash: {{ toYaml .Values | sha256sum }}

@spijet
Copy link
Collaborator

spijet commented Aug 10, 2023

Hello @wrenix!

Sorry for not answering earlier. Thank you for your PR! I'll review it tomorrow and will let you know if it needs any additional edits before merging.

@wrenix
Copy link
Contributor Author

wrenix commented Aug 11, 2023

thanks, i split it up for an easier review (there are some strange and not well readable expression - so the cleanup is here #86 )

@wrenix
Copy link
Contributor Author

wrenix commented Aug 17, 2023

any update?

@wrenix
Copy link
Contributor Author

wrenix commented Aug 26, 2023

is everything fine with your @spijet ? Or just no time left to speed?

@spijet
Copy link
Collaborator

spijet commented Aug 29, 2023

@wrenix, I'm terribly sorry for the delay — had a lot going on IRL.

I have two minor comments about the #84:

  1. Can you confirm that it plays nice with setups where people don't explicitly set any passwords or secrets, and so they get re-generated on every deploy? (I assume it does, since the secret hash would be different, but some caution won't hurt)
  2. Can you please add the si.jit.meet/ prefix to both annotations to emphasize that it's set by us (Chart developers) and required for smooth operation of the project?

As for #86, I'll have to study it a bit more. I see that you've removed a couple of tpl calls here and there, and some of them may have been used to create semi-dynamic stuff similar to what you can see in Bitnami charts, where you can use templating even inside values. I wanted to implement full-blown templating support for Jitsi chart, but got reluctant and lazy at the same time (reluctant to pull Bitnami's common library chart as a dependency, lazy to implement it myself 😅).

@spijet
Copy link
Collaborator

spijet commented Aug 29, 2023

I had some time to look at #86. Thank you very much! It really makes the chart much more readable and consistent than it was before. I added some comments to it and hopefully will add more tomorrow, after I look at it again with a fresh head. :)

@wrenix
Copy link
Contributor Author

wrenix commented Aug 30, 2023

lets only talk here about #84 (for #86 in there PR).

  1. it is restarted on every change in env, secretEnvs and extraEnvFrom (like any other value inside of the template section). Only on change of values which are references inside of extraEnvFrom, that should not be an job of the helmchart, therefore exists: .Values.podAnnotations
  2. done

@spijet
Copy link
Collaborator

spijet commented Aug 31, 2023

Thank you very much! Can I merge #84 now or should I wait until we finalize #86?

@wrenix
Copy link
Contributor Author

wrenix commented Aug 31, 2023

lets merge it now

@wrenix
Copy link
Contributor Author

wrenix commented Aug 31, 2023

do not forget to increase helm-chart version

@spijet
Copy link
Collaborator

spijet commented Aug 31, 2023

Done! Will file a Release now.

@spijet
Copy link
Collaborator

spijet commented Apr 22, 2024

Hello @wrenix!

It seems that this way of force-rebooting Prosody doesn't really work in the end because of two things:

  1. We don't really have .Values.env in the Prosody sub-chart, so the hash value is always the same;
  2. We feed Prosody with external ConfigMaps and Secrets via .Values.extraEnvFrom and so the sub-chart has no way of peeking inside these ConfigMaps and calculating the hash for them (the result is actually a hash of a YAML of a list of ConfigMap names, not of final environment variables).

At the moment I'm out of ideas, except for maybe introducing a new flag (something like .Values(.prosody).forceRedeploy), that would add the datetime annotation to the StatefulSet.

Maybe you have something else in mind?

@spijet spijet reopened this Apr 22, 2024
@spijet spijet added bug Something isn't working enhancement New feature or request labels Apr 22, 2024
@wrenix
Copy link
Contributor Author

wrenix commented May 26, 2024

If you change something of th values .Values.env or .Values.extraEnvFrom it is an change of the podTemplate, so kubernetes will rollout it.

But yes you are correct, if you change something inside the extra configmaps or secrets, it will not lead to restart.
So the helmchart user should know it (and handle with it with own annotations/labels) or use tools for it (e.g. reloader) or just make a manuelle restart with kubectl.

My opinion that is not a buisness of a/this helmchart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants